Repository navigation
Add Preferences panel for music volume and research prompt setting - #1015
Conversation
|
@ajhalme is changing how audio works in #1009 and adding more audio channels, so please coordinate on how to represent that. More broadly, while I understand this is mostly plumbing, preferences is not an area where we need to faithfully duplicate Civ3. Let's discuss what they should look like, both in-game and at rest, before getting too far into this. |
|
I agree that we don't have to have it faithfully reproduced and could make it better. I can also remove the audio settings as well. My main thing was that I wanted to have a user preference checkbox for promoting for the next tech when you finish your current one. |
|
The Eagle (#1009) has landed. Feel free to refactor the volume management logic out of AudioManager. We've got four buses now: music, sfx, ui, and ambient. Do adjust accordingly. My hope was that we could use a public static string reference to the buses, so we wouldn't have to declare the bus identifer magic string in every context where the bus reference is needed, but meh. Directionally all this looks reasonable, though I'd perhaps abandon the UI side for now and approach it with a fresh Godot scene rather than the popup route. If the audio volume changes get upgraded to play nicely with AudioManager we can get all the new settings stuff in and revisit the UI in another PR. |
799d883 to
23675d2
Compare
…o billy/preferences-panel
The preferences view used a hand-built dark StyleBoxFlat panel. Use the generic advisor frame instead (screens.wonders.background): wood panelling around a light tan interior, with no baked-in widgets. advisors.domestic was the wrong source because it carries the green/red commerce blocks. Layout follows the advisors: root CenterContainer so the 1024x768 texture keeps its native size, title drawn by AdvisorUtils, Game and Units as three-column grids, and the checkbox in the leftmost Game column with its icon left of the label. Also grow the checkbox to cover its own label. Godot cached the bare CheckBox minimum size before the icon/text container was built, so only the 15x15 icon was clickable.
Game, Units and Audio were stacked in one vertically centred box, which left them bunched in the middle of the panel. Give each section its own container that expands to fill an equal third of the inner frame, with the section contents centred inside its band.
Review feedback: use the same nameof convention as C7Settings.LastGame so section and key names are consistent, and accept the minor breakage on a brand new feature. Sections/keys become [Audio] MusicVolume, SfxAudioVolume, UiAudioVolume, AmbienceAudioVolume and [Preferences] PromptForResearch. ConfigureVolume now reads through GetSettingsValueOrDefault. A missing key returns null from GetSettingValue, so int.Parse threw ArgumentNullException, which the catch (ApplicationException) did not cover -- it escaped _Ready and left the sfx/ui/ambience players uninitialised. Unset volumes now fall back to 100 instead. Also drop eleven dead `= null` lines from main_menu.tscn; MenuButtonContainer exposes those as plain properties, not exports, so Godot ignored them.
I believe that i have fixed all of the issues that you wanted.
|
All three Godot export jobs died during savepack with
System.ApplicationException: Media path not found:
Art/3checkboxes-USE.pcx
at Civ3Checkbox..ctor() ... Civ3Checkbox.cs:15
reached through CreateManagedForGodotObjectScriptInstance, i.e. Godot
instantiating the script while packing a scene. C7.ini is gitignored, so
CI has no civ3InstallDir and useStandaloneMode defaults to false: the
config resolves ui.checkbox.* to the real Civ3 PCX, which cannot be
found without a Civ3 install.
preferences.tscn instances a Civ3Checkbox, and main_menu.tscn and
C7Game.tscn instance preferences.tscn, so this was the first scene to
reach that constructor during export.
Civ3Checkbox was the only control that loaded a texture outside _Ready.
Civ3TextureRect, which main_menu's background has always used, loads in
_Ready and was fine in CI, so move the three loads there too, ahead of
the textureRect that consumes them. Runtime is unchanged: the same keys
resolve in either mode, and standalone/textures.lua derives its config
from the civ3 one with the paths rewritten to PNG.
Also restore the trailing newline in PreferencesSettings.cs, which
dotnet format flagged.
|
This is great. Functional, even responsive -- main menu sliders change the music playing in the background!
Some of the comments, whitespacing and code layout feel a little off (see AudioManager), but I'm happy to take this as is. As a quality of life upgrade, we could add a sample to each non-music slider so their relative volume would be apparent while adjusting. But that can wait. I'll let this simmer for a day or two in case others have stronger feelings about the implementation. |
|
It works as expected, thanks. It sure is a nice quality of life feature. I only have two questions, but these are not blocking or anything. First, should we just get rid of the The other is this
I know we do this in a couple of places, so I am curious, is there a particular reason why this isn't an --
what do you mean, is there something wrong with it? I am not seeing what you are seeing? AJ, I think this PR also makes apparent that we should have added a master bus to the Audio Manager too 😄 |
I was trying to go with something consistent with what we already have. I think that sample sounds for the sliders would be good, but as a follow on. |
|
I'm also not super familiar with the Audio settings. They were more of an add on. The main reason I picked this up was to implement the checkbox for Prompt on next research, and then I have a follow on PR to actually implement that part. |
|
I was looking at something else completely, when I saw in the original source files this
I didn't manage to trigger it, and seems to be a legacy thing, perhaps its functionality removed in/before Conquests. So do we keep our equivalent since it doesn't do anything, or rather does the same with Preferences? |
I totally missed this functionality during the first round. CTRL-P should toggle the preferences screen open and closed when in game. the note is on the in game menu as well. |
|
Sorry, my comment wasn't clear enough. So, the question is, do we keep I thought CTRL+P worked in this PR, or at least I remember seeing that a hotkey was added somewhere in the godot files. |
Agree. AudioPreferences button can be removed as this combined settings page takes care of it all. We can look into splitting things up if this gets unwieldy in the future.
I was pattern matching on some gdscript code where the AudioManager is defined as a global thing available to all scripts. I figured we could have a static thing similar to
Fine to use/leave as is. Just unnecessary braiding of the wonders stuff into the settings world. We could have e.g.,
Yeah, we could have extra controls for the master bus, sure. I can take that on once this lands and clean up a few related things while I'm in there.
You can call me Antti - despite my handle, nobody calls me "AJ" 😄 |
That's fine, I get it. Something that I have though about in the past, is to make these node strings into constants at least. We load a few nodes like this around the project, some multiple tiles, might be worth it.
I still feel like I am missing something here, but it doesn't sound serious, so ok, let's leave it as is.
Roger that! |
|
the CTRL+P was in the in game text but it didnt actually work in game until after I pushed the last commit. I checked and it works now. I dont see any buttons for Audio Preferences on the Main menu or the in game menu, so I'm not sure what you guys are referencing. it may be in the code but hidden on the UI. |
4 participants
Adds a Civ3-style user Preferences panel, reachable from the main menu, the in-game
GameMenu, and Ctrl-P. This is the preferences plumbing only; it does not yet change research behaviour.Sections
Only currently available controls are wired up, per the plan to avoid stubbing out settings that do nothing yet.
Notes for review
Reuses the existing
[audio] musicVolumekey.C7Settings.Audiouses lower-case literals rather thannameofdeliberately: ini-parser treats section and key names as case-sensitive, andnameofwould have written a duplicate[Audio]/MusicVolumesection and silently reset everyone's music volume to 100%. Covered byTestMusicVolumeUsesExistingIniNames.Civ3Checkboxnow sizes to its content. It overrides_GetMinimumSize(), but Godot was not honouring that, so the control stayed 16x24px while its label was 170px wide — only the icon was clickable, not the text. Fixed by settingCustomMinimumSizefrom the box container's combined minimum size in_Ready.Civ3Checkboxhad no prior callers, so this is its first use.Main menu scene gained a
PopupOverlay(PopupLayer/PopupOverlay) plus apopupOverlayexport, matchingC7Game.tscn. The existing hidden "Audio Preferences" button was left hidden; its handler is now removed so it cannot point at the wrong panel.Typed accessors (
GetBoolOrDefault,GetIntOrDefault,SetBool) keep ini parsing in one place instead of at each call site.Popup title nit:
GameMenu's popup header still reads "Main Menu" even though it is the in-game menu. Left as-is to keep this PR focused; happy to fix separately.Testing
EngineTests/GameData/C7SettingsTest.cs— defaults, round-trips, malformed values, and the ini-name casing above. Uses an in-memoryIniDataso it never touches the realC7.ini, and never callsSaveSettings().C7.iniafter firing the checkbox and slider signals. No overlap or clipping.main_menu.tscn, confirming the Preferences button is visible, and callingPreferences()to confirm the overlay opens with aPreferencesPanel.build 0 errors / 89 tests pass /
dotnet format --verify-no-changesclean.Related: #1003 (free tech advances and the research picker), which this PR supports but does not fix.