[FEATURE] Convert Hard-Coded Globals to Godot Resource #285 - #431
Merged
Conversation
Issue #285, "[FEATURE] Convert Hard-Coded Globals to Godot Resource," focuses on migrating static configuration and game settings from the current globals.gd singleton into a more modular Godot Resource system.
Contributor
Reviewer's GuideMigrates global difficulty, logging settings, UI prompts, and scene paths from hard-coded globals in globals.gd into a new GameSettingsResource Godot Resource, wiring all existing usages through the resource and adding tests to validate behavior and persistence. Sequence diagram for loading and saving settings via GameSettingsResourcesequenceDiagram
actor Player
participant GameplaySettings
participant AdvancedSettings
participant Globals
participant GameSettingsResource as Settings
participant ConfigFile
Player->>GameplaySettings: Open gameplay settings
GameplaySettings->>Globals: access settings.difficulty
Globals->>Settings: get difficulty
Settings-->>Globals: difficulty
Globals-->>GameplaySettings: difficulty value
GameplaySettings->>GameplaySettings: update slider and label
Player->>GameplaySettings: Change difficulty slider
GameplaySettings->>Globals: settings.difficulty = value
Globals->>Settings: set difficulty(value)
Settings-->>Globals: clamped difficulty
GameplaySettings->>Globals: _save_settings()
Globals->>ConfigFile: set_value(log_level, settings.current_log_level)
Globals->>ConfigFile: set_value(difficulty, settings.difficulty)
ConfigFile-->>Globals: save(path)
Player->>AdvancedSettings: Open advanced settings
AdvancedSettings->>Globals: access settings.current_log_level
Globals->>Settings: get current_log_level
Settings-->>Globals: current_log_level
Globals-->>AdvancedSettings: enum value
AdvancedSettings->>AdvancedSettings: select matching option
Player->>AdvancedSettings: Change log level
AdvancedSettings->>Globals: settings.current_log_level = selected_enum
Globals->>Settings: update current_log_level
AdvancedSettings->>Globals: _save_settings()
Globals->>ConfigFile: set_value(log_level, settings.current_log_level)
ConfigFile-->>Globals: save(path)
Note over Globals,Settings: On startup, Globals._load_settings() reads ConfigFile
Globals->>ConfigFile: load(path)
ConfigFile-->>Globals: get_value(log_level)
Globals->>Settings: apply current_log_level
ConfigFile-->>Globals: get_value(difficulty)
Globals->>Settings: apply difficulty (with clamping)
Class diagram for Globals singleton and GameSettingsResource integrationclassDiagram
class Globals {
+CanvasLayer options_instance
+Array~Node~ hidden_menus
+bool options_open
+String previous_scene
+String next_scene
+String current_input_device
+GameSettingsResource settings
+void _ready()
+void load_key_mapping(menu_to_hide: Node)
+void load_options(menu_to_hide: Node)
+void _load_settings(path: String)
+void _save_settings(path: String)
+void log_message(message: String, level: LogLevel)
}
class GameSettingsResource {
<<Resource>>
+int current_log_level
+bool enable_debug_logging
+float difficulty
+String remap_prompt_keyboard
+String remap_prompt_gamepad
+PackedScene key_mapping_scene
+PackedScene options_scene
}
class GameplaySettings {
+Slider difficulty_slider
+Label difficulty_label
+void _ready()
+void _on_difficulty_value_changed(value: float)
}
class AdvancedSettings {
+OptionButton log_lvl_option
+void _ready()
+void _on_advanced_reset_button_pressed()
+void _on_log_level_item_selected(index: int)
}
class Bullet {
+float fire_rate
+Timer timer
+void fire()
}
class MainScene {
+Node2D background
+Node2D player
+void _process(delta: float)
}
class Player {
+Dictionary speed
+Dictionary fuel
+void _on_fuel_timer_timeout()
}
Globals *-- GameSettingsResource : settings
GameplaySettings ..> Globals : uses
AdvancedSettings ..> Globals : uses
Bullet ..> Globals : uses
MainScene ..> Globals : uses
Player ..> Globals : uses
GameplaySettings ..> GameSettingsResource : via Globals.settings
AdvancedSettings ..> GameSettingsResource : via Globals.settings
Bullet ..> GameSettingsResource : via Globals.settings
MainScene ..> GameSettingsResource : via Globals.settings
Player ..> GameSettingsResource : via Globals.settings
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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:
📝 WalkthroughWalkthroughCentralizes previously top-level Globals into a new exported Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Advanced UI / Gameplay UI
participant Globals as Globals (singleton)
participant Settings as GameSettingsResource
participant FS as ConfigFile / Resource I/O
participant Game as Game Components (Player/Bullet/Main)
UI->>Globals: read/write settings.* (e.g., current_log_level, difficulty, remap prompts)
Globals->>Settings: proxy access to fields
UI->>Globals: trigger _save_settings()
Globals->>FS: write Resource to disk (default_settings.tres / config path)
FS-->>Globals: write result
Game->>Globals: read settings.difficulty during runtime ticks
Globals->>Settings: return difficulty
Settings-->>Game: value used to scale behavior
FS->>Globals: on load -> provide Resource
Globals->>Settings: replace/merge loaded resource (fallback to defaults if corrupt)
Globals-->>UI: updated values reflected in UI controls
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| JavaScript |  |
Mar 1, 2026 5:38a.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 2 issues, and left some high level feedback:
- The
difficultyproperty setter inGameSettingsResourceassigns todifficultyinside its own setter, which will recurse indefinitely—use a backing field (e.g.,_difficulty) orsetgeton a separate variable to apply clamping safely. - You now clamp
difficultyboth in theGameSettingsResourcesetter and again inGlobals._load_settings; consider consolidating the clamping logic in one place (ideally the Resource) to avoid redundancy and potential divergence if the valid range changes. - The new
GameSettingsResourceis currently used as an in-memory holder while_load_settings/_save_settingsstill persist viaConfigFile; if the goal is Resource-backed profiles, you may want to add explicit load/save paths for.tresassets rather than relying solely on the legacy config format.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `difficulty` property setter in `GameSettingsResource` assigns to `difficulty` inside its own setter, which will recurse indefinitely—use a backing field (e.g., `_difficulty`) or `setget` on a separate variable to apply clamping safely.
- You now clamp `difficulty` both in the `GameSettingsResource` setter and again in `Globals._load_settings`; consider consolidating the clamping logic in one place (ideally the Resource) to avoid redundancy and potential divergence if the valid range changes.
- The new `GameSettingsResource` is currently used as an in-memory holder while `_load_settings`/`_save_settings` still persist via `ConfigFile`; if the goal is Resource-backed profiles, you may want to add explicit load/save paths for `.tres` assets rather than relying solely on the legacy config format.
## Individual Comments
### Comment 1
<location path="scripts/game_settings_resource.gd" line_range="14-16" />
<code_context>
+# Multiplier: 1.0=Normal, <1=Easy, >1=Hard
+# In globals.gd, change the difficulty variable in the Resource script:
+# game_settings_resource.gd
+@export var difficulty: float = 1.0:
+ set(value):
+ difficulty = clamp(value, 0.5, 2.0) # Use a setter to force clamping ALWAYS
+
+@export_group("UI & Scenes")
</code_context>
<issue_to_address>
**issue (bug_risk):** The difficulty setter is recursively assigning to itself and will cause infinite recursion.
In the setter you’re assigning to `difficulty` itself, which just re-invokes the same setter and never reaches a base assignment. Use a separate backing field or clamp only at the call site instead. For example:
```gdscript
var _difficulty_internal: float = 1.0
@export var difficulty: float:
get:
return _difficulty_internal
set(value):
_difficulty_internal = clamp(value, 0.5, 2.0)
```
Alternatively, keep `difficulty` as a plain `@export` and apply `clamp` wherever you set it (e.g. in `_load_settings`).
</issue_to_address>
### Comment 2
<location path="scripts/globals.gd" line_range="137" />
<code_context>
var loaded_difficulty: Variant = config.get_value("Settings", "difficulty")
if (loaded_difficulty is float) or (loaded_difficulty is int):
- difficulty = loaded_difficulty
+ settings.difficulty = loaded_difficulty
# Validate and clamp difficulty to slider range (0.5-2.0)
- if difficulty < 0.5 or difficulty > 2.0:
</code_context>
<issue_to_address>
**suggestion:** Difficulty is being clamped both here and in the Resource setter, which is redundant once the setter is fixed.
Once the recursion in `GameSettingsResource.difficulty` is fixed, assignments to `settings.difficulty` will already clamp the value. The range check and clamp in `_load_settings` can then be removed in favor of a direct assignment plus logging, avoiding duplicate logic and keeping the constraints defined in a single place.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![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: 8
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)
246-251:⚠️ Potential issue | 🟠 Major
get_log_level_index()still reads the legacy global field.Line 251 uses
Globals.current_log_levelwhile the rest of this file now writes toGlobals.settings.current_log_level(Line 165, Line 267). This can desync index calculations.✅ Direct fix
func get_log_level_index() -> int: ## Retrieves the index of the current log level in the enum values. ## ## :returns: The index of the current log level. ## :rtype: int - return Globals.LogLevel.values().find(Globals.current_log_level) + return Globals.LogLevel.values().find(Globals.settings.current_log_level)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/advanced_settings.gd` around lines 246 - 251, get_log_level_index() is still reading the legacy Globals.current_log_level which can desync with the rest of the file that uses Globals.settings.current_log_level; update the function to read from Globals.settings.current_log_level (replace any reference to Globals.current_log_level) so the index lookup uses the current settings object and remains consistent with code paths that write to Globals.settings.current_log_level.
🧹 Nitpick comments (2)
scripts/gameplay_settings.gd (1)
241-243: Prefer rendering the label from persisted/normalized difficulty.Line 243 uses raw
value; if normalization happens before/at Line 242, label text can diverge from the stored value.🔧 Small consistency tweak
func _on_difficulty_value_changed(value: float) -> void: Globals.settings.difficulty = value difficulty_slider.value = Globals.settings.difficulty - difficulty_label.text = "{" + str(value) + "}" + difficulty_label.text = "{" + str(Globals.settings.difficulty) + "}" Globals.log_message("Difficulty changed to: " + str(value), Globals.LogLevel.DEBUG) Globals._save_settings()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/gameplay_settings.gd` around lines 241 - 243, The label is rendered from the raw local variable value which can diverge after normalization; change the label construction to use the persisted/normalized value (e.g., Globals.settings.difficulty or difficulty_slider.value) instead of the raw value variable so difficulty_label.text reflects the actual stored/normalized setting; update the code that sets difficulty_label.text to reference Globals.settings.difficulty (or difficulty_slider.value) rather than value.test/gut/test_globals_resource.gd (1)
10-20: Test setup mutates singleton state without restoring it.
before_each()modifiesGlobals.settings.current_log_level, butafter_each()only deletes the temp file. This can leak state across tests/suites.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/gut/test_globals_resource.gd` around lines 10 - 20, The before_each() test setup mutates the singleton Globals by setting Globals.settings.current_log_level to Globals.LogLevel.NONE and never restores it; capture the original value of Globals.settings.current_log_level at the start of before_each() (e.g., save to a local or file‑scoped variable) and then restore that saved value in after_each() so the singleton state is returned to its prior value; ensure the fix references the existing functions before_each, after_each, Globals.settings.current_log_level, and Globals.LogLevel.NONE and that TEST_RESOURCE_PATH cleanup remains in after_each().
🤖 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 1-22: Run the formatter (gdformat -i) to fix CI formatting errors,
and also correct the invalid/dangerous difficulty property implementation in
GameSettingsResource: remove the stray colon after the default value and replace
the current setter that reassigns difficulty to itself with a proper setter
using a backing field (e.g., _difficulty) or Godot's setget pattern so you clamp
the incoming value with clamp(value, 0.5, 2.0) and assign to the backing
variable instead of recursively reassigning difficulty.
- Line 7: The exported variable current_log_level is unconstrained and can be
set outside the valid LogLevel index range (0–4); update the GameSettings
resource to enforce bounds by making current_log_level an exported ranged
property or adding a setter that clamps values to 0..4 (so any assignment via
the inspector or code always stays within 0–4), ensuring all places that index
LogLevel.keys() (e.g., globals.gd usage) cannot crash from out-of-range indices.
In `@scripts/globals.gd`:
- Around line 39-43: The CI shows formatter drift in the _ready() function; run
the Godot formatter (gdformat -i) to reformat this file, ensuring the function
signature, indentation, colon placement, and spacing around calls like
_load_settings(), log_message(), and references to settings.current_log_level /
LogLevel.keys() follow the project style; after formatting, commit the updated
file so the pipeline passes.
- Around line 137-149: The current range-warning is unreachable because you
assign through the clamping setter before checking; instead validate and log
using the raw loaded_difficulty variable prior to writing to
settings.difficulty: check if loaded_difficulty is outside 0.5–2.0, call
log_message with LogLevel.WARNING including the raw value, then assign
settings.difficulty = clamp(loaded_difficulty, 0.5, 2.0) (or assign directly if
within range) and finally log the loaded difficulty with LogLevel.DEBUG; update
references to settings.difficulty, loaded_difficulty, clamp, log_message, and
LogLevel.WARNING accordingly.
- Around line 105-106: Guard against settings.key_mapping_scene being null
before calling instantiate(): check settings.key_mapping_scene and if nil,
restore the hidden menu state (same as load_options_menu() does) and return
early; otherwise call settings.key_mapping_scene.instantiate(), assign to
km_instance, and add it to the scene tree with
get_tree().root.add_child(km_instance). Ensure the null-check logic mirrors the
defensive pattern in load_options_menu() so the hidden menu state is restored on
the early exit.
In `@test/gut/test_globals_resource.gd`:
- Around line 48-56: The test mutates Globals.settings.difficulty in-memory but
then calls Globals._load_settings(TEST_RESOURCE_PATH) without writing the
invalid value to disk, so it doesn't exercise load-time clamping; update
test_difficulty_clamping to persist an invalid difficulty to the test resource
(using the same TEST_RESOURCE_PATH) before calling
Globals._load_settings(TEST_RESOURCE_PATH), then call _load_settings and assert
that Globals.settings.difficulty is clamped to 0.5–2.0; reference
Globals.settings, Globals._load_settings, TEST_RESOURCE_PATH and the
test_difficulty_clamping function when making the change.
- Around line 24-31: The test test_logging_default_level is tautological because
it sets Globals.settings.current_log_level = 1 and then asserts it is 1; remove
the forced assignment and instead reset or reload the Globals.settings Resource
to its default state (e.g., reinstantiate or call the existing
reset/load-defaults method) so the test exercises default-loading behavior, then
assert Globals.settings.current_log_level == 1; reference
Globals.settings.current_log_level and test_logging_default_level when making
the change.
- Around line 34-45: Update the test to validate Resource-based persistence
(GameSettingsResource/.tres) instead of ConfigFile keying: change the
test_logging_persistence to invoke the same persistence entry points
(Globals._save_settings / Globals._load_settings) after migrating those
implementations to use ResourceSaver/ResourceLoader and GameSettingsResource,
then assert the saved file exists at TEST_RESOURCE_PATH, load it via
ResourceLoader (or Resource.new from file) and verify the loaded
GameSettingsResource.log_level (or equivalent field) equals the value set on
Globals.settings; also ensure Globals._save_settings and Globals._load_settings
are updated to write/read a GameSettingsResource using ResourceSaver.save and
ResourceLoader.load so the test exercises the Resource format rather than
ConfigFile.
---
Outside diff comments:
In `@scripts/advanced_settings.gd`:
- Around line 246-251: get_log_level_index() is still reading the legacy
Globals.current_log_level which can desync with the rest of the file that uses
Globals.settings.current_log_level; update the function to read from
Globals.settings.current_log_level (replace any reference to
Globals.current_log_level) so the index lookup uses the current settings object
and remains consistent with code paths that write to
Globals.settings.current_log_level.
---
Nitpick comments:
In `@scripts/gameplay_settings.gd`:
- Around line 241-243: The label is rendered from the raw local variable value
which can diverge after normalization; change the label construction to use the
persisted/normalized value (e.g., Globals.settings.difficulty or
difficulty_slider.value) instead of the raw value variable so
difficulty_label.text reflects the actual stored/normalized setting; update the
code that sets difficulty_label.text to reference Globals.settings.difficulty
(or difficulty_slider.value) rather than value.
In `@test/gut/test_globals_resource.gd`:
- Around line 10-20: The before_each() test setup mutates the singleton Globals
by setting Globals.settings.current_log_level to Globals.LogLevel.NONE and never
restores it; capture the original value of Globals.settings.current_log_level at
the start of before_each() (e.g., save to a local or file‑scoped variable) and
then restore that saved value in after_each() so the singleton state is returned
to its prior value; ensure the fix references the existing functions
before_each, after_each, Globals.settings.current_log_level, and
Globals.LogLevel.NONE and that TEST_RESOURCE_PATH cleanup remains in
after_each().
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
scripts/advanced_settings.gdscripts/bullet.gdscripts/game_settings_resource.gdscripts/game_settings_resource.gd.uidscripts/gameplay_settings.gdscripts/globals.gdscripts/main_scene.gdscripts/player.gdsettings/default_settings.trestest/gut/test_globals_resource.gdtest/gut/test_globals_resource.gd.uid
📜 Review details
🧰 Additional context used
🪛 GitHub Actions: Pull Request Pipeline
scripts/game_settings_resource.gd
[error] 1-1: gdformat formatting check failed. 2 files would be reformatted. Run 'gdformat -i' to fix formatting.
scripts/globals.gd
[error] 1-1: gdformat formatting check failed. 2 files would be reformatted. Run 'gdformat -i' to fix formatting.
🔇 Additional comments (9)
test/gut/test_globals_resource.gd.uid (1)
1-1: UID companion file looks correct.Line 1 is valid as a standalone Godot UID mapping for the new test resource.
scripts/main_scene.gd (1)
189-189: Difficulty source migration is consistent.Line 189 correctly switches to
Globals.settings.difficultyand matches the new centralized settings model.scripts/game_settings_resource.gd.uid (1)
1-1: UID file is valid.Line 1 is a proper UID entry for the new resource script.
scripts/bullet.gd (1)
45-45: Good migration to resource-backed difficulty.Line 45 correctly reads from
Globals.settings.difficulty.settings/default_settings.tres (1)
1-9: Default settings resource wiring looks good.The resource header, script linkage, and ext-resource declarations are consistent with the new
GameSettingsResourcesetup.scripts/player.gd (1)
362-362: Fuel drain update is consistent with the new settings source.Line 362 correctly pulls difficulty from
Globals.settings.scripts/gameplay_settings.gd (1)
32-33:_ready()difficulty initialization migration looks correct.Using
Globals.settings.difficultyhere keeps the UI synced with the new resource-backed source of truth.scripts/advanced_settings.gd (1)
35-41: Resource-backed log-level reads/writes are applied correctly in the main flow.Good migration to
Globals.settings.current_log_levelfor initialization, reset, and selection-save paths.Also applies to: 162-168, 263-271
test/gut/test_globals_resource.gd (1)
59-70: Good defensive smoke coverage on resource content and corrupted-file fallback.The PackedScene checks and non-null fallback assertion are useful baseline guards for this migration.
Also applies to: 73-83
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
scripts/globals.gd (1)
14-22: Consider removing commented-out code after migration stabilizes.The commented exports (lines 14-19, 30, 32) serve as reference during migration but should be cleaned up once the resource-based approach is validated. The new
settingsexport with preload is correctly implemented.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/globals.gd` around lines 14 - 22, Remove the leftover commented-out legacy configuration (the REMAP_PROMPT_* constants and the `@export` vars current_log_level, enable_debug_logging, difficulty) now that settings is implemented; update scripts/globals.gd by deleting those commented lines so only the new `@export` var settings: GameSettingsResource = preload("res://settings/default_settings.tres") remains and rely on GameSettingsResource fields (accessed via the settings variable) for configuration.scripts/game_settings_resource.gd (1)
11-13: Remove stray developer notes.Lines 12-13 appear to be leftover instructions/notes rather than documentation for the
difficultyproperty. Only line 11 is relevant documentation.🧹 Proposed cleanup
`@export_group`("Gameplay") # Multiplier: 1.0=Normal, <1=Easy, >1=Hard -# In globals.gd, change the difficulty variable in the Resource script: -# game_settings_resource.gd `@export` var difficulty: float = 1.0:🤖 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 11 - 13, Remove the stray developer notes in game_settings_resource.gd that follow the real documentation comment for the difficulty property: keep only the relevant doc comment ("Multiplier: 1.0=Normal, <1=Easy, >1=Hard") associated with the difficulty Resource and delete the leftover instruction lines referencing globals.gd; ensure the remaining comment immediately precedes the difficulty property so the Resource script contains only the intended documentation.
🤖 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 2-3: GDLint fails because class_name appears after extends; move
the class_name declaration "class_name GameSettingsResource" to be the first
statement in the file and then keep "extends Resource" after it so that the file
reads class_name first and extends second, ensuring the symbols class_name and
extends are in the correct order for the linter.
---
Nitpick comments:
In `@scripts/game_settings_resource.gd`:
- Around line 11-13: Remove the stray developer notes in
game_settings_resource.gd that follow the real documentation comment for the
difficulty property: keep only the relevant doc comment ("Multiplier:
1.0=Normal, <1=Easy, >1=Hard") associated with the difficulty Resource and
delete the leftover instruction lines referencing globals.gd; ensure the
remaining comment immediately precedes the difficulty property so the Resource
script contains only the intended documentation.
In `@scripts/globals.gd`:
- Around line 14-22: Remove the leftover commented-out legacy configuration (the
REMAP_PROMPT_* constants and the `@export` vars current_log_level,
enable_debug_logging, difficulty) now that settings is implemented; update
scripts/globals.gd by deleting those commented lines so only the new `@export` var
settings: GameSettingsResource = preload("res://settings/default_settings.tres")
remains and rely on GameSettingsResource fields (accessed via the settings
variable) for configuration.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scripts/game_settings_resource.gdscripts/globals.gd
📜 Review details
🧰 Additional context used
🪛 GitHub Actions: Pull Request Pipeline
scripts/game_settings_resource.gd
[error] 3-3: GDLint: Definition out of order in global scope (class-definitions-order).
🔇 Additional comments (10)
scripts/game_settings_resource.gd (3)
7-7: Constraincurrent_log_levelto valid enum range (0–4).This was flagged in a previous review and remains unaddressed. The unconstrained
intexport can cause crashes when used to indexLogLevel.keys()in globals.gd if the value falls outside 0–4.🔧 Suggested fix
-@export var current_log_level: int = 1 +@export_range(0, 4, 1) var current_log_level: int = 1
14-16: LGTM!The difficulty setter correctly clamps incoming values to the valid range [0.5, 2.0]. The GDScript 4.x setter pattern is properly implemented.
18-22: LGTM!The UI & Scenes group correctly exports string prompts and preloads the required PackedScene assets. Preloading ensures these critical UI scenes are validated at load time.
scripts/globals.gd (7)
39-43: LGTM!The
_readyfunction correctly integrates with the settings resource, conditionally enabling debug logging based on editor context or theenable_debug_loggingflag.
104-106: Guardkey_mapping_scenebefore calling.instantiate().This was flagged in a previous review and remains unaddressed. Line 105 can crash if
settings.key_mapping_sceneis null. Add a null check consistent with the defensive pattern used inload_options().🛡️ Suggested fix
# FIX: We must call .instantiate() on the PackedScene inside settings + if settings.key_mapping_scene == null: + log_message("Error: Key mapping scene not configured.", LogLevel.ERROR) + if not hidden_menus.is_empty(): + var prev_menu: Node = hidden_menus.pop_back() + if is_instance_valid(prev_menu): + prev_menu.visible = true + return var km_instance: CanvasLayer = settings.key_mapping_scene.instantiate()
138-150: Range-warning logic is unreachable after setter clamping.This was flagged in a previous review and remains unaddressed. Line 138 assigns through the clamping setter, so the out-of-range check at lines 140-149 can never trigger. Validate against the raw
loaded_difficultyvalue before assignment to log meaningful warnings.🔧 Suggested fix
if config.has_section_key("Settings", "difficulty"): var loaded_difficulty: Variant = config.get_value("Settings", "difficulty") if (loaded_difficulty is float) or (loaded_difficulty is int): - settings.difficulty = loaded_difficulty - # Validate and clamp difficulty to slider range (0.5-2.0) - if settings.difficulty < 0.5 or settings.difficulty > 2.0: + var raw_difficulty: float = float(loaded_difficulty) + # Validate and warn if out of range before clamping + if raw_difficulty < 0.5 or raw_difficulty > 2.0: log_message( ( "Invalid difficulty loaded (" - + str(settings.difficulty) + + str(raw_difficulty) + ") - clamping to valid range." ), LogLevel.WARNING ) - settings.difficulty = clamp(settings.difficulty, 0.5, 2.0) + settings.difficulty = raw_difficulty # Setter will clamp log_message("Loaded saved difficulty: " + str(settings.difficulty), LogLevel.DEBUG)
172-173: LGTM!The save logic correctly persists the resource-backed settings values to the ConfigFile.
223-227: LGTM!The
load_optionsfunction correctly guards against nulloptions_sceneand handles instantiation failures with proper menu state restoration. This defensive pattern should also be applied toload_key_mapping.
253-254: LGTM!The log level filtering correctly references the resource-backed
settings.current_log_level.
117-128: LGTM!The log level loading correctly validates the type and range before assignment to the settings resource.
Line 7 exports an unconstrained int. The code directly indexes LogLevel.keys() with this value in globals.gd (lines 42, 126), which will crash if the value falls outside 0–4. Enforce bounds in the property itself.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…//github.com/ikostan/SkyLockAssault into convert-hard-coded-globals-to-godot-resource
ikostan
modified the milestones:
Milestone 12: Input & Navigation Improvements,
Milestone 13: TBD
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/game_settings_resource.gd (1)
20-21: Remove migration-only inline notes from the resource file.These lines read like temporary implementation notes and can be dropped to keep this file focused on current behavior.
🤖 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 - 21, The resource file contains migration-only inline notes around the difficulty variable in the Resource script; remove those temporary comment lines so the file only contains current behavior and definitions (e.g., the difficulty variable declaration in game_settings_resource.gd), leaving the Resource class and exported properties intact and free of migration comments.
🤖 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/game_settings_resource.gd`:
- Around line 20-21: The resource file contains migration-only inline notes
around the difficulty variable in the Resource script; remove those temporary
comment lines so the file only contains current behavior and definitions (e.g.,
the difficulty variable declaration in game_settings_resource.gd), leaving the
Resource class and exported properties intact and free of migration comments.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.deepsource.tomlscripts/game_settings_resource.gdtest/gdunit4/test_globals.gdtest/gdunit4/test_player.gdtest/gdunit4/test_settings_persistence.gd
🚧 Files skipped from review as they are similar to previous changes (2)
- test/gdunit4/test_settings_persistence.gd
- test/gdunit4/test_player.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)
.deepsource.toml (1)
8-9: Looks good: Python runtime alias update is clear and maintainable.Using
runtime_version = "3"with the inline rationale improves portability across CI/analyzer environments while keeping intent explicit.test/gdunit4/test_globals.gd (2)
36-37: Resource-backed difficulty write is correctly wired.This update aligns with the new
Globals.settings.*model while keeping persistence orchestration onglobals.
56-57: Load/assert path now matches the new API split.Calling
_load_settingsonglobalsand asserting throughglobals.settings.difficultyis correct.scripts/game_settings_resource.gd (3)
10-16: Class/resource declaration and logging fields look good.The declaration order and grouped logging exports are clean and consistent.
22-27: Difficulty clamping implementation is solid.Using a backing field with clamped setter is the right fix and keeps the value bounded.
Also applies to: 34-34
28-32: UI and scene configuration centralization is well implemented.These exports cleanly support the move away from hard-coded globals.
Line 137 already routes through the clamping setter, so the Line 139 out-of-range branch won’t trigger. Validate/log against the raw loaded value before assignment.
It sets Line 28 to 1 and then asserts Line 31 is 1, so it does not validate default-loading behavior.
The implementation still uses ConfigFile for _save_settings() and _load_settings() (lines 163-178 in globals.gd), not ResourceSaver/ResourceLoader. The test correctly validates this ConfigFile approach, but given that GameSettingsResource exists as a proper Resource class, the storage mechanism should be migrated from ConfigFile to Resource-based persistence (.tres format). Until that migration is complete, this test validates the legacy format and will miss regressions in Resource field persistence.
github-project-automation
Bot
moved this from In Progress
to Done
in Sky Lock Assault Project
8 tasks
This was referenced Mar 13, 2026
8 tasks
8 tasks
This was referenced Apr 14, 2026
8 tasks
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.
Convert Hard-Coded Globals to Godot Resource," focuses on migrating static configuration and game settings from the current
globals.gdsingleton into a more modular Godot Resource system.Key Areas for Resource Migration
difficultyandcurrent_log_levelare managed as exported variables within the singleton. Moving these to a dedicatedGameSettingsResource would allow you to save and load different configuration profiles as.tresfiles, making the system more flexible than the currentConfigFileimplementation.REMAP_PROMPT_KEYBOARDand scene paths (e.g.,res://scenes/key_mapping_menu.tscn) are ideal candidates for aUIConfigResource. This centralizes assets and text, allowing for easier updates without modifying the core logic inglobals.gd.Benefits of the Proposed Change
Implementation Recommendations
globals.gdshould likely remain a singleton to act as the central accessor and manager for these Resources (e.g., handling the_save_settingsand_load_settingslogic using the Resource format).get_game_version()are already a good practice; these can be extended to verify Resource integrity during loading.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
Migrate global gameplay and logging configuration from hard-coded singleton fields to a Godot Resource–backed settings object and update consumers to use it.
New Features:
Enhancements:
Tests:
Summary by CodeRabbit
Refactor
Behavior
New Features
Tests
Chores