fix(ui): pre-merge code review — apply persisted settings without devtools, hide inert sliders
Two should-fix items from the pre-merge code review pass: 1. Persisted settings now apply on startup unconditionally (previously gated on ACDREAM_DEVTOOLS=1). 2. Music + Ambient volume sliders are hidden because the underlying engine paths don't exist yet (R5 MIDI playback). == 1. Settings load + apply outside DevToolsEnabled gate == Previous structure put SettingsStore construction, LoadDisplay / LoadAudio / etc, and ApplyDisplayWindowState inside the `if (DevToolsEnabled)` block. A user running with the env var unset silently got WindowOptions defaults (1280x720 / VSync=false / 60° FOV) instead of their saved settings.json values — even though the settings file existed and was valid. Refactored: extracted LoadAndApplyPersistedSettings() that runs unconditionally in OnLoad after _audioEngine is constructed but before the DevToolsEnabled block. Persisted values cached as _persistedDisplay / _persistedAudio / _persistedGameplay / _persistedChat / _persistedCharacter fields. The Settings PANEL construction (devtools-gated, naturally — no UI without ImGui) now reads those fields when wiring SettingsVM. The Settings UI gating is correct (panel needs ImGui devtools); the persisted-runtime-state gating was the bug. == 2. Music + Ambient sliders hidden == OpenAlAudioEngine has Music/MusicVolume/Ambient/AmbientVolume properties but they're never read — PlayMusic is a stub for R5 MIDI playback that hasn't shipped, StartAmbient reserves a handle but doesn't start a source. Dragging those sliders moved a number that nothing observed. Hid the Music + Ambient sliders from RenderAudioTab; left the AudioSettings record fields intact so settings.json round-trips the values across phases — when R5 lands and the sliders return, saved values will already be in place. Updated the panel's footer note to call out the limitation. Updated Audio_tab_when_active_renders_implemented_volume_sliders to assert Master + SFX are present AND Music + Ambient are absent. dotnet build green; dotnet test 1,309 / 1,309 green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Erik
parent
944a0364c5
commit
a37ebdebff
3 changed files with 114 additions and 62 deletions
|
|
@ -816,6 +816,17 @@ public sealed class GameWindow : IDisposable
|
|||
}
|
||||
}
|
||||
|
||||
// L.0 follow-up — load + apply persisted Display / Audio settings
|
||||
// BEFORE the DevToolsEnabled block. The settings.json values
|
||||
// (resolution, vsync, FOV, master volume, etc) are runtime
|
||||
// settings, not devtools settings — a user running without
|
||||
// ACDREAM_DEVTOOLS=1 still expects their saved values to take
|
||||
// effect. The Settings PANEL (editing UI) is gated on devtools;
|
||||
// the persisted state is not. Caches values into fields so the
|
||||
// SettingsVM construction in the devtools block reads them
|
||||
// without re-loading.
|
||||
LoadAndApplyPersistedSettings();
|
||||
|
||||
// Phase D.2a — ImGui devtools overlay. Zero cost when the env var
|
||||
// isn't set: no context creation, no per-frame branches hit.
|
||||
// See docs/plans/2026-04-24-ui-framework.md + memory/project_ui_architecture.md.
|
||||
|
|
@ -905,48 +916,15 @@ public sealed class GameWindow : IDisposable
|
|||
// the draft. Construction is null-safe vs. the
|
||||
// dispatcher because the dispatcher is built earlier in
|
||||
// the same OnLoad path (see _inputDispatcher field).
|
||||
if (_inputDispatcher is not null)
|
||||
if (_inputDispatcher is not null && _settingsStore is not null)
|
||||
{
|
||||
// L.0 — settings.json (display + audio + gameplay + chat
|
||||
// + character). Coexists with keybinds.json, which
|
||||
// keeps its own load/save path. Field-stored so the
|
||||
// post-EnterWorld branch can re-load the chosen
|
||||
// toon's character bag.
|
||||
_settingsStore = new AcDream.UI.Abstractions.Panels.Settings.SettingsStore(
|
||||
AcDream.UI.Abstractions.Panels.Settings.SettingsStore.DefaultPath());
|
||||
var settingsStore = _settingsStore;
|
||||
var persistedDisplay = settingsStore.LoadDisplay();
|
||||
var persistedAudio = settingsStore.LoadAudio();
|
||||
var persistedGameplay = settingsStore.LoadGameplay();
|
||||
var persistedChat = settingsStore.LoadChat();
|
||||
// Character bag is loaded against _activeToonKey ("default"
|
||||
// until BeginLiveSessionAsync swaps in the real name).
|
||||
var persistedCharacter = settingsStore.LoadCharacter(_activeToonKey);
|
||||
|
||||
// L.0 Display tab — apply persisted window-level
|
||||
// settings BEFORE the first frame so the user's saved
|
||||
// VSync / Resolution / Fullscreen take effect from
|
||||
// the start instead of flashing the WindowOptions
|
||||
// defaults. FOV and ShowFps come through the
|
||||
// per-frame push in OnRender so live preview works.
|
||||
if (_window is not null)
|
||||
{
|
||||
if (_window.VSync != persistedDisplay.VSync)
|
||||
_window.VSync = persistedDisplay.VSync;
|
||||
ApplyDisplayWindowState(persistedDisplay);
|
||||
}
|
||||
|
||||
// Apply persisted audio to the engine BEFORE the panel
|
||||
// host starts pushing per-frame so the first frame uses
|
||||
// the user's saved values instead of engine defaults.
|
||||
if (_audioEngine is not null && _audioEngine.IsAvailable)
|
||||
{
|
||||
_audioEngine.MasterVolume = persistedAudio.Master;
|
||||
_audioEngine.MusicVolume = persistedAudio.Music;
|
||||
_audioEngine.SfxVolume = persistedAudio.Sfx;
|
||||
_audioEngine.AmbientVolume = persistedAudio.Ambient;
|
||||
}
|
||||
|
||||
// L.0 — SettingsStore + persisted-settings load + apply
|
||||
// happened earlier in OnLoad via
|
||||
// LoadAndApplyPersistedSettings (settings are runtime
|
||||
// state, not devtools state — they take effect even
|
||||
// when ACDREAM_DEVTOOLS=0). Here we just construct the
|
||||
// Settings PANEL on top of the already-loaded values.
|
||||
var settingsStore = _settingsStore;
|
||||
_settingsVm = new AcDream.UI.Abstractions.Panels.Settings.SettingsVM(
|
||||
persisted: _keyBindings,
|
||||
dispatcher: _inputDispatcher,
|
||||
|
|
@ -966,7 +944,7 @@ public sealed class GameWindow : IDisposable
|
|||
Console.WriteLine($"keybinds: save failed: {ex.Message}");
|
||||
}
|
||||
},
|
||||
persistedDisplay: persistedDisplay,
|
||||
persistedDisplay: _persistedDisplay,
|
||||
onSaveDisplay: display =>
|
||||
{
|
||||
try
|
||||
|
|
@ -987,7 +965,7 @@ public sealed class GameWindow : IDisposable
|
|||
Console.WriteLine($"settings: display save failed: {ex.Message}");
|
||||
}
|
||||
},
|
||||
persistedAudio: persistedAudio,
|
||||
persistedAudio: _persistedAudio,
|
||||
onSaveAudio: audio =>
|
||||
{
|
||||
try
|
||||
|
|
@ -1002,7 +980,7 @@ public sealed class GameWindow : IDisposable
|
|||
Console.WriteLine($"settings: audio save failed: {ex.Message}");
|
||||
}
|
||||
},
|
||||
persistedGameplay: persistedGameplay,
|
||||
persistedGameplay: _persistedGameplay,
|
||||
onSaveGameplay: gameplay =>
|
||||
{
|
||||
try
|
||||
|
|
@ -1020,7 +998,7 @@ public sealed class GameWindow : IDisposable
|
|||
Console.WriteLine($"settings: gameplay save failed: {ex.Message}");
|
||||
}
|
||||
},
|
||||
persistedChat: persistedChat,
|
||||
persistedChat: _persistedChat,
|
||||
onSaveChat: chat =>
|
||||
{
|
||||
try
|
||||
|
|
@ -1039,7 +1017,7 @@ public sealed class GameWindow : IDisposable
|
|||
Console.WriteLine($"settings: chat save failed: {ex.Message}");
|
||||
}
|
||||
},
|
||||
persistedCharacter: persistedCharacter,
|
||||
persistedCharacter: _persistedCharacter,
|
||||
onSaveCharacter: character =>
|
||||
{
|
||||
try
|
||||
|
|
@ -5430,6 +5408,67 @@ public sealed class GameWindow : IDisposable
|
|||
// first EnterWorld.
|
||||
private AcDream.UI.Abstractions.Panels.Settings.SettingsStore? _settingsStore;
|
||||
private string _activeToonKey = "default";
|
||||
// L.0 follow-up: persisted-settings cache populated by
|
||||
// LoadAndApplyPersistedSettings (runs unconditionally in OnLoad,
|
||||
// not gated on DevToolsEnabled). The Settings PANEL construction
|
||||
// — which IS gated on devtools — reads these fields when wiring
|
||||
// SettingsVM. Defaults are placeholders; LoadAndApplyPersistedSettings
|
||||
// overwrites them with values from settings.json (or per-section
|
||||
// defaults when the file is missing/corrupt).
|
||||
private AcDream.UI.Abstractions.Panels.Settings.DisplaySettings _persistedDisplay
|
||||
= AcDream.UI.Abstractions.Panels.Settings.DisplaySettings.Default;
|
||||
private AcDream.UI.Abstractions.Panels.Settings.AudioSettings _persistedAudio
|
||||
= AcDream.UI.Abstractions.Panels.Settings.AudioSettings.Default;
|
||||
private AcDream.UI.Abstractions.Panels.Settings.GameplaySettings _persistedGameplay
|
||||
= AcDream.UI.Abstractions.Panels.Settings.GameplaySettings.Default;
|
||||
private AcDream.UI.Abstractions.Panels.Settings.ChatSettings _persistedChat
|
||||
= AcDream.UI.Abstractions.Panels.Settings.ChatSettings.Default;
|
||||
private AcDream.UI.Abstractions.Panels.Settings.CharacterSettings _persistedCharacter
|
||||
= AcDream.UI.Abstractions.Panels.Settings.CharacterSettings.Default;
|
||||
|
||||
/// <summary>
|
||||
/// L.0 follow-up: load every section from settings.json + apply the
|
||||
/// runtime-affecting ones (Display window state + Audio engine
|
||||
/// volumes) at startup. Runs unconditionally — settings are runtime
|
||||
/// state, not devtools state. Without this, a user running with
|
||||
/// <c>ACDREAM_DEVTOOLS=0</c> would silently get WindowOptions
|
||||
/// defaults instead of their saved Display/Audio preferences.
|
||||
/// </summary>
|
||||
private void LoadAndApplyPersistedSettings()
|
||||
{
|
||||
_settingsStore = new AcDream.UI.Abstractions.Panels.Settings.SettingsStore(
|
||||
AcDream.UI.Abstractions.Panels.Settings.SettingsStore.DefaultPath());
|
||||
_persistedDisplay = _settingsStore.LoadDisplay();
|
||||
_persistedAudio = _settingsStore.LoadAudio();
|
||||
_persistedGameplay = _settingsStore.LoadGameplay();
|
||||
_persistedChat = _settingsStore.LoadChat();
|
||||
// _activeToonKey is "default" pre-EnterWorld; the post-login
|
||||
// branch in BeginLiveSessionAsync swaps to the chosen toon's
|
||||
// name and re-loads via SettingsVM.LoadCharacterContext.
|
||||
_persistedCharacter = _settingsStore.LoadCharacter(_activeToonKey);
|
||||
|
||||
// Apply Display to the Silk.NET window. VSync goes via the
|
||||
// window property; resolution + fullscreen go through
|
||||
// ApplyDisplayWindowState which is shared with the on-Save path.
|
||||
if (_window is not null)
|
||||
{
|
||||
if (_window.VSync != _persistedDisplay.VSync)
|
||||
_window.VSync = _persistedDisplay.VSync;
|
||||
ApplyDisplayWindowState(_persistedDisplay);
|
||||
}
|
||||
|
||||
// Apply Audio to the OpenAL engine. Master + Sfx are wired
|
||||
// through to the engine; Music + Ambient are stored but inert
|
||||
// until R5 MIDI/ambient-loop engines exist (assigning them is
|
||||
// harmless — the engine just doesn't read them yet).
|
||||
if (_audioEngine is not null && _audioEngine.IsAvailable)
|
||||
{
|
||||
_audioEngine.MasterVolume = _persistedAudio.Master;
|
||||
_audioEngine.MusicVolume = _persistedAudio.Music;
|
||||
_audioEngine.SfxVolume = _persistedAudio.Sfx;
|
||||
_audioEngine.AmbientVolume = _persistedAudio.Ambient;
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// L.0 Display tab: framebuffer-resize handler — update GL viewport
|
||||
|
|
|
|||
|
|
@ -226,11 +226,15 @@ public sealed class SettingsPanel : IPanel
|
|||
}
|
||||
|
||||
/// <summary>
|
||||
/// Render the Audio tab — four volume sliders (Master / Music / SFX /
|
||||
/// Ambient). Volumes update <i>live</i>: the host pushes the VM's
|
||||
/// AudioDraft into the running OpenAL engine each frame, so dragging
|
||||
/// a slider is audible immediately. Cancel reverts the draft and the
|
||||
/// engine catches up on the next frame.
|
||||
/// Render the Audio tab — Master + SFX volume sliders (live preview
|
||||
/// against the running OpenAL engine). Music + Ambient fields exist
|
||||
/// in <see cref="AudioSettings"/> and persist round-trip, but their
|
||||
/// sliders are intentionally hidden here because the underlying
|
||||
/// engine paths (PlayMusic / StartAmbient) are stubbed for R5 MIDI
|
||||
/// playback that hasn't shipped yet — exposing the sliders would be
|
||||
/// "moving a knob that does nothing." When R5 lands, restore the
|
||||
/// hidden sliders below and the JSON-persisted values will already
|
||||
/// be in place.
|
||||
/// </summary>
|
||||
private void RenderAudioTab(IPanelRenderer renderer)
|
||||
{
|
||||
|
|
@ -240,22 +244,26 @@ public sealed class SettingsPanel : IPanel
|
|||
if (renderer.SliderFloat("Master", ref master, 0f, 1f))
|
||||
_vm.SetAudio(a with { Master = master });
|
||||
|
||||
float music = a.Music;
|
||||
if (renderer.SliderFloat("Music", ref music, 0f, 1f))
|
||||
_vm.SetAudio(a with { Music = music });
|
||||
|
||||
float sfx = a.Sfx;
|
||||
if (renderer.SliderFloat("SFX", ref sfx, 0f, 1f))
|
||||
_vm.SetAudio(a with { Sfx = sfx });
|
||||
|
||||
float ambient = a.Ambient;
|
||||
if (renderer.SliderFloat("Ambient", ref ambient, 0f, 1f))
|
||||
_vm.SetAudio(a with { Ambient = ambient });
|
||||
// Music + Ambient hidden until R5 MIDI / ambient-loop engines
|
||||
// exist. AudioSettings still carries the fields so the JSON
|
||||
// round-trips and a future client doesn't drop them on save.
|
||||
//
|
||||
// float music = a.Music;
|
||||
// if (renderer.SliderFloat("Music", ref music, 0f, 1f))
|
||||
// _vm.SetAudio(a with { Music = music });
|
||||
// float ambient = a.Ambient;
|
||||
// if (renderer.SliderFloat("Ambient", ref ambient, 0f, 1f))
|
||||
// _vm.SetAudio(a with { Ambient = ambient });
|
||||
|
||||
renderer.Spacing();
|
||||
renderer.TextWrapped(
|
||||
"Volume changes preview live as you drag. Save persists the "
|
||||
+ "values to settings.json; Cancel reverts to the saved values.");
|
||||
+ "values to settings.json; Cancel reverts to the saved values. "
|
||||
+ "Music + Ambient mixing arrives with R5 MIDI playback.");
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
|
|
|
|||
|
|
@ -346,8 +346,13 @@ public sealed class SettingsPanelTests
|
|||
// -- Audio tab content -----------------------------------------------
|
||||
|
||||
[Fact]
|
||||
public void Audio_tab_when_active_renders_four_volume_sliders()
|
||||
public void Audio_tab_when_active_renders_implemented_volume_sliders()
|
||||
{
|
||||
// L.0 ships Master + SFX only — Music + Ambient sliders are
|
||||
// hidden until R5 MIDI / ambient-loop engines exist. The
|
||||
// AudioSettings record still carries those fields so the
|
||||
// JSON round-trips, but the panel doesn't surface a slider
|
||||
// that wouldn't actually do anything.
|
||||
var (panel, _, _, _) = Build();
|
||||
var r = new FakePanelRenderer { ActiveTabLabel = "Audio" };
|
||||
|
||||
|
|
@ -356,9 +361,9 @@ public sealed class SettingsPanelTests
|
|||
var sliders = r.Calls.Where(c => c.Method == "SliderFloat")
|
||||
.Select(c => (string)c.Args[0]!).ToList();
|
||||
Assert.Contains("Master", sliders);
|
||||
Assert.Contains("Music", sliders);
|
||||
Assert.Contains("SFX", sliders);
|
||||
Assert.Contains("Ambient", sliders);
|
||||
Assert.DoesNotContain("Music", sliders);
|
||||
Assert.DoesNotContain("Ambient", sliders);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue