V1.1 #250
V1.1 #250
Conversation
* Transition to New Save System * Move Persistent Values to StageProducer Moved HasWon and FirstTime(renamed TutorialDone) to StageProducer * Code Cleanup and Tweaks Removed some commented code Added _ to specify ignored parameters
* Initial implementation The enemy descriptions class should be working but the box is small so it doesn't look good. Also the icons need to be different than just the note icon. Such as making the loop icon for loop type effects. * Finished except for temp icons I don't have it set up for every BattleEffectTrigger, so as we use more triggers we need to edit GetTriggerIcon() in EnemyDescriptions.cs * Updated temp arts * Minor Refinements Removed Description on Note class to build translation key. Made description for enemy effect an optional parameter. Adjusted translations for accuracy. Removed effect description from Strawman to not compete screen space with arrow input guides. Better set up description box in controls. --------- Co-authored-by: LifeHckr <jarodthereal@gmail.com>
Updated a few enemy descriptions. Removed debug buttons.
WalkthroughThis update introduces a new save system ( Changes
Sequence Diagram(s)sequenceDiagram
participant Game as Game Logic
participant Configkeeper
participant Savekeeper
participant StageProducer
participant UI
Game->>Configkeeper: Get/Update config (volume, input, etc.)
Game->>Savekeeper: RecordSave() / LoadFromFile()
Savekeeper-->>Game: Triggers Saving/GameSaved/Loaded events
StageProducer->>Savekeeper: Serialize/Deserialize run & persistent data
UI->>Configkeeper: Update config on user actions
UI->>StageProducer: Update persistent flags (tutorial, win)
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed 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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
Scenes/UI/TitleScreen/Scripts/TitleScreen.cs (1)
29-29: Remove the unreachable return statement.The
return;statement on line 29 makes all the debug key handling code below it unreachable. This means the Key0 functionality to clear saves and config won't work.Remove the return statement to restore debug functionality:
- return;
🧹 Nitpick comments (10)
Scenes/BattleDirector/Assets/DamageInstanceSymbol.png.import (1)
18-24: (Nit) Consider bumping lossy compression quality for UI icons
compress/lossy_quality=0.7is fine for large backgrounds, but small crisp UI symbols often benefit from higher quality (0.8-0.9) without a noticeable memory impact.Scenes/BattleDirector/Assets/BattleStartSymbol.png.import (1)
18-24: (Nit) Consistency with compression settingsIf you decide to tweak the quality for one battle-UI icon, keep the setting consistent across this and the other new symbol files to avoid subtle visual differences.
Scenes/BattleDirector/Assets/LoopSymbol.png.import (1)
18-24: (Nit) Consider enabling mipmaps for symbols used with scaling / animationsIf this icon is ever scaled or rotated in-game, enabling
mipmaps/generatecan reduce shimmering. For pixel-perfect usage you can keep it disabled.Scenes/BattleDirector/Assets/BattleEndSymbol.png.import (1)
18-24: (Nit) Align compression quality with other UI iconsMaintain uniform visual fidelity by matching
compress/lossy_qualitywith the rest of the battle-UI assets.Scenes/EventScene/EventScene.cs (1)
29-29: LGTM! Proper static field initialization.The explicit initialization of
EventPoolto an empty list prevents potential null reference exceptions and aligns with the new save/load system. The modern collection initialization syntax[]is also a nice touch.Consider removing the redundant null check on line 43 since
EventPoolis now guaranteed to be non-null:- if (EventPool == null || EventPool.Count == 0) + if (EventPool.Count == 0)Scenes/Puppets/Enemies/Effigy/P_Effigy.cs (1)
45-54: Consider adding an identifier for consistency.The third
EnemyEffect(OnBattleStart trigger) uses the discard pattern like the others but lacks a string identifier. For consistency with the enemy description system, consider adding an identifier like"EFFIGY_EFFECT3"or similar.new EnemyEffect( this, BattleEffectTrigger.OnBattleStart, 1, (e, _, _) => { _tutorialInstance = Toriel.AttachNewToriel(e.BD); _tutorialInstance.BossDialogue(); - } + }, + "EFFIGY_EFFECT3" ),Globals/Configkeeper.cs (2)
8-8: Update class comment to reflect new purpose.The comment still mentions "save files" but this class no longer handles save functionality - only configuration management.
- * <summary>SaveSystem: Manages FileI/O of configs and save files.</summary> + * <summary>Configkeeper: Manages FileI/O of user configuration settings.</summary>
149-149: Update error message to use correct class name.The error message still references the old
SaveSystemclass name.- GD.PushError("SaveSystem.UpdateConfig: Invalid config setting passed. " + setting); + GD.PushError("Configkeeper.UpdateConfig: Invalid config setting passed. " + setting);Globals/StageProducer.cs (1)
457-620: Comprehensive save/load implementation with room for improvement.The serialization and deserialization methods handle all necessary game state. While the deserialization is verbose (as noted in the TODO comment), it provides clear error handling for each data element.
Consider creating a generic deserialization helper method to reduce code duplication in
DeserializeRun(). This would make the code more maintainable and less error-prone.Globals/Savekeeper.cs (1)
136-144: Remove trailing array delimiter.The array formatting includes an unnecessary trailing delimiter after the last element, which complicates parsing.
+ bool first = true; foreach (object o in value) { + if (!first) + retString += ArrayDelimiter; + first = false; + if (o is string s && !s.IsValidFileName()) { - retString += InvalidFormatString + ArrayDelimiter; + retString += InvalidFormatString; continue; } - retString += o.ToString() + ArrayDelimiter; + retString += o.ToString(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
Globals/Translations/Translations.csvis excluded by!**/*.csvScenes/BattleDirector/Assets/BattleEndSymbol.pngis excluded by!**/*.pngScenes/BattleDirector/Assets/BattleStartSymbol.pngis excluded by!**/*.pngScenes/BattleDirector/Assets/DamageInstanceSymbol.pngis excluded by!**/*.pngScenes/BattleDirector/Assets/LoopSymbol.pngis excluded by!**/*.pngScenes/NoteManager/Assets/NoteWithOutline.pngis excluded by!**/*.png
📒 Files selected for processing (44)
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/Assets/BattleEndSymbol.png.import(1 hunks)Scenes/BattleDirector/Assets/BattleStartSymbol.png.import(1 hunks)Scenes/BattleDirector/Assets/DamageInstanceSymbol.png.import(1 hunks)Scenes/BattleDirector/Assets/LoopSymbol.png.import(1 hunks)Scenes/BattleDirector/BattleScene.tscn(6 hunks)Scenes/BattleDirector/EnemyDescriptions.tscn(1 hunks)Scenes/BattleDirector/Scripts/BattleDirector.cs(5 hunks)Scenes/BattleDirector/Scripts/EnemyDescriptions.cs(1 hunks)Scenes/BattleDirector/Scripts/EnemyDescriptions.cs.uid(1 hunks)Scenes/BattleDirector/Scripts/NotePlacementBar.cs(1 hunks)Scenes/BattleDirector/Tutorial/Toriel.cs(4 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(2 hunks)Scenes/Puppets/Enemies/EnemyEffect.cs(1 hunks)Scenes/Puppets/Enemies/Holograeme/P_Holograeme.cs(2 hunks)Scenes/Puppets/Enemies/Keythulu/P_Keythulu.cs(3 hunks)Scenes/Puppets/Enemies/LWS/P_LWS.cs(1 hunks)Scenes/Puppets/Enemies/Parasifly/P_Parasifly.cs(1 hunks)Scenes/Puppets/Enemies/Strawman/P_Strawman.cs(7 hunks)Scenes/Puppets/Enemies/TheGWS/P_TheGWS.cs(1 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 context used
🧠 Learnings (11)
Scenes/UI/TitleScreen/TitleScreenEffects.tscn (1)
Learnt from: collectioncard
PR: Project-Funk-Engine/ProjectFunkEngine#84
File: scenes/UI/EndScreen.tscn:22-25
Timestamp: 2025-03-03T03:48:45.545Z
Learning: Do not suggest modifications to .tscn files (Godot scene files), as they should only be modified through the Godot Editor rather than by direct text editing.
Scenes/NoteManager/Assets/NoteWithOutline.png.import (1)
Learnt from: collectioncard
PR: Project-Funk-Engine/ProjectFunkEngine#84
File: scenes/UI/EndScreen.tscn:22-25
Timestamp: 2025-03-03T03:48:45.545Z
Learning: Do not suggest modifications to .tscn files (Godot scene files), as they should only be modified through the Godot Editor rather than by direct text editing.
Scenes/UI/Options/HowToPlay.tscn (1)
Learnt from: collectioncard
PR: Project-Funk-Engine/ProjectFunkEngine#84
File: scenes/UI/EndScreen.tscn:22-25
Timestamp: 2025-03-03T03:48:45.545Z
Learning: Do not suggest modifications to .tscn files (Godot scene files), as they should only be modified through the Godot Editor rather than by direct text editing.
Scenes/Puppets/Enemies/Strawman/P_Strawman.cs (1)
Learnt from: LifeHckr
PR: Project-Funk-Engine/ProjectFunkEngine#239
File: Globals/Scribe.cs:643-665
Timestamp: 2025-06-18T18:42:42.199Z
Learning: In the BattleDirector event system, calling Note.OnHit() directly only executes the note's effect function and does not automatically trigger NoteHit events. The InvokeNoteHit() method that fires NoteHit events is called separately in the normal flow. Therefore, relics that call Note.OnHit() from within NoteHit event handlers (like the Quito relic) do not cause infinite recursion.
Scenes/Puppets/Enemies/BossBlood/P_BossBlood.cs (1)
Learnt from: LifeHckr
PR: Project-Funk-Engine/ProjectFunkEngine#239
File: Globals/Scribe.cs:643-665
Timestamp: 2025-06-18T18:42:42.227Z
Learning: In the BattleDirector event system, calling Note.OnHit() directly only executes the note's effect function and does not automatically trigger NoteHit events. The InvokeNoteHit() method that fires NoteHit events is called separately in the normal flow. Therefore, relics that call Note.OnHit() from within NoteHit event handlers (like the Quito relic) do not cause infinite recursion.
Scenes/BattleDirector/Scripts/NotePlacementBar.cs (1)
Learnt from: LifeHckr
PR: Project-Funk-Engine/ProjectFunkEngine#239
File: Scenes/ShopScene/Scripts/ShopScene.cs:335-338
Timestamp: 2025-06-18T18:27:35.453Z
Learning: In Scenes/ShopScene/Scripts/ShopScene.cs, the RemoveNote method uses Array.IndexOf to find and remove a specific note by index instead of using the existing RemoveNote(Note note) overload because that overload removes all notes of the same type rather than the specific note reference. This is temporary until the overload can be fixed to handle single instance removal.
Scenes/BattleDirector/EnemyDescriptions.tscn (1)
Learnt from: collectioncard
PR: Project-Funk-Engine/ProjectFunkEngine#84
File: scenes/UI/EndScreen.tscn:22-25
Timestamp: 2025-03-03T03:48:45.545Z
Learning: Do not suggest modifications to .tscn files (Godot scene files), as they should only be modified through the Godot Editor rather than by direct text editing.
Scenes/BattleDirector/BattleScene.tscn (1)
Learnt from: collectioncard
PR: Project-Funk-Engine/ProjectFunkEngine#84
File: scenes/UI/EndScreen.tscn:22-25
Timestamp: 2025-03-03T03:48:45.545Z
Learning: Do not suggest modifications to .tscn files (Godot scene files), as they should only be modified through the Godot Editor rather than by direct text editing.
Scenes/CustomSong/CustomScore.cs (2)
Learnt from: LifeHckr
PR: Project-Funk-Engine/ProjectFunkEngine#239
File: Scenes/ShopScene/Scripts/ShopScene.cs:335-338
Timestamp: 2025-06-18T18:27:35.453Z
Learning: In Scenes/ShopScene/Scripts/ShopScene.cs, the RemoveNote method uses Array.IndexOf to find and remove a specific note by index instead of using the existing RemoveNote(Note note) overload because that overload removes all notes of the same type rather than the specific note reference. This is temporary until the overload can be fixed to handle single instance removal.
Learnt from: LifeHckr
PR: Project-Funk-Engine/ProjectFunkEngine#239
File: Globals/Scribe.cs:643-665
Timestamp: 2025-06-18T18:42:42.227Z
Learning: In the BattleDirector event system, calling Note.OnHit() directly only executes the note's effect function and does not automatically trigger NoteHit events. The InvokeNoteHit() method that fires NoteHit events is called separately in the normal flow. Therefore, relics that call Note.OnHit() from within NoteHit event handlers (like the Quito relic) do not cause infinite recursion.
Scenes/Puppets/Enemies/Effigy/P_Effigy.cs (1)
Learnt from: LifeHckr
PR: Project-Funk-Engine/ProjectFunkEngine#239
File: Globals/Scribe.cs:643-665
Timestamp: 2025-06-18T18:42:42.199Z
Learning: In the BattleDirector event system, calling Note.OnHit() directly only executes the note's effect function and does not automatically trigger NoteHit events. The InvokeNoteHit() method that fires NoteHit events is called separately in the normal flow. Therefore, relics that call Note.OnHit() from within NoteHit event handlers (like the Quito relic) do not cause infinite recursion.
Scenes/ShopScene/Scripts/ShopScene.cs (1)
Learnt from: LifeHckr
PR: Project-Funk-Engine/ProjectFunkEngine#239
File: Scenes/ShopScene/Scripts/ShopScene.cs:335-338
Timestamp: 2025-06-18T18:27:35.453Z
Learning: In Scenes/ShopScene/Scripts/ShopScene.cs, the RemoveNote method uses Array.IndexOf to find and remove a specific note by index instead of using the existing RemoveNote(Note note) overload because that overload removes all notes of the same type rather than the specific note reference. This is temporary until the overload can be fixed to handle single instance removal.
🧬 Code Graph Analysis (9)
Scenes/UI/Options/Scripts/LanguageSelection.cs (1)
Globals/Configkeeper.cs (2)
Configkeeper(10-386)UpdateConfig(89-153)
Classes/Events/EventDatabase.cs (2)
Globals/StageProducer.cs (1)
StageProducer(11-623)Scenes/Puppets/Scripts/PlayerStats.cs (1)
PlayerStats(6-88)
Scenes/Puppets/Enemies/EnemyEffect.cs (2)
Scenes/Puppets/Enemies/EnemyPuppet.cs (2)
EnemyEffect(9-12)EnemyPuppet(3-13)Globals/FunkEngineNameSpace.cs (1)
BattleEventArgs(433-436)
Scenes/BattleDirector/Scripts/NotePlacementBar.cs (1)
Globals/Configkeeper.cs (1)
Configkeeper(10-386)
Scenes/UI/Options/Scripts/HowToPlay.cs (1)
Globals/StageProducer.cs (2)
StageProducer(11-623)UpdatePersistantValues(371-376)
Scenes/UI/Options/Scripts/OptionsMenu.cs (2)
Globals/Configkeeper.cs (2)
Configkeeper(10-386)UpdateConfig(89-153)Globals/StageProducer.cs (2)
StageProducer(11-623)GetPersistantVal(366-369)
Scenes/BattleDirector/Tutorial/Toriel.cs (1)
Globals/Configkeeper.cs (1)
Configkeeper(10-386)
Scenes/NoteManager/Scripts/InputHandler.cs (1)
Globals/Configkeeper.cs (2)
Configkeeper(10-386)UpdateConfig(89-153)
Scenes/UI/TitleScreen/Scripts/TitleScreen.cs (3)
Globals/Savekeeper.cs (2)
Savekeeper(13-281)ClearRun(271-278)Globals/Configkeeper.cs (2)
Configkeeper(10-386)ClearConfig(380-384)Globals/StageProducer.cs (2)
StageProducer(11-623)GetPersistantVal(366-369)
🔇 Additional comments (67)
Classes/Events/EventDatabase.cs (3)
20-20: LGTM: Proper use of discard pattern for unused parameters.The lambda parameters have been correctly updated to use the discard
_for the unusednodeparameter while keepingselfsince it's used within each lambda body. This improves code clarity by making it explicit that thenodeparameter is intentionally unused.Also applies to: 36-36, 52-52
179-179: LGTM: Consistent use of double discard pattern.All three outcome lambdas in the third event correctly use
(_, _)since neither theselfnornodeparameters are used within their lambda bodies. This maintains consistency with the discard pattern applied throughout the codebase.Also applies to: 186-186, 190-190
57-61: Confirm correct indexing for EVENT1 outcomesThe three lambdas for Event 1 now assign to
OutcomeDescriptions[0],[1], and[2]respectively. Removing the stray[0]assignment from the third lambda is correct—each outcome only updates its own slot.• Classes/Events/EventDatabase.cs – the Event 1 outcome handlers at indices 0, 1, and 2
No further changes needed.Globals/Scribe.cs (3)
802-806: Astrorat entry configured correctlyThe positional arguments are in the proper order (name → enemy scene list → chart), and the chart path leverages the updated
DefaultNoteChartPath. Looks good.
808-811: CatGirl entry configured correctlySame verdict as the previous block—arguments are valid and the resource path follows the new convention.
718-718: No remaining hard-coded “Audio/songMaps/” references found
Ran searches forAudio/songMapsandsongMapsacross all.csfiles with zero matches, confirming the switch tores://Audio/songMaps/is complete.Scenes/BattleDirector/Assets/DamageInstanceSymbol.png.import (1)
13-14: Confirmed presence of DamageInstanceSymbol.pngThe file
Scenes/BattleDirector/Assets/DamageInstanceSymbol.pngexists in the repository as expected. No further action required.Scenes/BattleDirector/Assets/BattleStartSymbol.png.import (1)
13-14: Double-check thatBattleStartSymbol.pngwas committedSame concern as the previous asset: ensure the PNG file itself is present so the import entry isn’t dangling.
Scenes/BattleDirector/Assets/LoopSymbol.png.import (1)
13-14: ConfirmLoopSymbol.pngexistsPlease verify the PNG is actually included; otherwise the import will break.
Scenes/BattleDirector/Assets/BattleEndSymbol.png.import (1)
13-14: CheckBattleEndSymbol.pngpresenceSame verification step—ensure the PNG file itself is present in the repository.
Globals/Savekeeper.cs.uid (1)
1-1: UID file looks goodTracking the
.uidalongside the newSavekeeper.csscript keeps Godot’s internal references stable. No issues.Scenes/BattleDirector/Scripts/EnemyDescriptions.cs.uid (1)
1-1: UID entry looks fine.The file is the standard single-line UID generated by the editor – nothing to review here.
Globals/Configkeeper.cs.uid (1)
1-1: UID entry looks fine.No action required.
Scenes/UI/TitleScreen/TitleScreenEffects.tscn (1)
4-4: Correct texture path casing – confirmed.
NoteWithOutline.pngmatches the asset’s actual filename and is present exactly once in the repo (excluding engine.importfiles). No further changes needed.
Approved.Scenes/UI/Options/HowToPlay.tscn (1)
10-10: Path fix replicated consistently – looks good.Same casing correction as in the title screen. Verified that the referenced UID (
uid://coav3xvksq4jy) matches the texture in the import metadata.project.godot (1)
25-33: Autoload order and Node inheritance confirmed for Savekeeper
res://Globals/Savekeeper.csexists and is registered correctly.public partial class Savekeeper : Node– inheritance verified.- It’s the first autoload entry, so no earlier singletons reference it.
Scenes/Puppets/Enemies/BossBlood/P_BossBlood.cs (2)
34-36: LGTM! Effect identifier added for enemy description system.The addition of "BOSSBLOOD_EFFECT1" identifier aligns with the new enemy descriptions UI system and will enable better effect tracking and display.
41-41: Good parameter optimization using discard pattern.Replacing unused parameters
effandvalwith_discards improves code readability by clearly indicating these parameters are not used in the lambda expression.Scenes/Puppets/Enemies/Parasifly/P_Parasifly.cs (1)
50-52: LGTM! Effect identifier added for enemy description system.The addition of "PARASIFLY_EFFECT1" identifier for the death effect (OnDamageInstance trigger) is consistent with the broader enemy descriptions refactoring and enables proper effect tracking in the UI.
Scenes/Puppets/Enemies/Holograeme/P_Holograeme.cs (2)
49-51: LGTM! Effect identifier added for battle start mechanics.The addition of "HOLOGRAEME_EFFECT1" identifier for the OnBattleStart effect correctly enables tracking of the enemy's battle initialization mechanics (AutoPlay, PlayerDisabled, etc.) in the enemy descriptions system.
102-104: LGTM! Effect identifier added for damage mitigation effect.The addition of "HOLOGRAEME_EFFECT2" identifier for the OnDamageInstance effect properly enables tracking of the enemy's damage mitigation logic in the descriptions UI system.
Scenes/UI/Options/Scripts/HowToPlay.cs (1)
49-49: Review comment not applicable: tutorial flag semantics are correctThe
StageProducersystem uses0to indicate “tutorial not done” and1for “tutorial completed.” InHowToPlay.DoTutorial, resetting the flag to0correctly triggers the tutorial, mirroring the old behavior of settingFirstTime = true. No changes are needed here.Likely an incorrect or invalid review comment.
Scenes/UI/Options/Scripts/LanguageSelection.cs (1)
28-28: LGTM! Clean migration to new configuration system.The replacement of
SaveSystem.UpdateConfigwithConfigkeeper.UpdateConfigcorrectly migrates the language setting update to the new configuration management system while maintaining identical functionality.Scenes/Puppets/Enemies/Turtle/P_Turtle.cs (1)
29-40: LGTM! Clean refactoring of EnemyEffect constructor.The changes properly mark unused parameters with underscore, remove unnecessary
this.prefix, and add the string identifier for the enemy description system. These changes align with the broader refactoring pattern across enemy puppet scripts.Scenes/NoteManager/Assets/NoteWithOutline.png.import (1)
6-6: LGTM! Typo fix in asset path.The capitalization correction from "NoteWIthOutline.png" to "NoteWithOutline.png" ensures consistent asset referencing across the project.
Also applies to: 13-14
Scenes/Puppets/Enemies/CyberFox/P_CyberFox.cs (1)
39-45: LGTM! Consistent EnemyEffect refactoring.The changes properly mark unused lambda parameters with underscores and add the string identifier for the enemy description system, maintaining consistency with the broader refactoring pattern.
Scenes/Puppets/Enemies/TheGWS/P_TheGWS.cs (1)
57-59: LGTM! Minimal and correct EnemyEffect update.The addition of the string identifier
"GWS_EFFECT1"is consistent with the enemy description system refactoring. The lambda parameters are correctly preserved since they're actually used in the logic, unlike other enemy scripts where unused parameters were marked with underscores.Scenes/Puppets/Enemies/LWS/P_LWS.cs (1)
33-35: LGTM! Clean addition of enemy effect description.The addition of the "LWS_EFFECT1" description parameter to the EnemyEffect constructor aligns perfectly with the new enemy description system. The naming convention is consistent and will support the UI display of effect descriptions.
Scenes/Puppets/Enemies/Strawman/P_Strawman.cs (2)
25-25: LGTM! Good practice using underscores for unused lambda parameters.The simplification of lambda parameters by replacing unused parameters with underscores improves code readability and clearly indicates which parameters are intentionally unused.
Also applies to: 35-35, 56-56, 70-70, 86-86, 95-95
104-107: LGTM! Proper migration to the new persistent data system.The change from
SaveSystem.UpdateConfig("FirstTime", ...)toStageProducer.UpdatePersistantValues(StageProducer.PersistKeys.TutorialDone, 1)correctly aligns with the new save system architecture. Using an enum-based key (TutorialDone) provides better type safety and consistency compared to string-based keys.Scenes/ShopScene/Scripts/ShopScene.cs (2)
346-346: LGTM! Improved healing system with proportional scaling.Converting the heal amount to a computed property that scales with maximum health (
MaxHealth / 4) is excellent game design. This ensures healing remains proportionally valuable regardless of max health upgrades, maintaining game balance better than a fixed heal amount.
369-369: LGTM! Consistent usage of the new HealAmount property.The heal call correctly uses the new
HealAmountproperty, maintaining consistency with the proportional healing system.Scenes/UI/TitleScreen/Scripts/TitleScreen.cs (2)
33-34: LGTM! Proper migration to new save and config systems.The replacement of
SaveSystem.ClearSave()andSaveSystem.ClearConfig()withSavekeeper.ClearRun()andConfigkeeper.ClearConfig()correctly aligns with the new save system architecture.
51-52: LGTM! Consistent migration to persistent value system.The change from
SaveSystem.GetConfigValue(SaveSystem.ConfigSettings.HasWon)toStageProducer.GetPersistantVal(StageProducer.PersistKeys.HasWon) == 1properly uses the new persistent data system with explicit integer comparison, which is more type-safe than casting to boolean.Scenes/Puppets/Enemies/EnemyEffect.cs (1)
11-11: LGTM! Well-designed addition of description support.The implementation correctly adds description support to enemy effects:
- The
Descriptionproperty has a private setter, preventing external modification- The optional constructor parameter with null default maintains backward compatibility
- The assignment in the constructor is straightforward and correct
This design enables the new enemy description UI system while maintaining the existing EnemyEffect interface.
Also applies to: 17-19, 26-26
Scenes/UI/Options/Scripts/OptionsMenu.cs (2)
47-48: LGTM! Clean migration to Configkeeper for configuration management.The systematic replacement of
SaveSystemcalls withConfigkeepercalls is consistent and correct. All method signatures and enum values are properly updated.Also applies to: 121-121, 135-135, 141-141, 147-147
61-61: Correct migration of tutorial completion check to persistent data.The change from
SaveSystemconfig toStageProducer.GetPersistantValis appropriate since tutorial completion is persistent game state rather than configuration data.Scenes/UI/Remapping/ControlSettings.cs (2)
154-157: LGTM! Consistent Configkeeper API usage.The migration from
SaveSystemtoConfigkeeperis implemented correctly with proper method calls and enum values.Also applies to: 254-257, 480-480
395-440: Good update to ConfigMap dictionary with new enum type.The dictionary properly uses
Configkeeper.ConfigSettingsenum values, maintaining consistency with the new configuration system.Scenes/Puppets/Enemies/Keythulu/P_Keythulu.cs (4)
30-31: Good elimination of magic number with named constant.Introducing
effect1Valconstant improves code readability and maintainability by replacing the magic number 6.Also applies to: 37-37
42-42: Good addition of effect identifier.Adding the identifier string
"KEYTHULU_EFFECT1"follows the pattern established in other enemy classes and improves debuggability.
48-48: Clean lambda parameter simplification.Using underscores for unused parameters is a good practice that clearly indicates the parameters are intentionally ignored.
Also applies to: 69-69
82-82: Correct migration to persistent data management.The change from
SaveSystemtoStageProducer.UpdatePersistantValuesis appropriate for tracking win state as persistent game data.Scenes/BattleDirector/Tutorial/Toriel.cs (2)
61-63: Consistent migration to Configkeeper API.The replacement of
SaveSystem.GetConfigValuewithConfigkeeper.GetConfigValuefollows the established migration pattern correctly.Also applies to: 73-75
344-344: Proper UI state management for enemy descriptions.The visibility control of
_currentDirector.Descriptionsappropriately hides the descriptions during boss dialogue and restores them when the dialogue is complete.Also applies to: 359-359
Scenes/BattleDirector/Scripts/NotePlacementBar.cs (1)
233-235: Consistent Configkeeper migration in NotePlaced method.The replacement of
SaveSystem.GetConfigValuewithConfigkeeper.GetConfigValuefor input type retrieval is correct and follows the established migration pattern.Scenes/BattleDirector/EnemyDescriptions.tscn (1)
1-51: LGTM! Well-structured UI scene for enemy descriptions.The scene file is properly structured with appropriate node hierarchy, anchoring, and resource references. The UI layout will provide a centered, framed container for displaying enemy description content.
Scenes/BattleDirector/Scripts/BattleDirector.cs (4)
37-38: LGTM! Proper integration of enemy descriptions.The new exported field follows the existing pattern and will be properly wired up through the Godot editor.
145-145: LGTM! Consistent cleanup of UI elements.Properly queues the descriptions UI for cleanup when the battle begins, maintaining clean resource management.
208-208: LGTM! Proper initialization of enemy descriptions.Correctly sets up the enemy descriptions UI with the first enemy's data during enemy initialization.
397-397: Verify the save system migrationEnsure that Savekeeper.ClearRun() fully replaces the old SaveSystem.ClearSave() behavior for run-specific data cleanup on battle loss.
Action items:
- Locate and review the ClearRun() implementation in the Savekeeper class and compare its logic to the legacy ClearSave() method.
- Confirm that every step of clearing run data (e.g., resetting flags, deleting temporary files) is covered.
- Search the codebase to verify there are no remaining calls to SaveSystem.ClearSave().
Scenes/Puppets/Enemies/Effigy/P_Effigy.cs (2)
23-23: LGTM! Good use of discard pattern for unused parameters.Using
_for unused lambda parameters improves code readability and clearly indicates which parameters are not used in the lambda bodies.Also applies to: 39-39, 49-49
33-33: LGTM! Effect identifiers support enemy descriptions.The string identifiers enable the new enemy description system to provide contextual information about these effects to players.
Also applies to: 43-43
Scenes/CustomSong/CustomScore.cs (3)
31-32: LGTM! Proper delegate field storage for event cleanup.Storing the event handlers in private fields enables proper unsubscription in the cleanup method, preventing potential memory leaks.
34-57: LGTM! Clean event subscription pattern.The refactored event subscription approach improves maintainability by making the handlers explicit and reusable for cleanup.
59-63: LGTM! Essential event cleanup implementation.The
_ExitTree()override properly unsubscribes from events when the node is removed from the scene tree, preventing memory leaks and ensuring clean resource management.Scenes/Maps/Scripts/Cartographer.cs (3)
41-41: LGTM! Save system migration to Savekeeper.The change from
SaveSystem.SaveGame()toSavekeeper.RecordSave()aligns with the new save architecture that separates save file management from configuration.
80-80: LGTM! Configuration system migration to Configkeeper.The change from
SaveSystem.UpdateConfigtoConfigkeeper.UpdateConfigproperly separates configuration management from save file handling in the new architecture.
77-77: Confirm tutorial completion flag migration and equivalencePlease verify that the old
FirstTimecheck has been fully replaced byPersistKeys.TutorialDoneand that the new logic behaves identically:• Ensure there are no remaining calls to
SaveSystem.GetConfigValue(SaveSystem.ConfigSettings.FirstTime)
• ConfirmTutorialDonedefaults to 0 on a fresh game and only flips to 1 when the tutorial truly completes (seeP_Strawman.cs)
• Verify all guards against running the tutorial (GetPersistantVal(...)==1) in:
Scenes/Maps/Scripts/Cartographer.csScenes/UI/Options/Scripts/OptionsMenu.cs
• Check that starting a new game or replaying the tutorial resetsTutorialDoneappropriately (e.g., inScenes/UI/Options/Scripts/HowToPlay.cs)Scenes/BattleDirector/BattleScene.tscn (1)
1-158: Scene file modifications look appropriate.The addition of the
EnemyDescriptionsnode and theStartButtonoffset adjustments appear to be valid changes made through the Godot Editor.Scenes/NoteManager/Scripts/InputHandler.cs (2)
68-74: Configuration system migration implemented correctly.The migration from
SaveSystemtoConfigkeeperfor input type configuration is properly implemented.
132-143: Excellent refactoring of UpdateArrowSprites method.The new implementation is more efficient and maintainable:
- Textures are cached instead of loaded multiple times
- Loop-based approach eliminates code duplication
- Clear array initialization for rotations
Scenes/BattleDirector/Scripts/EnemyDescriptions.cs (1)
1-78: Well-implemented enemy descriptions UI component.The class provides a clean implementation for displaying enemy effect descriptions with associated icons. The TODO comment on line 68 appropriately flags future extensibility needs for additional trigger types.
Globals/StageProducer.cs (3)
37-40: Proper integration with the new Savekeeper system.The event handler registration and initialization sequence correctly sets up both run and persistent data serialization.
354-406: Well-designed persistent data management system.The implementation provides a clean abstraction for managing persistent values like tutorial completion and win status, with proper serialization support.
19-19: Empty List Initialization ([]) Is Supported
The project’s.csprojtargetsnet8.0, which includes C# 12 support. Using[]to initialize an emptyList<int>at lines 19, 94–95, and 349–350 is valid. No changes needed.Globals/Savekeeper.cs (1)
170-200: Well-designed generic parsing implementation.The use of delegates and the ParseResult struct provides a flexible and informative parsing system with good error handling.
| public override void _UnhandledInput(InputEvent @event) | ||
| { | ||
| return; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify if disabling debug input is intentional.
The immediate return statement completely disables all debug functionality in _UnhandledInput, including enemy killing (Key0) and energy refill (Key9) debug features. Confirm this is intentional for the production build.
🏁 Script executed:
#!/bin/bash
# Description: Check if there are any debug-related configuration flags or build settings
# that might indicate this is intentional for production builds.
# Look for debug-related configurations
rg -A 3 -B 3 "debug|DEBUG|Debug" --type csLength of output: 8231
Ensure debug inputs aren’t unconditionally disabled
The return; at the top of _UnhandledInput in
Scenes/BattleDirector/Scripts/BattleDirector.cs (around lines 242–244)
short-circuits the entire method—including your Key0 and Key9 debug commands—in all builds. If you only want to disable these in production, wrap the early exit in a release-only check. For example:
public override void _UnhandledInput(InputEvent @event)
{
- return;
+ #if !DEBUG
+ return;
+ #endif
if (@event is InputEventKey eventKey && eventKey.Pressed)
{
if (eventKey.Keycode == Key.Key0)
DebugKillEnemy();
if (eventKey.Keycode == Key.Key9)
DebugRefillEnergy();
}
}– Confirm this matches your intent and adjust the conditional (e.g. use your CI/CD build symbols or Godot’s editor/runtime checks) so debug features remain available during development.
🤖 Prompt for AI Agents
In Scenes/BattleDirector/Scripts/BattleDirector.cs around lines 242 to 244, the
method _UnhandledInput currently returns immediately, disabling all input
handling including debug commands like Key0 and Key9 in every build. To fix
this, wrap the early return statement in a conditional that only triggers in
production or release builds, using appropriate build symbols or runtime checks,
so that debug inputs remain active during development and testing.
| if (!handler(s, out var result)) | ||
| return new ParseResult<T[]>( | ||
| list.ToArray(), | ||
| false, | ||
| finalIdx, | ||
| $"Unable to parse from: \"{s}\" to type: {typeof(T)}. Returning any successful values." | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reconsider partial result strategy on parsing failure.
Returning partial results when array parsing fails could lead to data corruption or unexpected behavior. Consider failing entirely if any element cannot be parsed.
Either return an empty array on any parsing failure, or add a flag to the ParseResult to indicate partial success.
🤖 Prompt for AI Agents
In Globals/Savekeeper.cs around lines 229 to 235, the current code returns
partial results when parsing an array fails, which risks data corruption or
unexpected behavior. Modify the logic to either return an empty array upon any
parsing failure or enhance the ParseResult class to include a flag that
explicitly indicates partial success, ensuring the caller can distinguish
between full and partial parse outcomes.
| if (value is string s && !s.IsValidFileName()) | ||
| return valName + Delimiter + InvalidFormatString + Delimiter; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reconsider string validation approach.
Using IsValidFileName() to validate save data strings is overly restrictive and could cause data loss. Save data might legitimately contain characters that aren't valid in filenames (e.g., colons, quotes, etc.).
Consider escaping special characters instead of marking strings as invalid, or use a different validation approach that only prevents delimiter conflicts.
🤖 Prompt for AI Agents
In Globals/Savekeeper.cs around lines 121 to 122, the current use of
IsValidFileName() to validate strings is too restrictive and may incorrectly
mark valid save data as invalid. Instead of rejecting strings based on filename
validity, modify the validation to allow all characters but escape or encode any
special characters that conflict with the delimiter. This approach prevents data
loss by preserving the original string content while ensuring delimiter
conflicts are avoided.
| public static Dictionary<String, String> GameSaveObjects { get; private set; } = | ||
| new Dictionary<String, String>(); |
There was a problem hiding this comment.
Consider thread safety for the static dictionary.
The static GameSaveObjects dictionary is mutable and accessed from static methods, which could lead to thread safety issues if accessed concurrently. Consider using ConcurrentDictionary or implementing proper locking mechanisms.
- public static Dictionary<String, String> GameSaveObjects { get; private set; } =
- new Dictionary<String, String>();
+ public static ConcurrentDictionary<String, String> GameSaveObjects { get; private set; } =
+ new ConcurrentDictionary<String, String>();Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In Globals/Savekeeper.cs around lines 39 to 40, the static dictionary
GameSaveObjects is mutable and accessed concurrently, risking thread safety
issues. Replace the Dictionary<String, String> with a
ConcurrentDictionary<String, String> to ensure thread-safe operations without
explicit locking, or alternatively implement locking mechanisms around all
accesses to the dictionary to prevent concurrent modification problems.
| public static void ClearRun() | ||
| { | ||
| if (GameSaveObjects.ContainsKey(DefaultRunSaveHeader)) | ||
| { | ||
| GameSaveObjects.Remove(DefaultRunSaveHeader); | ||
| SaveToFile(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for save operation.
The SaveToFile() call could fail silently, leaving the system in an inconsistent state where the run is cleared in memory but not on disk.
public static void ClearRun()
{
if (GameSaveObjects.ContainsKey(DefaultRunSaveHeader))
{
GameSaveObjects.Remove(DefaultRunSaveHeader);
- SaveToFile();
+ if (!SaveToFile())
+ {
+ GD.PrintErr("Failed to save after clearing run data");
+ }
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In Globals/Savekeeper.cs around lines 271 to 278, the SaveToFile() method call
after removing the run from GameSaveObjects lacks error handling, which could
cause silent failures. Wrap the SaveToFile() call in a try-catch block to catch
any exceptions, log or handle the error appropriately, and ensure the system
remains consistent if saving fails.
| FileAccess file = FileAccess.Open( | ||
| SaveFileDirectory + "/" + savePath, | ||
| FileAccess.ModeFlags.Write | ||
| ); | ||
| if (file == null) | ||
| return false; | ||
|
|
||
| foreach ((string key, string value) in GameSaveObjects) | ||
| file.StoreLine(key + Delimiter + value); | ||
| file.Close(); |
There was a problem hiding this comment.
Use proper resource management and path handling.
The file handling needs improvement:
- Use
usingstatement or try-finally to ensure file closure - Use
Path.Combineinstead of string concatenation for paths - Add error handling for file operations
- FileAccess file = FileAccess.Open(
- SaveFileDirectory + "/" + savePath,
- FileAccess.ModeFlags.Write
- );
- if (file == null)
- return false;
-
- foreach ((string key, string value) in GameSaveObjects)
- file.StoreLine(key + Delimiter + value);
- file.Close();
+ string fullPath = Path.Combine(SaveFileDirectory, savePath);
+ using (FileAccess file = FileAccess.Open(fullPath, FileAccess.ModeFlags.Write))
+ {
+ if (file == null)
+ return false;
+
+ foreach ((string key, string value) in GameSaveObjects)
+ file.StoreLine(key + Delimiter + value);
+ }📝 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.
| FileAccess file = FileAccess.Open( | |
| SaveFileDirectory + "/" + savePath, | |
| FileAccess.ModeFlags.Write | |
| ); | |
| if (file == null) | |
| return false; | |
| foreach ((string key, string value) in GameSaveObjects) | |
| file.StoreLine(key + Delimiter + value); | |
| file.Close(); | |
| string fullPath = Path.Combine(SaveFileDirectory, savePath); | |
| using (FileAccess file = FileAccess.Open(fullPath, FileAccess.ModeFlags.Write)) | |
| { | |
| if (file == null) | |
| return false; | |
| foreach ((string key, string value) in GameSaveObjects) | |
| file.StoreLine(key + Delimiter + value); | |
| } |
🤖 Prompt for AI Agents
In Globals/Savekeeper.cs around lines 57 to 66, improve file handling by
replacing string concatenation with Path.Combine to build the file path, wrap
the file opening and writing logic inside a using statement or try-finally block
to ensure the file is properly closed even if exceptions occur, and add
try-catch blocks to handle potential exceptions during file operations,
returning false or handling errors appropriately.
| FileAccess file = FileAccess.Open( | ||
| SaveFileDirectory + "/" + savePath, | ||
| FileAccess.ModeFlags.Read | ||
| ); | ||
| if (file == null) | ||
| return false; | ||
|
|
||
| HandlePrevSave(); | ||
|
|
||
| while (file.GetPosition() < file.GetLength()) | ||
| { | ||
| string line = file.GetLine().Trim(); | ||
| int idx = line.IndexOf(Delimiter); | ||
| if (idx == -1) | ||
| continue; | ||
|
|
||
| string key = line.Substring(0, idx); | ||
| string value = SanitizeSaveString(line.Substring(idx)); | ||
| if (value == null) | ||
| continue; | ||
|
|
||
| GameSaveObjects[key] = value; | ||
| } |
There was a problem hiding this comment.
Fix critical parsing bug and improve resource management.
Multiple issues in the load method:
- Line 93 has a bug -
line.Substring(idx)includes the delimiter character - Missing proper resource disposal
- Path concatenation should use
Path.Combine
- FileAccess file = FileAccess.Open(
- SaveFileDirectory + "/" + savePath,
- FileAccess.ModeFlags.Read
- );
- if (file == null)
- return false;
-
- HandlePrevSave();
-
- while (file.GetPosition() < file.GetLength())
- {
- string line = file.GetLine().Trim();
- int idx = line.IndexOf(Delimiter);
- if (idx == -1)
- continue;
-
- string key = line.Substring(0, idx);
- string value = SanitizeSaveString(line.Substring(idx));
- if (value == null)
- continue;
-
- GameSaveObjects[key] = value;
- }
+ string fullPath = Path.Combine(SaveFileDirectory, savePath);
+ using (FileAccess file = FileAccess.Open(fullPath, FileAccess.ModeFlags.Read))
+ {
+ if (file == null)
+ return false;
+
+ HandlePrevSave();
+
+ while (file.GetPosition() < file.GetLength())
+ {
+ string line = file.GetLine().Trim();
+ int idx = line.IndexOf(Delimiter);
+ if (idx == -1)
+ continue;
+
+ string key = line.Substring(0, idx);
+ string value = SanitizeSaveString(line.Substring(idx + 1));
+ if (value == null)
+ continue;
+
+ GameSaveObjects[key] = value;
+ }
+ }📝 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.
| FileAccess file = FileAccess.Open( | |
| SaveFileDirectory + "/" + savePath, | |
| FileAccess.ModeFlags.Read | |
| ); | |
| if (file == null) | |
| return false; | |
| HandlePrevSave(); | |
| while (file.GetPosition() < file.GetLength()) | |
| { | |
| string line = file.GetLine().Trim(); | |
| int idx = line.IndexOf(Delimiter); | |
| if (idx == -1) | |
| continue; | |
| string key = line.Substring(0, idx); | |
| string value = SanitizeSaveString(line.Substring(idx)); | |
| if (value == null) | |
| continue; | |
| GameSaveObjects[key] = value; | |
| } | |
| string fullPath = Path.Combine(SaveFileDirectory, savePath); | |
| using (FileAccess file = FileAccess.Open(fullPath, FileAccess.ModeFlags.Read)) | |
| { | |
| if (file == null) | |
| return false; | |
| HandlePrevSave(); | |
| while (file.GetPosition() < file.GetLength()) | |
| { | |
| string line = file.GetLine().Trim(); | |
| int idx = line.IndexOf(Delimiter); | |
| if (idx == -1) | |
| continue; | |
| string key = line.Substring(0, idx); | |
| string value = SanitizeSaveString(line.Substring(idx + 1)); | |
| if (value == null) | |
| continue; | |
| GameSaveObjects[key] = value; | |
| } | |
| } |
🤖 Prompt for AI Agents
In Globals/Savekeeper.cs between lines 76 and 98, fix the parsing bug by
changing line.Substring(idx) to line.Substring(idx + 1) to exclude the delimiter
character from the value. Replace string concatenation of file paths with
Path.Combine to correctly handle file paths across platforms. Also, ensure the
FileAccess object is properly disposed after use, preferably by wrapping it in a
using statement or explicitly calling Dispose to manage resources correctly.
Add Enemy Descriptions
Reformat save files (this breaks old saves: lose current run, lose tutorial win progress, and stage 2 win progress)
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Refactor
Assets