Skip to content

Conversation

@LifeHckr
Copy link
Contributor

@LifeHckr LifeHckr commented Jun 21, 2025

Summary by CodeRabbit

  • New Features

    • Introduced a new persistent save/load system, allowing for more robust and extensible game state management.
    • Added persistent tracking for tutorial completion and victory status, enabling these states to be retained across sessions.
  • Refactor

    • Replaced the previous save system with separate configuration and save managers for improved maintainability.
    • Unified and streamlined configuration management across menus and gameplay.
    • Improved event subscription management to prevent potential memory leaks.
  • Bug Fixes

    • Corrected file path capitalization errors for texture assets, ensuring proper resource loading.
  • Style

    • Simplified and clarified lambda expressions by marking unused parameters, enhancing code readability.
  • Chores

    • Updated project configuration to autoload the new save system.

LifeHckr added 3 commits June 19, 2025 22:10
Moved HasWon and FirstTime(renamed TutorialDone) to StageProducer
Removed some commented code
Added _ to specify ignored parameters
@coderabbitai
Copy link

coderabbitai bot commented Jun 21, 2025

Walkthrough

This update introduces a new persistent save system by adding the Savekeeper class and refactoring save/load logic throughout the codebase. The previous SaveSystem is split: configuration management moves to Configkeeper, while game state persistence is handled by Savekeeper. Numerous files are updated to use these new systems, and several lambda expressions are simplified for clarity.

Changes

File(s) Change Summary
Globals/Configkeeper.cs, Globals/Configkeeper.cs.uid Renamed SaveSystem to Configkeeper, removed save functionality, focused on configuration only, added UID file
Globals/Savekeeper.cs, Globals/Savekeeper.cs.uid, project.godot Added new persistent save system class Savekeeper, registered as autoload, and added UID file
Globals/StageProducer.cs Overhauled save/load logic to use Savekeeper, added persistent values, updated initialization and serialization
Scenes/Maps/Scripts/Cartographer.cs, Scenes/UI/Options/Scripts/OptionsMenu.cs, ... Replaced usage of SaveSystem with Savekeeper and/or Configkeeper for save/config actions
Scenes/BattleDirector/Scripts/BattleDirector.cs, Scenes/EventScene/EventScene.cs Updated event pool and save clearing to use new systems
Scenes/CustomSong/CustomScore.cs Refactored event handler subscription/unsubscription for safety
Scenes/NoteManager/Scripts/InputHandler.cs Switched to Configkeeper for input scheme, refactored arrow sprite assignment
Scenes/Puppets/Enemies//.cs Simplified lambda parameters, switched persistent state updates to new system
Scenes/UI/Options/Scripts/HowToPlay.cs, Scenes/UI/Options/Scripts/LanguageSelection.cs Updated tutorial/language setting logic to use new persistent/config systems
Scenes/UI/Remapping/ControlSettings.cs Switched all config references to Configkeeper
Scenes/UI/TitleScreen/Scripts/TitleScreen.cs Updated save/config clearing and unlock logic to use new systems
Classes/MidiMaestro/MidiMaestro.cs Simplified constructor, removed file loading logic, now accepts NoteChart directly
Classes/Events/EventDatabase.cs Replaced unused lambda parameters with underscores, minor cleanup
Scenes/ShopScene/Scripts/ShopScene.cs Changed heal amount from fixed field to computed property
Scenes/NoteManager/Assets/NoteWithOutline.png.import, Scenes/UI/Options/HowToPlay.tscn, ... Corrected file paths and resource references for consistent casing

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Game
    participant Configkeeper
    participant Savekeeper

    User->>Game: Start game / perform actions
    Game->>Configkeeper: Get/Update config values (e.g., input, language)
    Game->>Savekeeper: RecordSave() / LoadFromFile()
    Savekeeper-->>Game: Save/load persistent and run data
    Game->>Savekeeper: Update persistent values (e.g., tutorial, win state)
Loading

Possibly related PRs

