[FEATURE] Migrate to Resource persistence

[ikostan]

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.

---

@ikostan, you're absolutely right, and I apologize for the confusion!

The test is correct for your current implementation. Since you're still using ConfigFile in _save_settings() and _load_settings(), the test validating ConfigFile format is appropriate.

I was pointing out a discrepancy between the PR description and the actual code:

• PR objectives state: "replace the current ConfigFile approach" and "enable saving/loading profiles as .tres files"
• Actual implementation: Still uses ConfigFile.save() and ConfigFile.load() (lines 170-185 and 120-135 in globals.gd)

Two paths forward:

1. Keep ConfigFile (what you have now): The test is fine as-is. Just update the PR description to clarify you're centralizing settings into a Resource object but keeping ConfigFile persistence.

2. Migrate to Resource persistence (stated goal): Replace ConfigFile with ResourceSaver.save(settings, path) and ResourceLoader.load(path) in _save_settings() and _load_settings(), then update this test accordingly.

Which direction would you prefer? If option 1, I'll mark my previous comments as addressed since the test matches the implementation.

Originally posted by @coderabbitai[bot] in #431 (comment)

[ikostan]

---

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Relevant files =="
fd 'globals\.gd$'
fd 'game_settings_resource\.gd$'

echo "== Save/load implementation and storage format =="
rg -n -C4 --type=gd '\bfunc\s+_save_settings\b|\bfunc\s+_load_settings\b|ConfigFile|ResourceSaver|ResourceLoader|set_value\(|get_value\('

echo "== Resource field definitions =="
rg -n -C2 --type=gd 'class_name\s+GameSettingsResource|@export.*current_log_level|@export.*difficulty'

Repository: ikostan/SkyLockAssault

Length of output: 262

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Globals implementation =="
cat -n scripts/globals.gd

echo -e "\n== Game Settings Resource =="
cat -n scripts/game_settings_resource.gd

echo -e "\n== Test file =="
cat -n test/gut/test_globals_resource.gd

Repository: ikostan/SkyLockAssault

Length of output: 18943

---

Update test to validate Resource-based persistence instead of ConfigFile keying.

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.

Suggested resource-based approach

 func test_logging_persistence() -> void:
 	gut.p("Testing: Persistence writes to config file.")
 	Globals.settings.current_log_level = 0 # DEBUG
 	Globals._save_settings(TEST_RESOURCE_PATH)
 	
 	assert_true(FileAccess.file_exists(TEST_RESOURCE_PATH), "Config file should exist")
-	
-	var config := ConfigFile.new()
-	var err := config.load(TEST_RESOURCE_PATH)
-	assert_eq(err, OK, "ConfigFile should load successfully")
-	assert_eq(config.get_value("Settings", "log_level"), 0, "Saved value check")
+
+	var loaded := ResourceLoader.load(TEST_RESOURCE_PATH) as GameSettingsResource
+	assert_not_null(loaded, "Saved settings resource should load")
+	assert_eq(loaded.current_log_level, 0, "Saved value check")

🤖 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 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.

✅ Confirmed as addressed by @ikostan

---

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== Current test_logging_persistence implementation ==="
cat -n test/gut/test_globals_resource.gd | sed -n '34,44p'

echo -e "\n=== Full test file for context ==="
cat -n test/gut/test_globals_resource.gd

echo -e "\n=== Current _save_settings implementation ==="
rg -n -A20 'func _save_settings' scripts/globals.gd

echo -e "\n=== Current _load_settings implementation ==="
rg -n -A20 'func _load_settings' scripts/globals.gd

Length of output: 6604

---

@ikostan, I've reviewed the current code and I don't see any changes to the test_logging_persistence() function (lines 35-47). The test still validates ConfigFile format:

Current test (unchanged):

• Line 42-44: Creates ConfigFile, calls config.load(), checks OK status
• Line 47: Validates with config.get_value("Settings", "log_level")

Current implementation (also unchanged):

• _save_settings() (line 160): Uses ConfigFile.save() with manual field extraction
• _load_settings() (line 120): Uses ConfigFile.load() with manual field assignment

The Core Issue:
Your PR objectives state the goal is to "replace the current ConfigFile approach" and "enable saving/loading profiles as .tres files," but both the implementation and test still use ConfigFile. For true Resource-based persistence, you would need:

1. Update implementation:

   func _save_settings(path: String = Settings.CONFIG_PATH) -> void:
       var err := ResourceSaver.save(settings, path)
       if err != OK:
           log_message("Failed to save settings: " + str(err), LogLevel.ERROR)
   
   func _load_settings(path: String = Settings.CONFIG_PATH) -> void:
       if FileAccess.file_exists(path):
           settings = ResourceLoader.load(path) as GameSettingsResource
           if settings == null:
               settings = preload("res://settings/default_settings.tres")

2. Update test to match:

   var loaded := ResourceLoader.load(TEST_RESOURCE_PATH) as GameSettingsResource
   assert_not_null(loaded, "Saved settings resource should load")
   assert_eq(loaded.current_log_level, 0, "Saved value check")

Question: Did you intend to change a different test, or were you planning to migrate the implementation first? I see you improved test_logging_default_level() (line 29), but test_logging_persistence() appears untouched.

[ikostan]

The core problem is Leaky Abstractions: UI scripts (advanced_settings.gd and gameplay_settings.gd) are directly responsible for saving data and triggering global logging. This makes the UI code harder to test and creates a "tight coupling" where the UI has to know exactly how and when to persist data.

Current State Analysis

In the current code, every time a UI element changes, the script manually handles the persistence:

• advanced_settings.gd: Calls Globals.settings.current_log_level = selected_enum and then immediately calls Globals._save_settings().
• gameplay_settings.gd: Calls Globals.settings.difficulty = value and then immediately calls Globals._save_settings().

Recommended Refactor: The Observer Pattern

To fix this, we should move the "Save" and "Log" logic out of the UI and into the GameSettingsResource and Globals.

1. Enhance GameSettingsResource

Update your resource to emit a signal whenever a property is changed. This turns the resource into the "Subject" that others can observe.

# game_settings_resource.gd
extends Resource
class_name GameSettingsResource

signal setting_changed(setting_name: String, new_value: Variant)

@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 difficulty: float = 1.0:
	set(value):
		_difficulty = clamp(value, 0.5, 2.0)
		setting_changed.emit("difficulty", _difficulty)
	get:
		return _difficulty

2. Centralize Logic in Globals.gd

In Globals.gd, connect to that signal during _ready. Now, Globals is the only script that needs to know about _save_settings().

# globals.gd
func _ready() -> void:
    # ... existing load logic ...
    settings.setting_changed.connect(_on_setting_changed)

func _on_setting_changed(setting_name: String, new_value: Variant) -> void:
    log_message("Setting '%s' changed to %s" % [setting_name, str(new_value)], LogLevel.DEBUG)
    _save_settings()

3. Simplify UI Scripts

Now your UI scripts only care about the data. You can remove the calls to Globals._save_settings() and Globals.log_message() entirely.

• **New _on_difficulty_value_changed**:

func _on_difficulty_value_changed(value: float) -> void:
    Globals.settings.difficulty = value 
    # No more manual save or log calls here!

Summary of Benefits

1. Decoupling: The UI doesn't care if the game saves to a file, a database, or the cloud. It just updates the resource.
2. Maintainability: If you decide to add a "Apply" button later (to avoid saving on every slider tick), you only change the logic in one place (Globals), not every UI script.
3. Clean Logs: Centralizing the logs in Globals ensures consistent formatting across the whole project.