Adopt observer-based settings with automatic save and UI sync#469
Merged
Adopt observer-based settings with automatic save and UI sync#469
Conversation
## TEST SUITE: Verifies the Observer Pattern implementation for game settings. ## This suite ensures that UI-driven changes to GameSettingsResource correctly ## emit signals and that Globals.gd reacts by persisting data, thereby ## decoupling UI logic from the persistence layer.
This Pull Request addresses **Issue #432** by refactoring the settings system to use the **Observer Pattern**, effectively decoupling the UI layer from persistence and logging logic. ### Summary of Changes * **Observer Pattern Implementation**: * **Subject**: Enhanced `GameSettingsResource` with a `setting_changed` signal and property setters to automatically notify observers of state changes. * **Observer (Persistence)**: Refactored `Globals.gd` to listen for the `setting_changed` signal, centralizing all `log_message()` and `_save_settings()` calls in a single reactive handler. * **Observer (UI)**: Updated `gameplay_settings.gd` and `advanced_settings.gd` to sync their internal states when the resource is updated externally (e.g., during initialization or a reset). * **Decoupling & Cleanup**: * Removed "Leaky Abstractions" by stripping manual `_save_settings()` and logging calls from UI scripts; they now only update the resource data. * **Testing**: * Added a comprehensive GUT suite (`test_settings_observer.gd`) covering signal integrity, data clamping, and automated persistence. * Resolved strict typing errors and API mismatches in the test environment.
ikostan
added this to the
Milestone 13: Resource Persistence & Core Gameplay Signals milestone
Contributor
Reviewer's GuideIntroduce an observer-based settings flow where GameSettingsResource emits setting_changed signals, Globals listens for those to log and persist automatically, and the gameplay/advanced settings UIs plus tests are updated to align with this reactive, clamped, and auto-synced model. Sequence diagram for observer-based settings update from UI to persistencesequenceDiagram
actor Player
participant GameplaySettings as GameplaySettingsMenu
participant GameSettingsResource
participant Globals
participant ConfigFile as SettingsConfig
Player->>GameplaySettings: Adjust difficulty slider(value)
GameplaySettings->>GameplaySettings: _on_difficulty_value_changed(value)
GameplaySettings->>GameSettingsResource: set difficulty(value)
GameSettingsResource->>GameSettingsResource: clamp(value, 0.5, 2.0)
GameSettingsResource-->>GameSettingsResource: _difficulty updated
GameSettingsResource-->>GameplaySettings: setting_changed(difficulty, clamped_value)
GameSettingsResource-->>Globals: setting_changed(difficulty, clamped_value)
Globals->>Globals: _on_setting_changed(difficulty, clamped_value)
Globals->>Globals: log_message("Setting 'difficulty' updated to: ...")
Globals->>SettingsConfig: _save_settings()
SettingsConfig-->>Globals: save OK
GameplaySettings->>GameplaySettings: difficulty_slider.value = settings.difficulty
GameplaySettings->>GameplaySettings: difficulty_label.text = "{" + settings.difficulty + "}"
Sequence diagram for external settings change syncing back to UIsequenceDiagram
participant ExternalSystem as ExternalSystem
participant GameSettingsResource
participant Globals
participant GameplaySettings as GameplaySettingsMenu
ExternalSystem->>GameSettingsResource: set difficulty(new_value)
GameSettingsResource->>GameSettingsResource: clamp(new_value, 0.5, 2.0)
GameSettingsResource-->>GameSettingsResource: _difficulty updated
GameSettingsResource-->>Globals: setting_changed(difficulty, clamped_value)
GameSettingsResource-->>GameplaySettings: setting_changed(difficulty, clamped_value)
Globals->>Globals: _on_setting_changed(difficulty, clamped_value)
Globals->>Globals: log_message("Setting 'difficulty' updated to: ...")
Globals->>Globals: _save_settings()
GameplaySettings->>GameplaySettings: _on_external_setting_changed(difficulty, clamped_value)
GameplaySettings->>GameplaySettings: difficulty_slider.set_value_no_signal(clamped_value)
GameplaySettings->>GameplaySettings: difficulty_label.text = "{" + str(clamped_value) + "}"
Class diagram for observer-based GameSettingsResource and listenersclassDiagram
class GameSettingsResource {
<<Resource>>
+signal setting_changed(setting_name: String, new_value: Variant)
+current_log_level: int
+enable_debug_logging: bool
+difficulty: float
+main_menu_scene: PackedScene
+level_select_scene: PackedScene
+training_scene: PackedScene
+key_mapping_scene: PackedScene
+options_scene: PackedScene
-_current_log_level: int
-_difficulty: float
-_enable_debug_logging: bool
+set_current_log_level(value: int) void
+get_current_log_level() int
+set_enable_debug_logging(value: bool) void
+get_enable_debug_logging() bool
+set_difficulty(value: float) void
+get_difficulty() float
}
class Globals {
<<Node>>
+settings: GameSettingsResource
+_ready() void
+_on_setting_changed(setting_name: String, new_value: Variant) void
+_load_settings(path: String) void
+_save_settings(path: String) void
+log_message(message: String, level: int) void
}
class GameplaySettingsMenu {
<<Control>>
+difficulty_slider
+difficulty_label
+_ready() void
+_on_difficulty_value_changed(value: float) void
+_on_external_setting_changed(setting_name: String, new_value: Variant) void
+_on_tree_exited() void
}
class AdvancedSettingsMenu {
<<Control>>
+log_lvl_option
+_on_log_level_item_selected(index: int) void
}
GameSettingsResource <.. Globals : observes
GameSettingsResource <.. GameplaySettingsMenu : observes
GameSettingsResource <.. AdvancedSettingsMenu : updated_by
Globals o-- GameSettingsResource : settings
GameplaySettingsMenu --> Globals : uses
AdvancedSettingsMenu --> Globals : uses
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Python |  |
Mar 16, 2026 5:16a.m. | Review ↗ |
| JavaScript | |
Mar 16, 2026 5:16a.m. | Review ↗ |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The new setters for
current_log_levelandenable_debug_loggingassign back to the same property, which will recurse indefinitely; use a backing field (e.g._current_log_level) orset_deferred/set()on the underlying data instead. - In
gameplay_settings.gd, the connection toGlobals.settings.setting_changedis never disconnected in_on_tree_exited, and it assumesGlobals.settingsis always non-null—consider guarding against a missing settings resource and explicitly disconnecting in the cleanup path to avoid leaks or stray callbacks. - Several comments contain
[cite: ...]markers (e.g. ingame_settings_resource.gdand tests); these look like tooling artifacts and should be removed or replaced with clearer internal references before merging.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new setters for `current_log_level` and `enable_debug_logging` assign back to the same property, which will recurse indefinitely; use a backing field (e.g. `_current_log_level`) or `set_deferred`/`set()` on the underlying data instead.
- In `gameplay_settings.gd`, the connection to `Globals.settings.setting_changed` is never disconnected in `_on_tree_exited`, and it assumes `Globals.settings` is always non-null—consider guarding against a missing settings resource and explicitly disconnecting in the cleanup path to avoid leaks or stray callbacks.
- Several comments contain `[cite: ...]` markers (e.g. in `game_settings_resource.gd` and tests); these look like tooling artifacts and should be removed or replaced with clearer internal references before merging.
## Individual Comments
### Comment 1
<location path="scripts/game_settings_resource.gd" line_range="20-23" />
<code_context>
# Current log level: 0=DEBUG, 1=INFO, 2=WARNING, 3=ERROR, 4=NONE
-@export_range(0, 4, 1) var current_log_level: int = 1
-@export var enable_debug_logging: bool = false
+@export_range(0, 4, 1) var current_log_level: int = 1:
+ set(value):
+ current_log_level = value
+ setting_changed.emit("current_log_level", value)
+
+@export var enable_debug_logging: bool = false:
</code_context>
<issue_to_address>
**issue (bug_risk):** Setter for `current_log_level` is recursively assigning to itself and will never terminate.
Inside the setter, `current_log_level = value` re-enters the same setter and will recurse indefinitely. Use a separate backing field or write to the underlying storage instead. For example:
```gdscript
var _current_log_level := 1
@export_range(0, 4, 1) var current_log_level: int = 1:
set(value):
_current_log_level = value
setting_changed.emit("current_log_level", value)
get:
return _current_log_level
```
</issue_to_address>
### Comment 2
<location path="scripts/game_settings_resource.gd" line_range="25-28" />
<code_context>
+ current_log_level = value
+ setting_changed.emit("current_log_level", value)
+
+@export var enable_debug_logging: bool = false:
+ set(value):
+ enable_debug_logging = value
+ setting_changed.emit("enable_debug_logging", value)
@export_group("Gameplay")
</code_context>
<issue_to_address>
**issue (bug_risk):** The `enable_debug_logging` setter has the same self-recursive assignment problem as `current_log_level`.
Assigning to `enable_debug_logging` inside its own setter will recurse indefinitely. Use a separate backing field (e.g. `_enable_debug_logging`) and update it in the setter while emitting the signal.
</issue_to_address>
### Comment 3
<location path="scripts/gameplay_settings.gd" line_range="44" />
<code_context>
tree_exited.connect(_on_tree_exited)
+
+ # NEW: The UI now observes the resource for external changes
+ Globals.settings.setting_changed.connect(_on_external_setting_changed)
if os_wrapper.has_feature("web"):
</code_context>
<issue_to_address>
**issue (bug_risk):** New connection to `Globals.settings.setting_changed` can be created multiple times without being disconnected.
If this node leaves and re-enters the scene tree, `_ready` may run multiple times and create duplicate connections, so `_on_external_setting_changed` would be invoked multiple times per signal. Since you already handle cleanup in `_on_tree_exited`, also disconnect this signal there and/or guard the connection with:
```gdscript
if not Globals.settings.setting_changed.is_connected(_on_external_setting_changed):
Globals.settings.setting_changed.connect(_on_external_setting_changed)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughGameSettingsResource now emits Changes
Sequence DiagramsequenceDiagram
participant UI as UI Component (gameplay_settings)
participant Resource as GameSettingsResource
participant Globals as Globals
participant Disk as Config File
UI->>Resource: set difficulty / change setting
activate Resource
Resource->>Resource: update internal value / clamp
Resource->>Resource: emit setting_changed(setting_name, new_value)
deactivate Resource
Resource->>Globals: setting_changed(setting_name, new_value)
activate Globals
Globals->>Globals: log change
Globals->>Globals: _save_settings()
Globals->>Disk: write config file
deactivate Globals
Resource->>UI: setting_changed(setting_name, new_value)
activate UI
UI->>UI: update controls/labels without saving
deactivate UI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…can be created multiple times without being disconnected. If this node leaves and re-enters the scene tree, _ready may run multiple times and create duplicate connections, so _on_external_setting_changed would be invoked multiple times per signal. Since you already handle cleanup in _on_tree_exited, also disconnect this signal there and/or guard the connection
…ng to itself and will never terminate. Inside the setter, current_log_level = value re-enters the same setter and will recurse indefinitely. Use a separate backing field or write to the underlying storage instead. For example:
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/advanced_settings.gd (1)
162-167:⚠️ Potential issue | 🟡 MinorRemove duplicate
_save_settings()call.Setting
current_log_leveltriggers the setter which emitssetting_changed, and the observer inGlobals._on_setting_changedalready calls_save_settings(). The explicit call here causes a duplicate disk write.This is inconsistent with
_on_log_level_item_selected(Line 272) where the call is correctly commented out.🔧 Proposed fix
func _on_advanced_reset_button_pressed() -> void: Globals.log_message("Advanced Settings reset pressed.", Globals.LogLevel.DEBUG) # Log level should be reset to INFO Globals.settings.current_log_level = Globals.LogLevel.INFO log_lvl_option.selected = Globals.LogLevel.values().find(Globals.LogLevel.INFO) - Globals._save_settings()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/advanced_settings.gd` around lines 162 - 167, In _on_advanced_reset_button_pressed(), remove the explicit duplicate call to Globals._save_settings() because setting Globals.settings.current_log_level uses its setter which emits setting_changed and Globals._on_setting_changed already calls _save_settings(); keep the current_log_level assignment and log_lvl_option.selected update as-is so the observer-driven save handles persistence, mirroring the approach used in _on_log_level_item_selected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@scripts/advanced_settings.gd`:
- Around line 162-167: In _on_advanced_reset_button_pressed(), remove the
explicit duplicate call to Globals._save_settings() because setting
Globals.settings.current_log_level uses its setter which emits setting_changed
and Globals._on_setting_changed already calls _save_settings(); keep the
current_log_level assignment and log_lvl_option.selected update as-is so the
observer-driven save handles persistence, mirroring the approach used in
_on_log_level_item_selected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 21c78242-0ba7-496f-b131-fb93853f9399
📒 Files selected for processing (6)
scripts/advanced_settings.gdscripts/game_settings_resource.gdscripts/gameplay_settings.gdscripts/globals.gdtest/gut/test_settings_observer.gdtest/gut/test_settings_observer.gd.uid
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Browser Functional Tests / test
🔇 Additional comments (13)
test/gut/test_settings_observer.gd.uid (1)
1-1: LGTM!Standard Godot 4.x UID file for test resource tracking.
scripts/globals.gd (2)
52-56: LGTM!Good ordering: connecting the signal after
_load_settings()ensures that loading persisted values doesn't trigger redundant save operations.
58-67: LGTM!Clean reactive handler centralizing persistence. The immediate save on every change is simple and ensures durability.
scripts/advanced_settings.gd (1)
267-272: LGTM!Correct migration to the observer pattern. The commented-out
_save_settings()call documents the intentional change while letting the observer handle persistence automatically.scripts/gameplay_settings.gd (3)
43-45: LGTM!Proper observer hookup allowing the UI to stay synchronized with external resource changes.
86-91: LGTM!Good use of
set_value_no_signal()to update the slider without triggeringvalue_changed, preventing a circular signal loop.
251-256: LGTM!Properly migrated to the observer pattern. The UI only modifies the resource and relies on
Globals._on_setting_changedfor automatic persistence.test/gut/test_settings_observer.gd (4)
15-22: LGTM!Good test isolation - using a dedicated test resource and cleaning up test config files between runs.
25-50: LGTM!Comprehensive signal integrity tests covering both difficulty and log level changes, plus clamping validation.
53-68: LGTM!Correct approach: manually connecting to the test resource and directing saves to
_test_config_pathensures isolation from production config while validating actual persistence behavior.
109-115: LGTM!Good end-to-end test verifying that external resource changes propagate to the UI slider through the observer pattern.
scripts/game_settings_resource.gd (2)
10-17: LGTM!Well-documented signal declaration clearly explaining the Observer pattern intent and usage.
20-35: LGTM!Consistent setter implementation across all observable properties. The difficulty setter correctly emits the clamped value rather than the raw input.
…t problem as current_log_level. issue (bug_risk): The enable_debug_logging setter has the same self-recursive assignment problem as current_log_level. Assigning to enable_debug_logging inside its own setter will recurse indefinitely. Use a separate backing field (e.g. _enable_debug_logging) and update it in the setter while emitting the signal.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
scripts/game_settings_resource.gd (1)
20-25: Consider clampingcurrent_log_levelin the setter for consistency.Unlike
difficultywhich clamps in its setter (line 38),current_log_levelrelies solely on@export_rangewhich only constrains UI input, not programmatic assignments. For defensive consistency, consider adding range enforcement here as well.🔧 Optional fix to add clamping
`@export_range`(0, 4, 1) var current_log_level: int = 1: set(value): - _current_log_level = value + _current_log_level = clampi(value, 0, 4) - setting_changed.emit("current_log_level", value) + setting_changed.emit("current_log_level", _current_log_level) get: return _current_log_level🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/game_settings_resource.gd` around lines 20 - 25, The current_log_level setter should enforce the same 0–4 bounds as the UI export_range to prevent invalid programmatic values; inside the set(value) for current_log_level clamp value to the allowed range (0..4) before assigning to _current_log_level and emitting setting_changed, mirroring the approach used in the difficulty setter to ensure defensive consistency.scripts/gameplay_settings.gd (1)
100-106: Missing disconnect forGlobals.settings.setting_changed.The cleanup disconnects local signals but doesn't disconnect from
Globals.settings.setting_changedconnected at line 45. For consistency with the other explicit disconnects and to prevent callbacks during the exit process, consider adding the disconnect here.🔧 Proposed fix to add missing disconnect
# Disconnect Godot signals if still connected if difficulty_slider.value_changed.is_connected(_on_difficulty_value_changed): difficulty_slider.value_changed.disconnect(_on_difficulty_value_changed) if gameplay_back_button.pressed.is_connected(_on_gameplay_back_button_pressed): gameplay_back_button.pressed.disconnect(_on_gameplay_back_button_pressed) if gameplay_reset_button.pressed.is_connected(_on_gameplay_reset_button_pressed): gameplay_reset_button.pressed.disconnect(_on_gameplay_reset_button_pressed) + if Globals.settings.setting_changed.is_connected(_on_external_setting_changed): + Globals.settings.setting_changed.disconnect(_on_external_setting_changed)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/gameplay_settings.gd` around lines 100 - 106, The cleanup block is missing a disconnect for Globals.settings.setting_changed which was connected earlier; update the same teardown section where difficulty_slider, gameplay_back_button, and gameplay_reset_button are disconnected to also check if Globals.settings.setting_changed.is_connected(_on_setting_changed) and if so call Globals.settings.setting_changed.disconnect(_on_setting_changed) so the Settings callback (_on_setting_changed) is properly removed during exit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/gameplay_settings.gd`:
- Around line 252-257: The label shows the raw parameter instead of the clamped
setting; change code so difficulty_label.text uses the clamped value stored in
Globals.settings.difficulty (not the incoming value) to keep UI consistent:
after assigning Globals.settings.difficulty = value and updating
difficulty_slider.value, set difficulty_label.text to use
Globals.settings.difficulty (formatted as before) and keep logging via
Globals.log_message as-is.
---
Nitpick comments:
In `@scripts/game_settings_resource.gd`:
- Around line 20-25: The current_log_level setter should enforce the same 0–4
bounds as the UI export_range to prevent invalid programmatic values; inside the
set(value) for current_log_level clamp value to the allowed range (0..4) before
assigning to _current_log_level and emitting setting_changed, mirroring the
approach used in the difficulty setter to ensure defensive consistency.
In `@scripts/gameplay_settings.gd`:
- Around line 100-106: The cleanup block is missing a disconnect for
Globals.settings.setting_changed which was connected earlier; update the same
teardown section where difficulty_slider, gameplay_back_button, and
gameplay_reset_button are disconnected to also check if
Globals.settings.setting_changed.is_connected(_on_setting_changed) and if so
call Globals.settings.setting_changed.disconnect(_on_setting_changed) so the
Settings callback (_on_setting_changed) is properly removed during exit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 34f9f2af-1be4-4549-8938-dc4ab61ccb3e
📒 Files selected for processing (2)
scripts/game_settings_resource.gdscripts/gameplay_settings.gd
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Browser Functional Tests / test
🔇 Additional comments (6)
scripts/game_settings_resource.gd (4)
10-16: Well-structured Observer pattern implementation.The signal documentation is clear and the
Varianttype fornew_valueprovides flexibility for different setting types.
27-32: LGTM!Boolean setter correctly uses backing field and emits the signal.
36-41: LGTM!The setter correctly clamps the value before storing and emits the signal with the clamped value, ensuring observers receive the actual applied value.
49-52: LGTM!Backing field defaults correctly match the
@exportdefaults, preventing initialization mismatches.scripts/gameplay_settings.gd (2)
43-45: LGTM!Good use of connection guard to prevent duplicate signal handlers.
87-91: LGTM!Using
set_value_no_signalcorrectly prevents circular signal emissions when external changes update the UI.
Line 254 correctly uses the clamped value from Globals.settings.difficulty for the slider, but line 255 uses the raw value parameter for the label. If a user or JS callback passes a value outside 0.5-2.0 (e.g., 2.5), the slider will show 2.0 but the label will show "{2.5}".
Calling sut._ready() at Line 77 causes globals.gd to reload settings from res://config_resources/default_settings.tres (see globals.gd Line 40), which overwrites the _resource assignment at Line 76. The signal connection then binds to the production resource, not the test resource. The assertion at Line 81 only verifies that _resource.difficulty holds the assigned value—it doesn't confirm that the observer in sut received the signal or triggered persistence. Consider either: Mocking the resource load in _ready(), or Manually connecting the signal after _ready() and verifying persistence to _test_config_path.
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
gameplay_settings.gd,_ready()connects toGlobals.settings.setting_changed, but_on_tree_exited()doesn’t disconnect from that signal; consider explicitly disconnecting there to avoid stale connections if the node is freed and re-instantiated. - In
globals.gd’s_ready(), you connectsettings.setting_changedto_on_setting_changedwithout checkingis_connected; for robustness (especially in editor reloads or tests that may reassignGlobals.settings), mirror the defensive pattern used ingameplay_settings.gdand guard the connection.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `gameplay_settings.gd`, `_ready()` connects to `Globals.settings.setting_changed`, but `_on_tree_exited()` doesn’t disconnect from that signal; consider explicitly disconnecting there to avoid stale connections if the node is freed and re-instantiated.
- In `globals.gd`’s `_ready()`, you connect `settings.setting_changed` to `_on_setting_changed` without checking `is_connected`; for robustness (especially in editor reloads or tests that may reassign `Globals.settings`), mirror the defensive pattern used in `gameplay_settings.gd` and guard the connection.
## Individual Comments
### Comment 1
<location path="scripts/gameplay_settings.gd" line_range="87-91" />
<code_context>
Globals.log_message("Gameplay Settings menu loaded.", Globals.LogLevel.DEBUG)
+func _on_external_setting_changed(setting_name: String, new_value: Variant) -> void:
+ if setting_name == "difficulty":
+ # Update UI without triggering a circular signal loop
+ difficulty_slider.set_value_no_signal(new_value)
+ difficulty_label.text = "{" + str(new_value) + "}"
+
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** UI updates for difficulty are now split between the external observer and the local value-changed handler, which is redundant and risks subtle signal loops.
The UI is now updated in both `_on_external_setting_changed` and `_on_difficulty_value_changed`. The observer sets the slider with `set_value_no_signal` and updates the label from `new_value`, while the local handler updates `Globals.settings.difficulty` (emitting the signal) and then updates the slider and label again from the resource.
This duplication makes the flow harder to follow and increases the risk of subtle signal loops if the slider ever emits on unchanged `value`. Consider having the local handler only update the resource and letting `_on_external_setting_changed` be the single place that updates the UI, or consistently using `set_value_no_signal` for programmatic slider changes.
Suggested implementation:
```
func _on_external_setting_changed(setting_name: String, new_value: Variant) -> void:
if setting_name == "difficulty":
# Single source of truth for difficulty UI: reflect resource changes without emitting signals
difficulty_slider.set_value_no_signal(new_value)
difficulty_label.text = "{" + str(new_value) + "}"
```
You’ll also need to adjust the local difficulty slider handler (and any similar handlers) in the same file:
1. In `func _on_difficulty_value_changed(value: float) -> void` (or similarly named), remove any direct UI updates, keeping only the resource write. For example, change a block like:
- `Globals.settings.difficulty = int(value)`
- `difficulty_slider.value = value` (or `set_value_no_signal`)
- `difficulty_label.text = "{" + str(value) + "}"`
to only:
- `Globals.settings.difficulty = int(value)`
The `setting_changed` signal from the resource will then invoke `_on_external_setting_changed`, which will update the slider and label exactly once, using `set_value_no_signal` to avoid loops.
2. If there are any other places in `gameplay_settings.gd` where the difficulty slider or label are updated programmatically (e.g. when loading settings, resetting to defaults), change those to:
- Write the new value to `Globals.settings.difficulty` instead of directly setting `difficulty_slider` / `difficulty_label`.
- Rely on the `setting_changed` signal + `_on_external_setting_changed` to refresh the UI.
3. Verify that the difficulty slider’s `value_changed` signal is connected only to the local handler (`_on_difficulty_value_changed`) and not to both handlers, so the flow remains: **UI → local handler → resource → setting_changed → external handler → UI**.
</issue_to_address>
### Comment 2
<location path="scripts/advanced_settings.gd" line_range="264-272" />
<code_context>
var selected_enum: Globals.LogLevel = log_level_display_to_enum.get(
selected_name, Globals.LogLevel.INFO
)
+ # Only update the resource; the Observer handles the rest
Globals.settings.current_log_level = selected_enum
log_lvl_option.selected = Globals.LogLevel.values().find(selected_enum)
# Temporary raw print to bypass log_message
Globals.log_message("Log level changed to: " + selected_name, Globals.LogLevel.DEBUG)
- Globals._save_settings()
+ # Globals._save_settings()
</code_context>
<issue_to_address>
**suggestion:** Log messages for log-level changes are now duplicated: once here and once via the centralized Observer.
Since `_on_setting_changed` in `globals.gd` now logs every `current_log_level` change, this `Globals.log_message("Log level changed to: " + selected_name, ...)` causes duplicate entries. Consider removing this call or changing it to a more UI-specific message or different level so it doesn’t overlap with the centralized logging.
```suggestion
var selected_enum: Globals.LogLevel = log_level_display_to_enum.get(
selected_name, Globals.LogLevel.INFO
)
# Only update the resource; the Observer handles the rest (including logging)
Globals.settings.current_log_level = selected_enum
log_lvl_option.selected = Globals.LogLevel.values().find(selected_enum)
# Logging of log-level changes is handled centrally in Globals._on_setting_changed
# Globals._save_settings()
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Owner
Author
|
The following feedback is technically valid but highlights a discrepancy between the architectural goal and the current implementation.
This PR goal was specifically "Decoupling": therefore this work is a successful first step. We have fixed the "Leaky Abstractions" by moving logic to the Resource and Globals, which makes the eventual switch to .tres persistence much easier. Based on the current development status of SkyLockAssault, the issues in Milestone 14 focus on migrating hard-coded elements (Fuel, Speed, Weapons, Bullets, and Globals) into Godot Resources. While this covers the data conversion, there are a few "missing links" in the logic and testing departments that you might want to address with additional issues. Suggested Additional Issues1. [FEATURE] Centralized Resource ManagerCurrently, you have separate issues for converting individual elements (Fuel, Speed, etc.). You may need a dedicated issue to implement a Resource Manager or update the 2. [TEST] Resource Serialization & Persistence ValidationThe feedback from your recent code review highlighted that while the Observer Pattern is in place, you aren't yet fully utilizing
3. [REFACTOR] Signal Decoupling for New ResourcesSince you are adding signals for Fuel and Speed systems, you should ensure these signals are emitted by the Resources themselves rather than the
Milestone 14 Checklist Comparison
In order to address all the above following bugs/feature requests were opened: |
Owner
Author
|
@sourcery-ai title |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/gameplay_settings.gd (1)
87-91: Consolidate duplicated difficulty-UI sync logic into one helper.
_on_external_setting_changedand_on_difficulty_value_changedboth update slider/label state. Extracting a shared UI-sync helper will reduce drift risk.♻️ Suggested refactor
+func _sync_difficulty_ui(value: float, emit_signal: bool) -> void: + if emit_signal: + difficulty_slider.value = value + else: + difficulty_slider.set_value_no_signal(value) + difficulty_label.text = "{" + str(value) + "}" func _on_external_setting_changed(setting_name: String, new_value: Variant) -> void: if setting_name == "difficulty": - # Update UI without triggering a circular signal loop - difficulty_slider.set_value_no_signal(new_value) - difficulty_label.text = "{" + str(new_value) + "}" + _sync_difficulty_ui(float(new_value), false) func _on_difficulty_value_changed(value: float) -> void: # Update the resource first (this triggers clamping in the setter) Globals.settings.difficulty = value # Update the UI components using the ALREADY CLAMPED value from the resource - difficulty_slider.value = Globals.settings.difficulty - difficulty_label.text = "{" + str(Globals.settings.difficulty) + "}" + _sync_difficulty_ui(Globals.settings.difficulty, true)Also applies to: 252-256
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/gameplay_settings.gd` around lines 87 - 91, Create a single helper (e.g., sync_difficulty_ui(value, use_no_signal: bool)) and move the shared UI update logic into it: set the slider value via difficulty_slider.set_value_no_signal(value) when use_no_signal is true or difficulty_slider.set_value(value) otherwise, and update difficulty_label.text = "{" + str(value) + "}". Replace the duplicated blocks in _on_external_setting_changed and _on_difficulty_value_changed to call sync_difficulty_ui(new_value, true) for the external setting path and sync_difficulty_ui(value, false) (or just sync_difficulty_ui(value)) for the local change path so all label/slider updates are centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/gameplay_settings.gd`:
- Around line 87-91: Create a single helper (e.g., sync_difficulty_ui(value,
use_no_signal: bool)) and move the shared UI update logic into it: set the
slider value via difficulty_slider.set_value_no_signal(value) when use_no_signal
is true or difficulty_slider.set_value(value) otherwise, and update
difficulty_label.text = "{" + str(value) + "}". Replace the duplicated blocks in
_on_external_setting_changed and _on_difficulty_value_changed to call
sync_difficulty_ui(new_value, true) for the external setting path and
sync_difficulty_ui(value, false) (or just sync_difficulty_ui(value)) for the
local change path so all label/slider updates are centralized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 07774eac-9223-44bc-a528-45d0e37ab00f
📒 Files selected for processing (3)
scripts/gameplay_settings.gdtest/gut/test_settings_observer.gdtests/difficulty_flow_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
- test/gut/test_settings_observer.gd
📜 Review details
🔇 Additional comments (3)
tests/difficulty_flow_test.py (1)
175-175: Assertion text now correctly matches the observer-based log wording.This update is aligned with the current message emitted by the gameplay settings flow, so the test intent remains correct.
scripts/gameplay_settings.gd (2)
43-45: Observer connection guard is implemented correctly.Using
is_connectedbefore connectingGlobals.settings.setting_changedprevents duplicate signal hookups in normal menu lifecycle.
257-259: Clamped-value logging text update is consistent with the new flow.Logging
"Difficulty updated to: ..."with the clamped resource value keeps messaging consistent with the observer-based settings model.
…n the external observer and the local value-changed handler, which is redundant and risks subtle signal loops. he UI is now updated in both _on_external_setting_changed and _on_difficulty_value_changed. The observer sets the slider with set_value_no_signal and updates the label from new_value, while the local handler updates Globals.settings.difficulty (emitting the signal) and then updates the slider and label again from the resource. This duplication makes the flow harder to follow and increases the risk of subtle signal loops if the slider ever emits on unchanged value. Consider having the local handler only update the resource and letting _on_external_setting_changed be the single place that updates the UI, or consistently using set_value_no_signal for programmatic slider changes.
…ce here and once via the centralized Observer.
Since _on_setting_changed in globals.gd now logs every current_log_level change, this Globals.log_message("Log level changed to: " + selected_name, ...) causes duplicate entries. Consider removing this call or changing it to a more UI-specific message or different level so it doesn’t overlap with the centralized logging.
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
gameplay_settings.gd::_on_external_setting_changed, reassigningGlobals.settings.difficultywhen handling thedifficultychange is redundant because the resource already holdsnew_value(and the setter early-returns on same value); you can drop that assignment and just update the UI to avoid unnecessary calls into the settings resource. - The new
Globals.settings.setting_changedconnection ingameplay_settings.gdis never disconnected in_on_tree_exited, which can leave stale observers around if the menu is created/destroyed multiple times; consider explicitly disconnecting the signal in_on_tree_exited.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `gameplay_settings.gd::_on_external_setting_changed`, reassigning `Globals.settings.difficulty` when handling the `difficulty` change is redundant because the resource already holds `new_value` (and the setter early-returns on same value); you can drop that assignment and just update the UI to avoid unnecessary calls into the settings resource.
- The new `Globals.settings.setting_changed` connection in `gameplay_settings.gd` is never disconnected in `_on_tree_exited`, which can leave stale observers around if the menu is created/destroyed multiple times; consider explicitly disconnecting the signal in `_on_tree_exited`.
## Individual Comments
### Comment 1
<location path="scripts/gameplay_settings.gd" line_range="87-95" />
<code_context>
Globals.log_message("Gameplay Settings menu loaded.", Globals.LogLevel.DEBUG)
+func _on_external_setting_changed(setting_name: String, new_value: Variant) -> void:
+ if setting_name == "difficulty":
+ # Prevent Circular Updates in the UI
+ if difficulty_slider.value != float(new_value):
+ Globals.settings.difficulty = float(new_value)
+ # Update UI without triggering a circular signal loop
+ # Use set_value_no_signal to avoid re-triggering _on_difficulty_value_changed
+ difficulty_slider.set_value_no_signal(new_value)
+ difficulty_label.text = "{" + str(new_value) + "}"
+
+
</code_context>
<issue_to_address>
**suggestion:** The write-back to `Globals.settings.difficulty` in the external observer looks unnecessary and slightly convoluted.
In `_on_external_setting_changed`, when `setting_name == "difficulty"`, this callback is already responding to a change in `Globals.settings`. Writing `Globals.settings.difficulty = float(new_value)` again is redundant and depends on the setter guard to avoid loops. Consider using this handler only to sync UI (`difficulty_slider` and `difficulty_label`) from `new_value`, and omit the write-back to the resource.
```suggestion
func _on_external_setting_changed(setting_name: String, new_value: Variant) -> void:
if setting_name != "difficulty":
return
# Sync UI from the updated resource value without emitting UI change signals
difficulty_slider.set_value_no_signal(float(new_value))
difficulty_label.text = "{" + str(new_value) + "}"
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…rnal observer looks unnecessary and slightly convoluted. In _on_external_setting_changed, when setting_name == "difficulty", this callback is already responding to a change in Globals.settings. Writing Globals.settings.difficulty = float(new_value) again is redundant and depends on the setter guard to avoid loops. Consider using this handler only to sync UI (difficulty_slider and difficulty_label) from new_value, and omit the write-back to the resource.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/advanced_settings.gd (1)
255-272:⚠️ Potential issue | 🟡 MinorAdvanced settings is still not observing the resource it now relies on.
After removing the direct save/log calls, this menu never subscribes to
Globals.settings.setting_changed. Ifcurrent_log_levelchanges anywhere else while the panel is open,log_lvl_optionstays stale, so this screen is not actually synced toGameSettingsResourceyet.Mirror the external-setting hookup used in
scripts/gameplay_settings.gdand drive programmatic selection through that observer path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/advanced_settings.gd` around lines 255 - 272, The log level selector never subscribes to Globals.settings.setting_changed, so UI stays out-of-sync; update the panel to connect to the same observer pattern used in gameplay_settings.gd (connect to Globals.settings.setting_changed in _ready or when the panel is shown), implement or reuse the handler used there to update log_lvl_option.selected programmatically (instead of setting selected directly in _on_log_level_item_selected), and ensure you disconnect the signal when the panel is hidden/freed to avoid leaks; reference _on_log_level_item_selected, Globals.settings.setting_changed, and log_lvl_option when making the changes.
🧹 Nitpick comments (1)
scripts/gameplay_settings.gd (1)
87-95: Keep_on_external_setting_changed()UI-only.
setting_changedis emitted after the resource has already stored the canonical difficulty. WritingGlobals.settings.difficultyagain here is redundant and keeps a second model-write path alive for the same event.Small cleanup
func _on_external_setting_changed(setting_name: String, new_value: Variant) -> void: if setting_name == "difficulty": - # Prevent Circular Updates in the UI - if difficulty_slider.value != float(new_value): - Globals.settings.difficulty = float(new_value) - # Update UI without triggering a circular signal loop - # Use set_value_no_signal to avoid re-triggering _on_difficulty_value_changed - difficulty_slider.set_value_no_signal(new_value) - difficulty_label.text = "{" + str(new_value) + "}" + var difficulty_value: float = float(new_value) + difficulty_slider.set_value_no_signal(difficulty_value) + difficulty_label.text = "{" + str(difficulty_value) + "}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/gameplay_settings.gd` around lines 87 - 95, The handler _on_external_setting_changed is performing a redundant model write (Globals.settings.difficulty = float(new_value)) — remove that assignment so the function only updates the UI; keep the circular-update guard and update calls on difficulty_slider and difficulty_label, and ensure you pass a numeric value into difficulty_slider.set_value_no_signal (e.g., float(new_value)) and format difficulty_label.text as before; reference symbols: _on_external_setting_changed, Globals.settings.difficulty, difficulty_slider.set_value_no_signal, difficulty_label.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/game_settings_resource.gd`:
- Around line 27-30: Update the globals save/load to persist the new
enable_debug_logging flag: modify scripts/globals.gd::_save_settings() to
include the setting (use the exported name "enable_debug_logging" or the backing
value _enable_debug_logging from GameSettings) when serializing settings, and
modify scripts/globals.gd:_load_settings() to read that key and set
GameSettings.enable_debug_logging (or the backing field) with a sensible default
if missing; ensure the load path uses the same key name so the emitted
setting_changed flow actually survives reloads.
- Around line 20-23: The setter for current_log_level currently assigns any
integer and immediately emits setting_changed; update the setter on
current_log_level to validate the incoming value (ensure it's an integer within
the allowed range 0..4) before writing to _current_log_level and before calling
setting_changed.emit; if the value is out-of-range, either clamp it to the
nearest valid value or reject it (e.g., return/warn) so invalid values are never
stored or broadcast from current_log_level, and reference the current_log_level
setter, the backing field _current_log_level, and the signal setting_changed
when making the change.
In `@scripts/gameplay_settings.gd`:
- Around line 261-263: Remove the duplicate debug log that reports difficulty
changes: delete the Globals.log_message(...) call that uses
Globals.settings.difficulty (the local log inside the difficulty setter/update
block) so changes are only logged centrally by the existing setting_changed
signal handler (Globals._on_setting_changed). Ensure the assignment
Globals.settings.difficulty = value remains but no extra logging is performed
locally.
---
Outside diff comments:
In `@scripts/advanced_settings.gd`:
- Around line 255-272: The log level selector never subscribes to
Globals.settings.setting_changed, so UI stays out-of-sync; update the panel to
connect to the same observer pattern used in gameplay_settings.gd (connect to
Globals.settings.setting_changed in _ready or when the panel is shown),
implement or reuse the handler used there to update log_lvl_option.selected
programmatically (instead of setting selected directly in
_on_log_level_item_selected), and ensure you disconnect the signal when the
panel is hidden/freed to avoid leaks; reference _on_log_level_item_selected,
Globals.settings.setting_changed, and log_lvl_option when making the changes.
---
Nitpick comments:
In `@scripts/gameplay_settings.gd`:
- Around line 87-95: The handler _on_external_setting_changed is performing a
redundant model write (Globals.settings.difficulty = float(new_value)) — remove
that assignment so the function only updates the UI; keep the circular-update
guard and update calls on difficulty_slider and difficulty_label, and ensure you
pass a numeric value into difficulty_slider.set_value_no_signal (e.g.,
float(new_value)) and format difficulty_label.text as before; reference symbols:
_on_external_setting_changed, Globals.settings.difficulty,
difficulty_slider.set_value_no_signal, difficulty_label.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cf0f3add-a560-4327-bd62-6ae38984e2a7
📒 Files selected for processing (3)
scripts/advanced_settings.gdscripts/game_settings_resource.gdscripts/gameplay_settings.gd
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Browser Functional Tests / test
The new Globals.settings.setting_changed connection in gameplay_settings.gd is never disconnected in _on_tree_exited, which can leave stale observers around if the menu is created/destroyed multiple times; consider explicitly disconnecting the signal in _on_tree_exited.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
To ensure that enable_debug_logging persists across reloads, you must update the serialization logic in globals.gd. Currently, your centralized save flow is triggered by the signal in GameSettingsResource, but the _save_settings() and _load_settings() functions in globals.gd only handle log_level and difficulty. While your refactored GameSettingsResource correctly emits the setting_changed signal, the Observer (Globals.gd) was only "notified" that something happened; it didn't know it needed to write that specific new field to the disk. By updating these two functions, you complete the persistence loop for the debug flag.
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- GameSettingsResource.enable_debug_logging always emits setting_changed even when the value is unchanged, unlike difficulty/current_log_level; consider adding an early return on no-op updates to avoid redundant logging and saves.
- The tests repeatedly wire custom lambdas to setting_changed to call Globals._save_settings with a test path; you might simplify this by extracting the save-on-change behavior into a small helper that takes a Config path so both production code and tests can reuse the same observer logic.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- GameSettingsResource.enable_debug_logging always emits setting_changed even when the value is unchanged, unlike difficulty/current_log_level; consider adding an early return on no-op updates to avoid redundant logging and saves.
- The tests repeatedly wire custom lambdas to setting_changed to call Globals._save_settings with a test path; you might simplify this by extracting the save-on-change behavior into a small helper that takes a Config path so both production code and tests can reuse the same observer logic.
## Individual Comments
### Comment 1
<location path="scripts/game_settings_resource.gd" line_range="30-35" />
<code_context>
+ get:
+ return _current_log_level
+
+@export var enable_debug_logging: bool = false:
+ set(value):
+ _enable_debug_logging = value
+ setting_changed.emit("enable_debug_logging", value)
+ get:
+ return _enable_debug_logging
@export_group("Gameplay")
</code_context>
<issue_to_address>
**suggestion:** Guard the debug logging setter to avoid redundant signal emissions
Here `enable_debug_logging` emits `setting_changed` even when the value hasn’t changed, unlike `current_log_level` and `difficulty`. This causes unnecessary log + save operations when the UI reassigns the same value. Consider normalizing the input (if needed) and returning early when `_enable_debug_logging == value` before emitting the signal.
```suggestion
@export var enable_debug_logging: bool = false:
set(value):
var new_value: bool = bool(value)
if _enable_debug_logging == new_value:
return
_enable_debug_logging = new_value
setting_changed.emit("enable_debug_logging", new_value)
get:
return _enable_debug_logging
```
</issue_to_address>
### Comment 2
<location path="scripts/globals.gd" line_range="59-67" />
<code_context>
+
+
+## Reactive handler for the Observer Pattern [cite: 141]
+func _on_setting_changed(setting_name: String, new_value: Variant) -> void:
+ # FIX: Ensure we are comparing String to String or using correct types
+ var log_msg: String = "Setting '%s' updated to: %s" % [setting_name, str(new_value)]
+
+ # Automatically log the change [cite: 59]
+ log_message(log_msg, LogLevel.DEBUG)
+
+ # Automatically persist to disk [cite: 53]
+ _save_settings()
</code_context>
<issue_to_address>
**suggestion (performance):** Observer handler may cause excessive disk writes and logging during bulk loads
Since `_on_setting_changed` logs and calls `_save_settings()` on every property update, bulk operations like `_load_settings()` will result in one disk write and log entry per field. To avoid this overhead, introduce a way to disable persistence/logging during bulk updates (e.g., a `is_loading_settings` flag or a batch apply API) and perform a single save once loading is complete, while preserving reactive behavior for normal runtime changes.
Suggested implementation:
```
var _is_loading_settings: bool = false
## Reactive handler for the Observer Pattern [cite: 141]
func _on_setting_changed(setting_name: String, new_value: Variant) -> void:
# Suppress reactive side effects when performing bulk load / batch updates
if _is_loading_settings:
return
# FIX: Ensure we are comparing String to String or using correct types
var log_msg: String = "Setting '%s' updated to: %s" % [setting_name, str(new_value)]
# Automatically log the change [cite: 59]
log_message(log_msg, LogLevel.DEBUG)
# Automatically persist to disk [cite: 53]
_save_settings()
```
To fully support bulk operations without excessive logging/disk writes, adjust the loading/batch APIs elsewhere in `scripts/globals.gd` as follows:
1. In your bulk loading function (likely named `_load_settings()` or similar), wrap the bulk updates:
```gdscript
func _load_settings() -> void:
_is_loading_settings = true
# ... existing logic that sets many settings / triggers setting_changed ...
_is_loading_settings = false
_save_settings() # optional: single save after all updates
```
2. If you have any other batch-apply APIs that update multiple settings, they should also set `_is_loading_settings = true` before the batch and reset it to `false` afterwards, optionally performing a single `_save_settings()` at the end.
This ensures that normal runtime changes still trigger logging and persistence, while bulk loads perform only a single save and avoid noisy logs.
</issue_to_address>
### Comment 3
<location path="scripts/advanced_settings.gd" line_range="264-272" />
<code_context>
var selected_enum: Globals.LogLevel = log_level_display_to_enum.get(
selected_name, Globals.LogLevel.INFO
)
+ # Only update the resource; the Observer handles the rest
Globals.settings.current_log_level = selected_enum
log_lvl_option.selected = Globals.LogLevel.values().find(selected_enum)
# Temporary raw print to bypass log_message
Globals.log_message("Log level changed to: " + selected_name, Globals.LogLevel.DEBUG)
- Globals._save_settings()
+ # Globals._save_settings()
</code_context>
<issue_to_address>
**suggestion:** Avoid duplicating logging behavior now handled by the settings observer
Since `current_log_level` changes are already logged and persisted via `_on_setting_changed`, this extra `Globals.log_message` will create duplicate entries for a single change. Please either remove this call or move the logging responsibility entirely into the observer so there’s a single source of truth for log side effects.
```suggestion
var selected_enum: Globals.LogLevel = log_level_display_to_enum.get(
selected_name, Globals.LogLevel.INFO
)
# Only update the resource; the Observer handles logging & persistence
Globals.settings.current_log_level = selected_enum
log_lvl_option.selected = Globals.LogLevel.values().find(selected_enum)
# Globals._save_settings()
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
…rites and logging during bulk loads Since _on_setting_changed logs and calls _save_settings() on every property update, bulk operations like _load_settings() will result in one disk write and log entry per field. To avoid this overhead, introduce a way to disable persistence/logging during bulk updates (e.g., a is_loading_settings flag or a batch apply API) and perform a single save once loading is complete, while preserving reactive behavior for normal runtime changes.
…ikostan/SkyLockAssault into migrate-to-resource-persistence
github-project-automation
Bot
moved this from In Progress
to Done
in Sky Lock Assault Project
8 tasks
This was referenced Apr 7, 2026
This was referenced Apr 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
name: Default Pull Request Template
about: Suggesting changes to SkyLockAssault
title: ''
labels: ''
assignees: ''
Description
What does this PR do? (e.g., "Fixes player jump physics in level 2" or "Adds
new enemy AI script")
Related Issue
Closes #ISSUE_NUMBER (if applicable)
Changes
system")
Testing
works on Win10 with 60 FPS")
Checklist
Additional Notes
Anything else? (e.g., "Tested on Win10 64-bit; needs Linux validation")
Summary by Sourcery
Adopt an observer-based settings system that centralizes reactions to configuration changes, enabling automatic logging, persistence, and UI synchronization.
New Features:
Enhancements:
Tests:
Summary by CodeRabbit
New Features
Improvements
Tests