Suggested reviewers

  • collectioncard

Poem

In burrows deep, the save files grew,
Now rabbits keep your configs true.
With Savekeeper’s paws and Configkeeper’s mind,
Your progress and settings are safely entwined.
Old systems split, new roots run wide,
Hop ahead—your data’s safe inside!

((\
( -.-)
o_(")(")

✨ Finishing Touches
  • 📝 Generate Docstrings

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@LifeHckr LifeHckr changed the base branch from main to milestone-4 June 21, 2025 00:08
@LifeHckr LifeHckr merged commit 046b9ff into milestone-4 Jun 21, 2025
1 of 2 checks passed
@LifeHckr LifeHckr deleted the RefinedSaves branch June 21, 2025 00:08
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🔭 Outside diff range comments (2)
Classes/Events/EventDatabase.cs (1)

52-61: Potential bug: Missing outcome description assignment.

The lambda calculates localizedName but no longer assigns it to self.OutcomeDescriptions[0]. This means the outcome message won't show which item was stolen, only the money amount will be displayed.

Was this assignment removal intentional? If not, restore it:

                (self, _) =>
                {
                    string stolenMoney = (StageProducer.PlayerStats.Money / 2).ToString();
                    StageProducer.PlayerStats.Money /= 2;

+                   self.OutcomeDescriptions[0] = string.Format(
+                       TranslationServer.Translate("EVENT_EVENT1_OUTCOME1"),
+                       stolenMoney
+                   );
                    self.OutcomeDescriptions[2] = string.Format(
                        TranslationServer.Translate("EVENT_EVENT1_OUTCOME3"),
                        stolenMoney
                    );
                },
Globals/Configkeeper.cs (1)

149-149: Fix error message to use correct class name.

The error message still references the old class name.

-                GD.PushError("SaveSystem.UpdateConfig: Invalid config setting passed. " + setting);
+                GD.PushError("Configkeeper.UpdateConfig: Invalid config setting passed. " + setting);
🧹 Nitpick comments (2)
Scenes/EventScene/EventScene.cs (1)

43-44: Optional cleanup: Remove redundant null check.

Since EventPool is now initialized to an empty list at declaration, the null check is redundant.

-        if (EventPool == null || EventPool.Count == 0)
+        if (EventPool.Count == 0)
             RefreshPool();
Globals/Configkeeper.cs (1)

8-9: Update class documentation to reflect new responsibilities.

The comment still mentions "save files", but this class now only handles configuration.

 /**
- * <summary>SaveSystem: Manages FileI/O of configs and save files.</summary>
+ * <summary>Configkeeper: Manages FileI/O of user configuration settings.</summary>
  */
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3285633 and 0fa4ab2.

⛔ Files ignored due to path filters (1)
  • Scenes/NoteManager/Assets/NoteWithOutline.png is excluded by !**/*.png
📒 Files selected for processing (31)
  • Classes/Events/EventDatabase.cs (4 hunks)
  • Classes/MidiMaestro/MidiMaestro.cs (0 hunks)
  • Globals/Configkeeper.cs (1 hunks)
  • Globals/Configkeeper.cs.uid (1 hunks)
  • Globals/Savekeeper.cs (1 hunks)
  • Globals/Savekeeper.cs.uid (1 hunks)
  • Globals/Scribe.cs (2 hunks)
  • Globals/StageProducer.cs (6 hunks)
  • Scenes/BattleDirector/Scripts/BattleDirector.cs (1 hunks)
  • Scenes/BattleDirector/Scripts/NotePlacementBar.cs (1 hunks)
  • Scenes/BattleDirector/Tutorial/Toriel.cs (2 hunks)
  • Scenes/CustomSong/CustomScore.cs (2 hunks)
  • Scenes/EventScene/EventScene.cs (1 hunks)
  • Scenes/Maps/Scripts/Cartographer.cs (2 hunks)
  • Scenes/NoteManager/Assets/NoteWithOutline.png.import (1 hunks)
  • Scenes/NoteManager/Scripts/InputHandler.cs (3 hunks)
  • Scenes/Puppets/Enemies/BossBlood/P_BossBlood.cs (1 hunks)
  • Scenes/Puppets/Enemies/CyberFox/P_CyberFox.cs (1 hunks)
  • Scenes/Puppets/Enemies/Effigy/P_Effigy.cs (3 hunks)
  • Scenes/Puppets/Enemies/Keythulu/P_Keythulu.cs (3 hunks)
  • Scenes/Puppets/Enemies/Strawman/P_Strawman.cs (7 hunks)
  • Scenes/Puppets/Enemies/Turtle/P_Turtle.cs (1 hunks)
  • Scenes/ShopScene/Scripts/ShopScene.cs (2 hunks)
  • Scenes/UI/Options/HowToPlay.tscn (1 hunks)
  • Scenes/UI/Options/Scripts/HowToPlay.cs (1 hunks)
  • Scenes/UI/Options/Scripts/LanguageSelection.cs (1 hunks)
  • Scenes/UI/Options/Scripts/OptionsMenu.cs (4 hunks)
  • Scenes/UI/Remapping/ControlSettings.cs (4 hunks)
  • Scenes/UI/TitleScreen/Scripts/TitleScreen.cs (2 hunks)
  • Scenes/UI/TitleScreen/TitleScreenEffects.tscn (1 hunks)
  • project.godot (1 hunks)
💤 Files with no reviewable changes (1)
  • Classes/MidiMaestro/MidiMaestro.cs
🔇 Additional comments (44)
Globals/Scribe.cs (2)

718-718: Good standardization of resource path.

Adding the "res://" prefix aligns with Godot's resource loading conventions and makes the path explicit and deployment-agnostic.


803-804: Good consistency improvement.

Simplifying the constructor calls to use positional arguments makes these entries consistent with all other SongTemplate entries in the array, reducing visual noise while maintaining the same functionality.

Also applies to: 808-809

Scenes/NoteManager/Assets/NoteWithOutline.png.import (2)

6-6: Fixed import path casing.
Updated path to correctly use "NoteWithOutline.png", matching the actual filename and preventing missing asset errors during import.


13-14: Updated asset dependency entries.
Adjusted source_file and dest_files to reference the corrected "NoteWithOutline.png" path, ensuring dependencies remain in sync with the import update.

Globals/Configkeeper.cs.uid (1)

1-1: Add unique identifier for Configkeeper.
The new UID file Globals/Configkeeper.cs.uid correctly associates with the refactored Configkeeper class for configuration management.

Globals/Savekeeper.cs.uid (1)

1-1: Add unique identifier for Savekeeper.
The new UID file Globals/Savekeeper.cs.uid enables global registration of the Savekeeper singleton for game state persistence.

Scenes/UI/TitleScreen/TitleScreenEffects.tscn (1)

4-4: Correct resource path for outline texture.
Fixed capitalization in the path attribute to match the actual NoteWithOutline.png asset, preventing lookup failures at runtime.

Scenes/UI/Options/Scripts/LanguageSelection.cs (1)

28-28: Switched config update to new Configkeeper API.
Replaced SaveSystem.UpdateConfig with Configkeeper.UpdateConfig. Confirm that Configkeeper is properly namespaced and that ConfigSettings.LanguageKey aligns with the new config key.

Run the following script to ensure no legacy SaveSystem.UpdateConfig calls remain:

#!/bin/bash
# Verify removal of old SaveSystem.UpdateConfig references
rg "SaveSystem\\.UpdateConfig"
project.godot (1)

27-27: LGTM!

The autoload entry correctly registers the new Savekeeper class as a global singleton following standard Godot conventions.

Scenes/UI/Options/HowToPlay.tscn (1)

10-10: Good catch fixing the filename capitalization.

Correcting "NoteWIthOutline.png" to "NoteWithOutline.png" ensures compatibility with case-sensitive filesystems and maintains consistency.

Classes/Events/EventDatabase.cs (1)

20-20: Good use of underscore for unused parameters.

Replacing unused lambda parameters with _ improves code clarity and follows C# conventions.

Also applies to: 36-36, 52-52, 179-179, 186-186, 190-190

Scenes/UI/Options/Scripts/HowToPlay.cs (1)

49-49: ```shell
#!/bin/bash

Locate the StageProducer class and inspect method definitions

rg "class StageProducer" -n
rg -C2 "UpdatePersistantValues" -n
rg -C2 "UpdatePersistentValues" -n


</details>
<details>
<summary>Scenes/EventScene/EventScene.cs (1)</summary>

`29-29`: **Good defensive programming with list initialization.**

Initializing `EventPool` to an empty list ensures it's never null, preventing potential null reference exceptions.

</details>
<details>
<summary>Scenes/Puppets/Enemies/BossBlood/P_BossBlood.cs (1)</summary>

`40-40`: **LGTM: Good use of discard parameters.**

The lambda parameter simplification correctly uses underscore placeholders for unused parameters, improving code readability and following C# conventions.

</details>
<details>
<summary>Scenes/BattleDirector/Scripts/BattleDirector.cs (1)</summary>

`391-391`: **LGTM: API migration to new save system.**

The replacement of `SaveSystem.ClearSave()` with `Savekeeper.ClearRun()` correctly aligns with the new architecture that separates configuration management from game state persistence.

</details>
<details>
<summary>Scenes/Puppets/Enemies/Effigy/P_Effigy.cs (1)</summary>

`23-23`: **LGTM: Consistent lambda parameter simplification.**

All three lambda expressions correctly use underscore placeholders for unused parameters, maintaining consistency with the refactoring pattern applied across enemy puppet files.



Also applies to: 38-38, 47-47

</details>
<details>
<summary>Scenes/Puppets/Enemies/CyberFox/P_CyberFox.cs (1)</summary>

`39-39`: **LGTM: Proper use of discard parameters.**

The lambda parameter simplification correctly identifies and marks unused parameters with underscores, consistent with the refactoring pattern applied across all enemy puppet files.

</details>
<details>
<summary>Scenes/BattleDirector/Tutorial/Toriel.cs (1)</summary>

`61-63`: **LGTM: Consistent API migration to Configkeeper.**

Both instances correctly migrate from `SaveSystem` to `Configkeeper` for configuration access, maintaining the same functionality while aligning with the new architecture that separates configuration management from save data persistence.



Also applies to: 73-75

</details>
<details>
<summary>Scenes/Puppets/Enemies/Turtle/P_Turtle.cs (2)</summary>

`29-29`: **Good practice: Using underscore for unused parameter.**

The change from `eff` to `_` clearly indicates this parameter is unused, improving code readability and following C# conventions.

---

`34-34`: **Clean simplification of method call.**

Removing the explicit `this` reference makes the code cleaner while maintaining the same functionality.

</details>
<details>
<summary>Scenes/Puppets/Enemies/Keythulu/P_Keythulu.cs (3)</summary>

`45-45`: **Good practice: Using underscores for all unused parameters.**

All lambda parameters are correctly marked as unused, improving code clarity.

---

`66-66`: **Appropriate parameter usage in lambda.**

Only the first parameter `e` is used, while the others are correctly marked as unused.

---

`79-79`: ```shell
#!/bin/bash
# Search for definition of PersistKeys enum in StageProducer
rg -n "enum PersistKeys" -A 10

# Search for the UpdatePersistantValues method signature
rg -n "UpdatePersistantValues" -C 5

# Search for any occurrences of HasWon across the repo
rg -n "HasWon"
Scenes/UI/TitleScreen/Scripts/TitleScreen.cs (2)

51-52: Consistent migration to persistent values system.

The change from config-based HasWon flag to persistent value lookup is consistent with the pattern used throughout the codebase. The integer comparison == 1 aligns with how the value is set in other files.


33-34: Appropriate migration to new save/config system.

The changes correctly separate save data clearing (Savekeeper.ClearRun()) from configuration clearing (Configkeeper.ClearConfig()), aligning with the new system architecture.

#!/bin/bash
# Description: Verify Savekeeper and Configkeeper classes exist with expected methods
# Expected: Find class definitions and ClearRun/ClearConfig methods

# Search for Savekeeper class and ClearRun method
ast-grep --pattern 'class Savekeeper {
  $$$
}'

ast-grep --pattern 'ClearRun($_) {
  $$$
}'

# Search for Configkeeper class and ClearConfig method  
ast-grep --pattern 'class Configkeeper {
  $$$
}'

ast-grep --pattern 'ClearConfig($_) {
  $$$
}'
Scenes/ShopScene/Scripts/ShopScene.cs (2)

346-346: Excellent improvement: Dynamic heal amount calculation.

Converting from a fixed field to a calculated property ensures the heal amount stays proportional to the player's current max health, making the healing more responsive to gameplay progression.


369-369: Proper usage of the new dynamic property.

The heal method correctly uses the new HealAmount property, ensuring the healing scales with the player's current max health.

Scenes/Puppets/Enemies/Strawman/P_Strawman.cs (2)

25-25: Consistent lambda parameter simplification across battle effects.

All unused parameters are properly marked with underscores, improving code readability and following C# conventions consistently across all battle effect handlers.

Also applies to: 35-35, 56-56, 70-70, 86-86, 95-95


104-107: Proper migration to persistent values system for tutorial completion.

The change from SaveSystem.UpdateConfig with "FirstTime" flag to StageProducer.UpdatePersistantValues with TutorialDone key is consistent with the new save system architecture. The achievement call remains appropriately unchanged.

#!/bin/bash
# Description: Verify TutorialDone persistent key is defined and used consistently
# Expected: Find TutorialDone key definition and related usage

# Search for TutorialDone key definition in StageProducer
ast-grep --pattern 'TutorialDone'

# Search for all tutorial-related persistent value operations
rg -A 3 -B 3 "TutorialDone"
Scenes/UI/Options/Scripts/OptionsMenu.cs (3)

47-49: LGTM!

Configuration retrieval correctly updated to use Configkeeper.


59-61: Good refactoring of persistent game state.

The tutorial completion check correctly uses the new persistent value system via StageProducer instead of storing game state in configuration.


121-121: LGTM!

All configuration updates correctly migrated to use Configkeeper.

Also applies to: 135-135, 141-141, 147-147

Scenes/BattleDirector/Scripts/NotePlacementBar.cs (1)

233-235: LGTM!

Configuration retrieval correctly updated to use Configkeeper.

Scenes/UI/Remapping/ControlSettings.cs (3)

154-157: LGTM!

Configuration retrieval correctly updated to use Configkeeper.


254-257: LGTM!

Configuration updates correctly migrated to use Configkeeper.

Also applies to: 480-480


393-440: Good type consistency update.

The ConfigMap dictionary type correctly updated to use Configkeeper.ConfigSettings enums throughout.

Scenes/Maps/Scripts/Cartographer.cs (2)

41-41: LGTM!

Save recording correctly migrated to use Savekeeper.


77-80: Good separation of concerns.

Tutorial completion check correctly uses the persistent value system, and configuration update uses Configkeeper.

Scenes/CustomSong/CustomScore.cs (1)

31-63: Good refactoring of event handler lifecycle management!

Storing the event handlers as fields and properly unsubscribing them in _ExitTree prevents memory leaks from dangling subscriptions. This is a solid improvement to the code.

Scenes/NoteManager/Scripts/InputHandler.cs (2)

68-74: Configuration system migration looks good.

The migration from SaveSystem to Configkeeper is correctly implemented and consistent with the project-wide refactoring.

Also applies to: 97-97


132-143: Nice refactoring of arrow sprite loading logic!

Loading textures once and applying rotations via a loop reduces code duplication and improves maintainability.

Globals/StageProducer.cs (1)

37-40: Proper integration of the new save system.

Good use of event subscription pattern for save serialization. Loading from file on initialization ensures persistent data is available early.

Globals/Savekeeper.cs (2)

47-51: Good use of deferred execution for save operations.

Using CallDeferred ensures the save operation doesn't interfere with the current frame processing, which is a best practice in Godot.


104-112: I've not yet located the Delimiter definition to confirm its intended value and usage. Let’s find where it’s declared:

#!/bin/bash
# Locate the Delimiter constant across the repo
rg -n "Delimiter" -C3 .

Comment on lines +457 to 620
private bool DeserializeRun() //TODO: This is really verbose and bad.
{
if (!Savekeeper.GameSaveObjects.ContainsKey(Savekeeper.DefaultRunSaveHeader))
return false;
int idx = 0;
string loadRun = Savekeeper.GameSaveObjects[Savekeeper.DefaultRunSaveHeader];

var ulongSuccess = Savekeeper.Parse<ulong>(
RunSaveValues.RngSeed.ToString(),
loadRun,
idx,
ulong.TryParse
);
if (!ulongSuccess.Success)
return false;
GlobalRng.Seed = ulongSuccess.Value;
idx = ulongSuccess.NextIdx;

var intSuccess = Savekeeper.Parse<int>(
RunSaveValues.Area.ToString(),
loadRun,
idx,
int.TryParse
);
if (!intSuccess.Success)
return false;
CurLevel = MapLevels.GetLevelFromId(intSuccess.Value);
idx = intSuccess.NextIdx;

var bPoolSuccess = Savekeeper.ParseArray<int>(
RunSaveValues.BattlePool.ToString(),
loadRun,
idx,
int.TryParse
);
if (bPoolSuccess.Success)
{
BattlePool = bPoolSuccess.Value.ToList();
idx = bPoolSuccess.NextIdx;
}
else
{
GD.PushWarning("Could not parse battle pool!");
BattlePool = [];
}

var ePoolSuccess = Savekeeper.ParseArray<int>(
RunSaveValues.EventPool.ToString(),
loadRun,
idx,
int.TryParse
);
if (ePoolSuccess.Success)
{
EventScene.EventPool = ePoolSuccess.Value.ToList();
idx = ePoolSuccess.NextIdx;
}
else
{
GD.PushWarning("Could not parse event pool!");
EventScene.EventPool = [];
}

GenerateMapConsistent();

ulongSuccess = Savekeeper.Parse<ulong>(
RunSaveValues.RngState.ToString(),
loadRun,
idx,
ulong.TryParse
);
if (!ulongSuccess.Success)
return false;
GlobalRng.State = ulongSuccess.Value;
idx = ulongSuccess.NextIdx;

intSuccess = Savekeeper.Parse<int>(
RunSaveValues.LastRoomIdx.ToString(),
loadRun,
idx,
int.TryParse
);
if (!intSuccess.Success)
return false;
CurRoom = intSuccess.Value;
idx = intSuccess.NextIdx;

Scribe.InitRelicPools();
PlayerStats = new PlayerStats();
PlayerStats.CurNotes = [];

var noteSuccess = Savekeeper.ParseArray<int>(
RunSaveValues.NoteIds.ToString(),
loadRun,
idx,
int.TryParse
);
if (!noteSuccess.Success)
return false;
foreach (int noteId in noteSuccess.Value)
{
PlayerStats.AddNote(Scribe.NoteDictionary[noteId]);
}
idx = noteSuccess.NextIdx;

var relicSuccess = Savekeeper.ParseArray<int>(
RunSaveValues.RelicIds.ToString(),
loadRun,
idx,
int.TryParse
);
if (!relicSuccess.Success)
return false;
foreach (int relicId in relicSuccess.Value)
{
PlayerStats.AddRelic(Scribe.RelicDictionary[relicId]);
}
idx = relicSuccess.NextIdx;

intSuccess = Savekeeper.Parse<int>(
RunSaveValues.PlayerHealth.ToString(),
loadRun,
idx,
int.TryParse
);
if (!intSuccess.Success)
return false;
PlayerStats.CurrentHealth = intSuccess.Value;
idx = intSuccess.NextIdx;

intSuccess = Savekeeper.Parse<int>(
RunSaveValues.Money.ToString(),
loadRun,
idx,
int.TryParse
);
if (!intSuccess.Success)
return false;
PlayerStats.Money = intSuccess.Value;
idx = intSuccess.NextIdx;

intSuccess = Savekeeper.Parse<int>(
RunSaveValues.Shortcuts.ToString(),
loadRun,
idx,
int.TryParse
);
if (!intSuccess.Success)
return false;
PlayerStats.Shortcuts = intSuccess.Value;
idx = intSuccess.NextIdx;

intSuccess = Savekeeper.Parse<int>(
RunSaveValues.PlayerMaxCombo.ToString(),
loadRun,
idx,
int.TryParse
);
if (!intSuccess.Success)
return false;
PlayerStats.MaxComboBar = intSuccess.Value;

return true;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider refactoring the verbose deserialization logic.

The DeserializeRun method contains highly repetitive parsing patterns. Each field follows the same pattern: parse, check success, update index. This could be significantly simplified using a helper method or reflection-based approach.

Consider creating a helper method to reduce repetition:

+private bool ParseAndAssign<T>(string key, string data, ref int idx, TryParseHandler<T> parser, Action<T> assign)
+{
+    var result = Savekeeper.Parse<T>(key, data, idx, parser);
+    if (!result.Success)
+        return false;
+    assign(result.Value);
+    idx = result.NextIdx;
+    return true;
+}

Then use it like:

-var ulongSuccess = Savekeeper.Parse<ulong>(
-    RunSaveValues.RngSeed.ToString(),
-    loadRun,
-    idx,
-    ulong.TryParse
-);
-if (!ulongSuccess.Success)
-    return false;
-GlobalRng.Seed = ulongSuccess.Value;
-idx = ulongSuccess.NextIdx;
+if (!ParseAndAssign<ulong>(RunSaveValues.RngSeed.ToString(), loadRun, ref idx, ulong.TryParse, v => GlobalRng.Seed = v))
+    return false;

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In Globals/StageProducer.cs between lines 457 and 620, the DeserializeRun method
is very verbose due to repetitive parsing and success checking for each field.
Refactor by creating a helper method that encapsulates the parse-check-update
pattern to reduce duplication. Replace each individual parse block with calls to
this helper, passing the appropriate parameters, to simplify the code and
improve readability.

Comment on lines +121 to +125
if (value is string s && !s.IsValidFileName())
return valName + Delimiter + InvalidFormatString + Delimiter;

return valName + Delimiter + value + Delimiter;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Potential issue with invalid string handling.

When a string is invalid (!s.IsValidFileName()), you're storing InvalidFormatString but there's no corresponding handling during deserialization to convert it back or flag it as invalid.

Consider adding validation during parse operations to handle InvalidFormatString:

public static bool StringParse(string value, out string result)
{
+    if (value == InvalidFormatString)
+    {
+        result = null;
+        return false;
+    }
    result = value;
    return true;
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (value is string s && !s.IsValidFileName())
return valName + Delimiter + InvalidFormatString + Delimiter;
return valName + Delimiter + value + Delimiter;
}
public static bool StringParse(string value, out string result)
{
if (value == InvalidFormatString)
{
result = null;
return false;
}
result = value;
return true;
}
🤖 Prompt for AI Agents
In Globals/Savekeeper.cs around lines 121 to 125, the code stores
InvalidFormatString when a string is invalid but lacks corresponding handling
during deserialization. To fix this, update the parsing or deserialization logic
to detect InvalidFormatString and handle it appropriately, either by flagging
the value as invalid or converting it back to a recognizable invalid state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants