Sprint 3 #84
Sprint 3 #84
Conversation
New background and UI frame More clear note sprites Added note outlines for sight readability
player also starts with 1 copy of this note in their queue
Created puppet scenes Added WIP map icons Created BattleConfig
Created puppet scenes Added WIP map icons Created BattleConfig
steals 1 health from enemy
Uses the art from the node queue, should probably change to make more visible on the notes, but for now it works.
Game beautification
Moved note cost note side Prevented charge bar from overcapping.
added new placed note types, and art to the notes
Grab texture from notes
Moved song data into config object in StageProducer for persistant and progressable map Moved menu inputs into abstracted node for portability Added chest scene, currently gives relic selection
+Small tweak to room picking on map generation
Also fixed readme attribution list
These should probably rotate, but I cant find a way to snap it to the 4 cardinal directions
Win lose feedback
Menu music and title screen finishing touches
Finalize title screen design and add music that plays across scenes
Enemies can have effects Relics can scale values
Enemy effects and Scalable relics
And move directory location
I think everything should be steady enough that they can be removed.
WalkthroughThis pull request introduces extensive changes across the project. The update includes modifications to version control files and project configuration, such as .gitignore and csproj adjustments, alongside removal and addition of several audio import configuration files. New classes and methods for MIDI handling, song data, note and relic management have been implemented, while many UI and scene files have been added or updated with enhanced layouts, particle effects, control mapping, and enemy behaviors. Numerous texture asset import configurations, shader files, and remapping settings have been revised to improve visual appearance and gameplay transitions. Changes
Sequence Diagram(s)sequenceDiagram
participant SP as StageProducer
participant RM as Room Manager
participant BC as BattleConfig
participant SC as Scene Controller
SP->>SP: TransitionStage(nextStage, nextRoomIdx)
SP->>BC: MakeConfig(nextStage, nextRoomIdx)
SP->>RM: Update current room state
SP->>SC: Trigger scene transition (Battle, Chest, or Title)
sequenceDiagram
participant P as Player
participant BD as BattleDirector
participant NP as NotePlacementBar
participant C as Conductor
P->>BD: Initiate note input
BD->>NP: Attempt to add note (PlayerAddNote)
NP->>C: Process note and combo effects
C-->>BD: Return timing/feedback info
BD->>P: Display battle outcome (Win/Lose/Rewards)
Possibly related PRs
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
There was a problem hiding this comment.
Technically doesn't have to be here until we have actual midi playback implemented but its fine
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 28
🔭 Outside diff range comments (1)
scenes/Puppets/scripts/PlayerStats.cs (1)
10-16: 🛠️ Refactor suggestionUsing hardcoded indices creates fragile code.
The initialization of
CurNotesuses hardcoded indices fromScribe.NoteDictionary, which could break if the dictionary structure changes.- public Note[] CurNotes = new Note[] - { - Scribe.NoteDictionary[1].Clone(), - Scribe.NoteDictionary[1].Clone(), - Scribe.NoteDictionary[2].Clone(), - Scribe.NoteDictionary[3].Clone(), - }; + public Note[] CurNotes = new Note[] + { + Scribe.GetNoteByKey("basic").Clone(), + Scribe.GetNoteByKey("basic").Clone(), + Scribe.GetNoteByKey("medium").Clone(), + Scribe.GetNoteByKey("advanced").Clone(), + };This requires implementing a
GetNoteByKeymethod in theScribeclass that maps logical names to dictionary indices.
🧹 Nitpick comments (114)
scenes/SceneTransitions/assets/2d_lights_and_shadows_neutral_point_light.webp.import (1)
1-34: The import configuration appears appropriate for a standard 2D light texture.This configuration follows standard Godot import settings for a 2D texture asset. The settings are reasonable for general use cases.
Since this is specifically a point light texture that likely contains gradients, you might consider these optional improvements:
-mipmaps/generate=false +mipmaps/generate=trueEnabling mipmaps could improve the visual quality when the light is rendered at different scales/distances, preventing potential flickering or aliasing.
scenes/SceneTransitions/assets/TitleFont.gdshader (1)
32-42: Consider cleaning up commented code or documenting its purpose.There's a large block of commented code that appears to be an alternative implementation. Consider either removing it if it's no longer needed or adding a comment explaining why it's kept for future reference.
scenes/UI/Pause.tscn (1)
20-20: Improved pause menu visibility with increased opacity.The alpha value of the background color has been significantly increased from 0.467 to 0.855, making the pause menu much more visible and providing better contrast for the menu options.
Consider using a slightly darker background color (around 0.15-0.18 instead of 0.24) to provide even better contrast for the menu text. The higher opacity is good, but ensuring appropriate contrast ratios will improve accessibility.
scenes/Puppets/Enemies/EnemyPuppet.cs (3)
5-14: Base enemy class looks good but needs TODO resolution.The
EnemyPuppetbase class establishes a solid foundation for enemy implementations with battle event handling, but the TODO comment should be addressed before finalizing.The class structure is clean and follows good inheritance practices by extending from
PuppetTemplate. The virtual method allows for polymorphic behavior in derived classes.Would you like me to help generate a list of common requirements for enemy entities based on typical game patterns? This could help address the TODO comment and ensure consistency across enemy implementations.
8-8: Consider documenting the purpose of_battleEvents.Adding a brief XML comment explaining the purpose and usage of
_battleEventswould improve code readability and maintainability.- protected EnemyEffect[] _battleEvents = Array.Empty<EnemyEffect>(); + /// <summary> + /// Array of battle effects this enemy can trigger during combat. + /// Derived classes should initialize this in their constructor or _Ready method. + /// </summary> + protected EnemyEffect[] _battleEvents = Array.Empty<EnemyEffect>();
10-13: Consider adding documentation for theGetBattleEventsmethod.XML documentation for the method would improve API discoverability and usage.
+/// <summary> +/// Returns all battle events this enemy can trigger. +/// </summary> +/// <returns>Array of enemy effects that can be triggered in battle</returns> public virtual EnemyEffect[] GetBattleEvents() { return _battleEvents; }scenes/Puppets/Enemies/TheGWS/P_TheGWS.cs (2)
4-17: Animation setup looks good but consider encapsulating the tween logic.The floating animation implementation works well but could be refactored for better maintainability.
Consider extracting the tween setup into a dedicated method for better readability and potential reuse:
public partial class P_TheGWS : EnemyPuppet { public override void _Ready() { base._Ready(); - var enemTween = CreateTween(); - enemTween.TweenProperty(Sprite, "position", Vector2.Down * 10, 3f).AsRelative(); - enemTween.TweenProperty(Sprite, "position", Vector2.Up * 10, 3f).AsRelative(); - enemTween.SetTrans(Tween.TransitionType.Quad); - enemTween.SetEase(Tween.EaseType.InOut); - enemTween.SetLoops(); - enemTween.Play(); + SetupFloatingAnimation(); } + + private void SetupFloatingAnimation() + { + var enemTween = CreateTween(); + enemTween.TweenProperty(Sprite, "position", Vector2.Down * 10, 3f).AsRelative(); + enemTween.TweenProperty(Sprite, "position", Vector2.Up * 10, 3f).AsRelative(); + enemTween.SetTrans(Tween.TransitionType.Quad); + enemTween.SetEase(Tween.EaseType.InOut); + enemTween.SetLoops(); + enemTween.Play(); + } }
9-15: Consider using property access instead of string literals.Using string literals for property names is error-prone and prevents compile-time checking.
- enemTween.TweenProperty(Sprite, "position", Vector2.Down * 10, 3f).AsRelative(); - enemTween.TweenProperty(Sprite, "position", Vector2.Up * 10, 3f).AsRelative(); + enemTween.TweenProperty(Sprite, nameof(Sprite2D.Position), Vector2.Down * 10, 3f).AsRelative(); + enemTween.TweenProperty(Sprite, nameof(Sprite2D.Position), Vector2.Up * 10, 3f).AsRelative();This assumes
Spriteis of typeSprite2D. Adjust the type name as needed for your specific implementation.scenes/Puppets/Enemies/BossBlood/P_BossBlood.cs (3)
11-17: Animation setup is good but should be refactored similar to other enemies.The animation setup is similar to other enemy classes and would benefit from the same refactoring.
Consider extracting the tween setup into a dedicated method for consistency with other enemies and improved code organization:
public override void _Ready() { base._Ready(); - var enemTween = CreateTween(); - enemTween.TweenProperty(Sprite, "position", Vector2.Down * 5, 1f).AsRelative(); - enemTween.TweenProperty(Sprite, "position", Vector2.Up * 5, 1f).AsRelative(); - enemTween.SetTrans(Tween.TransitionType.Spring); - enemTween.SetEase(Tween.EaseType.In); - enemTween.SetLoops(); - enemTween.Play(); + SetupBobbingAnimation(); _battleEvents = new EnemyEffect[] { // ... }; } + private void SetupBobbingAnimation() + { + var enemTween = CreateTween(); + enemTween.TweenProperty(Sprite, nameof(Sprite2D.Position), Vector2.Down * 5, 1f).AsRelative(); + enemTween.TweenProperty(Sprite, nameof(Sprite2D.Position), Vector2.Up * 5, 1f).AsRelative(); + enemTween.SetTrans(Tween.TransitionType.Spring); + enemTween.SetEase(Tween.EaseType.In); + enemTween.SetLoops(); + enemTween.Play(); + }
19-30: Self-healing mechanism is implemented well but needs documentation.The battle event for self-healing is implemented correctly, but adding comments would improve clarity.
Consider adding a comment to explain the healing mechanism:
_battleEvents = new EnemyEffect[] { + // Self-healing effect: Heals 5 HP on each animation loop new EnemyEffect( this, BattleEffectTrigger.OnLoop, 5, (director, eff, val) => { eff.Owner.Heal(val); } ), };
21-29: Consider extracting the lambda to a named method.The lambda function used for the healing effect could be replaced with a named method for better readability and testability.
_battleEvents = new EnemyEffect[] { new EnemyEffect( this, BattleEffectTrigger.OnLoop, 5, - (director, eff, val) => - { - eff.Owner.Heal(val); - } + HealSelf ), }; + + private void HealSelf(BattleDirector director, EnemyEffect effect, int value) + { + effect.Owner.Heal(value); + }scenes/Puppets/Enemies/Parasifly/P_Parasifly.cs (1)
1-17: Good implementation of the floating animation for the Parasifly enemy.The tween animation for the Parasifly enemy creates a nice floating effect with the sprite moving up and down. The tween configuration with linear transition and infinite looping provides a smooth, continuous animation.
Consider adding null checking for the Sprite property and making the animation parameters configurable through exported variables:
public partial class P_Parasifly : EnemyPuppet { + [Export] + public float FloatDistance = 2.0f; + + [Export] + public float FloatDuration = 2.0f; + public override void _Ready() { base._Ready(); + if (Sprite == null) + { + GD.PrintErr("Sprite is null in P_Parasifly"); + return; + } var enemTween = CreateTween(); - enemTween.TweenProperty(Sprite, "position", Vector2.Down * 2, 2f).AsRelative(); - enemTween.TweenProperty(Sprite, "position", Vector2.Up * 2, 2f).AsRelative(); + enemTween.TweenProperty(Sprite, "position", Vector2.Down * FloatDistance, FloatDuration).AsRelative(); + enemTween.TweenProperty(Sprite, "position", Vector2.Up * FloatDistance, FloatDuration).AsRelative(); enemTween.SetTrans(Tween.TransitionType.Linear); enemTween.SetEase(Tween.EaseType.In); enemTween.SetLoops(); enemTween.Play(); } }scenes/SceneTransitions/scripts/TitleScreen.cs (1)
14-22: Good implementation of the title light animation.The tween implementation creates a nice pulsing effect for the title light, transitioning between energy levels of 2 and 7 over 5 seconds each. Setting the tween to loop infinitely ensures the animation continues throughout the title screen's lifetime.
Consider adding null checking for TextLight and making the animation parameters configurable through exported variables:
public partial class TitleScreen : Control { [Export] public PointLight2D TextLight; + [Export] + public float MinLightEnergy = 2.0f; + + [Export] + public float MaxLightEnergy = 7.0f; + + [Export] + public float LightTransitionDuration = 5.0f; // ... private void TweenLight() { + if (TextLight == null) + { + GD.PrintErr("TextLight is null in TitleScreen"); + return; + } var tween = CreateTween(); tween.SetTrans(Tween.TransitionType.Linear); - tween.TweenProperty(TextLight, "energy", 7, 5f); - tween.TweenProperty(TextLight, "energy", 2, 5f); + tween.TweenProperty(TextLight, "energy", MaxLightEnergy, LightTransitionDuration); + tween.TweenProperty(TextLight, "energy", MinLightEnergy, LightTransitionDuration); tween.SetLoops(-1); tween.Play(); } }scenes/UI/scripts/MenuModule.cs (1)
4-23: Consider extracting menu creation logic into separate methods.The current implementation has duplicated logic for creating menus. This could be extracted into separate methods to improve readability and maintainability.
Refactor the code to extract menu creation logic:
public partial class MenuModule : CanvasLayer { public override void _Input(InputEvent @event) { if (@event.IsActionPressed("Pause")) { - var pauseMenu = GD.Load<PackedScene>("res://scenes/UI/Pause.tscn"); - AddChild(pauseMenu.Instantiate()); - GetTree().Paused = true; + OpenPauseMenu(); } if (@event.IsActionPressed("Inventory")) { - var invenMenu = GD.Load<PackedScene>("res://scenes/UI/inventory.tscn") - .Instantiate<Inventory>(); - AddChild(invenMenu); - invenMenu.Display(StageProducer.PlayerStats ?? new PlayerStats()); //For now work around for testing - GetTree().Paused = true; + OpenInventoryMenu(); } } + + private void OpenPauseMenu() + { + // Check if a pause menu already exists + foreach (Node child in GetChildren()) + { + if (child.Name.Contains("Pause")) + { + return; // Don't create another pause menu + } + } + + var pauseScene = GD.Load<PackedScene>("res://scenes/UI/Pause.tscn"); + if (pauseScene == null) + { + GD.PrintErr("Failed to load pause scene"); + return; + } + + AddChild(pauseScene.Instantiate()); + GetTree().Paused = true; + } + + private void OpenInventoryMenu() + { + // Check if an inventory already exists + foreach (Node child in GetChildren()) + { + if (child is Inventory) + { + return; // Don't create another inventory + } + } + + var inventoryScene = GD.Load<PackedScene>("res://scenes/UI/inventory.tscn"); + if (inventoryScene == null) + { + GD.PrintErr("Failed to load inventory scene"); + return; + } + + var invenMenu = inventoryScene.Instantiate<Inventory>(); + AddChild(invenMenu); + + // Use the null-coalescing operator to handle the null case + PlayerStats stats = StageProducer.PlayerStats ?? new PlayerStats(); + invenMenu.Display(stats); // For now work around for testing + GetTree().Paused = true; + } }scenes/Puppets/Enemies/EnemyEffect.cs (3)
5-11: Consider improving encapsulation for class properties.The class design could benefit from better encapsulation practices:
Triggeris defined with private access but has public getters/setters, making it effectively public. Consider making it a proper public property or keeping it truly private.OwnerandValueare public fields rather than properties, which limits your ability to add validation or change the implementation later.- private BattleEffectTrigger Trigger { get; set; } - public EnemyPuppet Owner; - private int _baseValue; - public int Value; + public BattleEffectTrigger Trigger { get; private set; } + public EnemyPuppet Owner { get; private set; } + private int _baseValue; + public int Value { get; set; }
27-30: Add null check for delegate invocation.The
_onEnemyEffectdelegate is invoked without a null check, which could lead to aNullReferenceExceptionif the delegate was never set or became null after initialization.public void OnTrigger(BattleDirector battleDirector) { - _onEnemyEffect(battleDirector, this, Value); + _onEnemyEffect?.Invoke(battleDirector, this, Value); }
32-35: Redundant getter method.Since
GetTrigger()simply returns theTriggerproperty, consider makingTriggerpublic with a private setter or removing this method if the IBattleEvent interface allows it.scenes/NoteManager/scripts/NoteChecker.cs (1)
9-9: Use properties instead of public fields for better encapsulation.Public fields limit your ability to add validation, change implementation details, or intercept access. Consider using a property for
Particlesinstead.- public HitParticles Particles; + public HitParticles Particles { get; private set; }scenes/ChartViewport/hit_particles.tscn (1)
8-8: Consider increasing the default particle amount.The current amount is set to 1, which might not create a visually impactful effect. Though this is overridden in the
Emitmethod, a slightly higher default value might be beneficial for testing in the editor.- amount = 1 + amount = 10scenes/UI/scripts/EndScreen.cs (4)
7-10: Consider adding defensive checks for array access.The
buttonsarray is referenced directly in_Ready()without any null or bounds checking. This could lead to runtime exceptions if the array is not properly initialized in the editor or has fewer than 3 elements.[Export] private Button[] buttons; [Export] public Label TopLabel; public override void _Ready() { + if (buttons == null || buttons.Length < 3) + { + GD.PrintErr("EndScreen: buttons array not properly initialized or has fewer than 3 elements."); + return; + } buttons[0].Pressed += Restart; buttons[1].Pressed += QuitToMainMenu; buttons[2].Pressed += Quit; buttons[0].GrabFocus(); }
20-25: Add error handling for StageProducer access.The code directly accesses the StageProducer without checking if it exists, which could lead to a NullReferenceException if the node is not found.
private void Restart() { GetTree().Paused = false; StageProducer.IsInitialized = false; - GetNode<StageProducer>("/root/StageProducer").TransitionStage(Stages.Map); + var stageProducer = GetNode<StageProducer>("/root/StageProducer"); + if (stageProducer != null) + { + stageProducer.TransitionStage(Stages.Map); + } + else + { + GD.PrintErr("EndScreen: Failed to find StageProducer node."); + } }
27-30: Add error handling for StageProducer access.Similar to the Restart method, add error handling when accessing the StageProducer.
private void Quit() { - GetNode<StageProducer>("/root/StageProducer").TransitionStage(Stages.Quit); + var stageProducer = GetNode<StageProducer>("/root/StageProducer"); + if (stageProducer != null) + { + stageProducer.TransitionStage(Stages.Quit); + } + else + { + GD.PrintErr("EndScreen: Failed to find StageProducer node."); + } }
32-36: Add error handling for StageProducer access.Apply the same error handling pattern to the QuitToMainMenu method.
private void QuitToMainMenu() { GetTree().Paused = false; - GetNode<StageProducer>("/root/StageProducer").TransitionStage(Stages.Title); + var stageProducer = GetNode<StageProducer>("/root/StageProducer"); + if (stageProducer != null) + { + stageProducer.TransitionStage(Stages.Title); + } + else + { + GD.PrintErr("EndScreen: Failed to find StageProducer node."); + } }scenes/SceneTransitions/assets/transparentStars.gdshader (1)
9-20: Optimize shader performance and add safeguards.The shader can be optimized by reducing redundant random function calls and adding protection against potential division by zero.
void fragment() { float star_intensity = 0.0; float alpha = 0.0; - if (rand(SCREEN_UV.xy / 20.0) > 0.996) { - float r = rand(SCREEN_UV.xy); + vec2 scaled_uv = SCREEN_UV.xy / 20.0; + float random_check = rand(scaled_uv); + + if (random_check > 0.996) { + float r = rand(SCREEN_UV.xy); + // Ensure time_scale is not too close to zero to prevent potential issues + float safe_time_scale = max(0.0001, time_scale); - star_intensity = r * (0.85 * sin((TIME * time_scale) * (r * 5.0) + 720.0 * r) + 0.95); + star_intensity = r * (0.85 * sin((TIME * safe_time_scale) * (r * 5.0) + 720.0 * r) + 0.95); alpha = star_intensity; } COLOR = vec4(vec3(star_intensity), alpha); }Classes/MidiMaestro/SongTemplate.cs (2)
3-9: Improve encapsulation with properties instead of public fields.Using public fields reduces encapsulation and can lead to unexpected behavior if modified directly. Consider using properties to provide controlled access to the class data.
- public partial class SongTemplate + public class SongTemplate { - public string Name; - public string AudioLocation; - public string MIDILocation; - public string EnemyScenePath; - public SongData SongData; + public string Name { get; set; } + public string AudioLocation { get; set; } + public string MIDILocation { get; set; } + public string EnemyScenePath { get; set; } + public SongData SongData { get; set; }
11-24: Add path validation to ensure resources exist.The constructor accepts paths to audio, MIDI, and enemy scene resources without validation. Adding checks can help prevent issues at runtime.
public SongTemplate( SongData songData, string name = "", string audioLocation = "", string midiLocation = "", string enemyScenePath = "" ) { + // Basic validation + if (string.IsNullOrEmpty(name)) + { + GD.PrintWarning("SongTemplate created with empty name"); + } + Name = name; AudioLocation = audioLocation; MIDILocation = midiLocation; SongData = songData; EnemyScenePath = enemyScenePath; + // Log if paths are not provided + if (string.IsNullOrEmpty(audioLocation)) + { + GD.PrintWarning($"SongTemplate '{name}': No audio location provided"); + } + if (string.IsNullOrEmpty(midiLocation)) + { + GD.PrintWarning($"SongTemplate '{name}': No MIDI location provided"); + } + if (string.IsNullOrEmpty(enemyScenePath)) + { + GD.PrintWarning($"SongTemplate '{name}': No enemy scene path provided"); + } }scenes/ChartViewport/scripts/Loopable.cs (4)
4-7: Improve class documentation clarity.The class documentation includes "WIP" which indicates this is still under development. Consider adding more details about the intended behavior and completion status for better understanding by other developers.
-/** - * @class Loopable - * @brief A general class fo textures on the chart which should have a position at which point they loop. WIP - */ +/** + * @class Loopable + * @brief A general class for textures on the chart which loop at a specific position. + * @details Handles the positioning and looping of chart textures based on time progression and chart length. + */
8-11: Property usage requires more context.The
LoopOffsetproperty has a comment that provides some context about its usage, but it could be clearer about the units and exact purpose.- [Export] - public float LoopOffset = 700f; //px Pos to loop or do something at. + [Export] + /// <summary> + /// The horizontal position (in pixels) at which the texture should loop. + /// </summary> + public float LoopOffset = 700f;
21-24: Complex position calculation needs better documentation.The position calculation uses a complex formula with minimal explanation. The Desmos link is helpful but inline documentation would make maintenance easier.
- newPos.X = - (float)( //Yes I know. https://www.desmos.com/calculator/fkmoqi50ee - (TimeKeeper.PosMod(-relativePosition - interval / 2, interval) - interval / 2) / 2 - ) + LoopOffset; + // Calculate the looping position using a periodic function + // Formula visualized at: https://www.desmos.com/calculator/fkmoqi50ee + // This creates a smooth loop effect by: + // 1. Calculating the relative position in the chart + // 2. Using modular arithmetic to wrap around when exceeding bounds + // 3. Adjusting by interval/2 to center the effect + // 4. Scaling by 0.5 to control the movement speed + newPos.X = + (float)( + (TimeKeeper.PosMod(-relativePosition - interval / 2, interval) - interval / 2) / 2 + ) + LoopOffset;
25-26: Improve NaN handling with logging.The current code checks for NaN values but doesn't provide any feedback when they occur, making debugging difficult.
- if (!float.IsNaN(newPos.X)) - Position = newPos; + if (float.IsNaN(newPos.X)) + { + GD.PrintErr("Loopable calculated NaN position. Check TimeKeeper values."); + return; + } + Position = newPos;scenes/Puppets/scripts/PlayerStats.cs (3)
21-31: Inconsistent method implementation style.The
AddRelicmethod applies effects on pickup, but theAddNotemethod doesn't have similar logic, leading to inconsistent behavior when adding items.public void AddRelic(RelicTemplate relic) { foreach (RelicEffect effect in relic.Effects) { if (effect.GetTrigger() == BattleEffectTrigger.OnPickup) { effect.OnTrigger(null); } } CurRelics = CurRelics.Append(relic).ToArray(); }Consider documenting why notes don't need similar pickup effects, or implement them if they should have such behavior.
33-36: Lack of validation in the AddNote method.The
AddNotemethod adds notes without any validation or limit checking, which could lead to unexpected behavior if too many notes are added.public void AddNote(Note nSelection) { + // Consider adding validation here, e.g., maximum number of notes + if (CurNotes.Length >= 10) // Example maximum + { + GD.Print("Maximum number of notes reached"); + return; + } + CurNotes = CurNotes.Append(nSelection).ToArray(); }
33-36: Performance concern with array manipulation.The current implementation creates a new array on each addition, which can be inefficient for frequent operations. Consider using a List instead.
- public Note[] CurNotes = new Note[] + private List<Note> _curNotesList = new List<Note> { Scribe.NoteDictionary[1].Clone(), Scribe.NoteDictionary[1].Clone(), Scribe.NoteDictionary[2].Clone(), Scribe.NoteDictionary[3].Clone(), }; + public Note[] CurNotes => _curNotesList.ToArray(); public void AddNote(Note nSelection) { - CurNotes = CurNotes.Append(nSelection).ToArray(); + _curNotesList.Add(nSelection); }scenes/Puppets/PlayerPuppet.tscn (1)
11-11: Hardcoded StartPos value may limit reusability.The
StartPosproperty is set to a fixed Vector2(158, 125), which may make the scene less adaptable to different screen sizes or layouts.Consider making this an exported variable that can be configured in the editor or calculating it based on screen dimensions.
scenes/SceneTransitions/assets/font.TTF.import (1)
31-37: Font preload configuration has empty character arrays.The preload configuration has empty character and glyph arrays, which might cause suboptimal font loading if specific characters are needed.
Consider specifying the commonly used characters in your game to optimize font loading:
preload=[{ -"chars": [], +"chars": [" ", "!", "\"", "#", "$", "%", "&", "'", "(", ")", "*", "+", ",", "-", ".", "/", "0", "1", "2", "3", "4", "5", "6", "7", "8", "9", ":", ";", "<", "=", ">", "?", "@", "A", "B", "C", "D", "E", "F", "G", "H", "I", "J", "K", "L", "M", "N", "O", "P", "Q", "R", "S", "T", "U", "V", "W", "X", "Y", "Z", "[", "\\", "]", "^", "_", "`", "a", "b", "c", "d", "e", "f", "g", "h", "i", "j", "k", "l", "m", "n", "o", "p", "q", "r", "s", "t", "u", "v", "w", "x", "y", "z", "{", "|", "}", "~"], "glyphs": [], "name": "New Configuration", "size": Vector2i(16, 0), "variation_embolden": 0.0 }]scenes/Puppets/Enemies/BossBlood/Boss1.tscn (3)
29-30: Consider using relative positioning instead of fixed coordinates.The StartPos vector uses absolute coordinates (500, 125), which may not work well across different screen resolutions or aspect ratios. This could cause positioning issues when the game runs on different devices.
Consider using relative positioning based on screen dimensions or viewport size:
-StartPos = Vector2(500, 125) +StartPos = Vector2(0.5 * GetViewportRect().Size.X, 0.25 * GetViewportRect().Size.Y)
8-16: Optimize particle emission settings for better performance.The particle system uses a high emission_point_count (1078) with fixed-size particles, which could impact performance on lower-end devices.
Consider reducing the emission point count and varying the particle scale for a more natural effect:
-emission_point_count = 1078 +emission_point_count = 250 -scale_min = 4.0 -scale_max = 4.0 +scale_min = 3.0 +scale_max = 5.0
36-43: Add collision detection for the particle system.The blood particles currently don't have any collision detection. Adding collision could enhance visual realism by making blood particles interact with surfaces.
Consider adding collision properties to the particle system:
[node name="Blood Particles" type="GPUParticles2D" parent="Sprite"] z_index = -1 position = Vector2(-32, -32) amount = 4 process_material = SubResource("ParticleProcessMaterial_3jiu6") texture = SubResource("GradientTexture1D_16sp0") lifetime = 2.0 trail_lifetime = 10.0 +collision_base_size = 2.0 +collide_with_areas = trueREADME.md (1)
18-26: Improve attribution section formatting.The "Attributions:" heading has a trailing colon which violates markdown best practices.
Consider this change to improve formatting:
-#### Attributions: +#### AttributionsThe organization of attributions into Music and Images categories is clear and well-structured.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
18-18: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
scenes/Puppets/Enemies/Parasifly/Parasifly.tscn (2)
11-11: Consider using relative positioning instead of fixed coordinates.Similar to the Boss1 scene, the StartPos vector uses absolute coordinates (500, 125), which may not work well across different screen resolutions or aspect ratios.
Consider using relative positioning based on screen dimensions or viewport size:
-StartPos = Vector2(500, 125) +StartPos = Vector2(0.5 * GetViewportRect().Size.X, 0.25 * GetViewportRect().Size.Y)
17-21: Consider adding animations for the health bar.The health bar is statically positioned, but adding animations when health changes could improve player feedback.
In the P_Parasifly.cs script, consider adding animation for health changes:
// Add this to the script private void UpdateHealth(float newValue, float maxValue) { _healthBar.Value = newValue; var tween = CreateTween(); tween.TweenProperty(_healthBar, "modulate", new Color(1, 0, 0, 1), 0.2f); tween.TweenProperty(_healthBar, "modulate", new Color(1, 1, 1, 1), 0.2f); }scenes/UI/EndScreen.tscn (2)
52-56: Add accessibility features to buttons.The buttons have proper focus navigation, but could benefit from additional accessibility features like tooltips and keyboard shortcuts.
Enhance button accessibility with tooltips and shortcuts:
[node name="Restart" type="Button" parent="MarginContainer2/VBoxContainer/MarginContainer"] layout_mode = 2 focus_neighbor_top = NodePath("../../MarginContainer3/Quit") focus_neighbor_bottom = NodePath("../../MarginContainer2/Title") text = "Restart" +tooltip_text = "Start a new game from the beginning" +shortcut_in_tooltip = true
34-39: Consider adding localization support for UI text.The "Game Over!" text is hardcoded, which makes localization difficult. Consider implementing a localization system for better international support.
Instead of hardcoded text, use a translation key:
[node name="TopLabel" type="Label" parent="MarginContainer2/VBoxContainer"] unique_name_in_owner = true layout_mode = 2 theme_override_font_sizes/font_size = 20 -text = "Game Over!" +text = "TR_GAME_OVER" horizontal_alignment = 1Then implement a translation system in the EndScreen.cs script to replace "TR_GAME_OVER" with the localized string.
scenes/Remapping/ControlSchemes.cs (4)
4-41: Consider making dictionaries read-only to prevent accidental modifications.The
Schemesdictionary contains control scheme mappings that should likely remain constant during runtime. Making it read-only would prevent accidental modifications that could lead to unexpected behavior.Consider changing the dictionary declaration to be read-only:
- public static Dictionary<string, Dictionary<string, string>> Schemes = new Dictionary< + public static readonly Dictionary<string, Dictionary<string, string>> Schemes = new Dictionary<
43-78: Same pattern should be applied to SpriteMappings dictionary.Similar to the Schemes dictionary, the SpriteMappings dictionary should also be made read-only to prevent accidental modifications during runtime.
Consider making this dictionary read-only as well:
- public static Dictionary<string, Dictionary<string, string>> SpriteMappings = new Dictionary< + public static readonly Dictionary<string, Dictionary<string, string>> SpriteMappings = new Dictionary<
4-79: Add XML documentation for better code clarity.Adding XML documentation to public classes and members would improve code readability and provide better IDE integration for other developers.
Consider adding documentation comments:
+/// <summary> +/// Provides predefined control schemes and their corresponding sprite mappings. +/// </summary> public static class ControlSchemes { + /// <summary> + /// Maps control scheme names to their key mappings for directional inputs. + /// </summary> public static Dictionary<string, Dictionary<string, string>> Schemes = new Dictionary<
4-79: Add validation method to verify control scheme integrity.There's currently no way to verify that a scheme contains all the necessary mappings. A validation method would be helpful to ensure that all schemes are properly configured.
Consider adding a validation method:
/// <summary> /// Validates that the specified scheme contains all required directional mappings. /// </summary> /// <param name="schemeName">The name of the scheme to validate.</param> /// <returns>True if the scheme is valid, false otherwise.</returns> public static bool ValidateScheme(string schemeName) { if (!Schemes.ContainsKey(schemeName)) return false; var requiredMappings = new[] { "arrowUp", "arrowDown", "arrowLeft", "arrowRight" }; var scheme = Schemes[schemeName]; return requiredMappings.All(mapping => scheme.ContainsKey(mapping)); }Classes/Relics/RelicTemplate.cs (4)
5-11: Make Effects consistent with other properties for better encapsulation.While you've updated Name, Texture, and Tooltip to be auto-implemented properties, Effects remains a public field. For consistency and to follow C# best practices, consider converting it to a property as well.
- public RelicEffect[] Effects; + public RelicEffect[] Effects { get; set; }
5-5: Consider adding XML documentation for the IDisplayable interface implementation.Adding documentation about the IDisplayable interface implementation would improve code clarity and help other developers understand the purpose of this interface.
+/// <summary> +/// Template class for relics that implements the IDisplayable interface +/// to standardize how relics are displayed in the UI. +/// </summary> public partial class RelicTemplate : Resource, IDisplayable
7-11: Consider adding property validation for critical properties.The properties currently lack validation logic. If certain properties like Name or Texture are required, consider adding validation to ensure they're not set to null or empty values.
Example implementation for the Name property:
- public string Name { get; set; } + private string _name; + public string Name + { + get => _name; + set + { + if (string.IsNullOrEmpty(value)) + throw new ArgumentException("Name cannot be null or empty"); + _name = value; + } + }
26-30: Consider returning new RelicTemplate in a single statement.The Clone method creates a new object and immediately returns it. This could be simplified to a single statement for better readability.
public RelicTemplate Clone() { - RelicTemplate newRelic = new RelicTemplate(Name, Tooltip, Texture, Effects); - return newRelic; + return new RelicTemplate(Name, Tooltip, Texture, Effects); }scenes/UI/RewardSelectionUI.tscn (3)
13-16: Use theme constants for UI colors instead of hard-coded values.Hard-coded color values make it difficult to maintain consistent styling across the UI. Consider using theme constants for better maintainability.
Consider creating a theme resource file that defines your colors and applying it to the UI elements.
37-42: Add translation support for UI text.The "Rewards!" text is hard-coded, which makes localization difficult. Consider using Godot's translation system for better internationalization support.
Instead of hard-coding the text directly, use Godot's tr() function:
text = tr("Rewards!")Then add appropriate translation entries in your project's translation files.
53-58: Consider responsive design for SelectionBG.The SelectionBG has a fixed minimum size of 540x105 pixels, which might not work well on all screen sizes. Consider using relative sizing or container-based layouts for better responsiveness.
Evaluate whether this UI element could use size_flags or other container properties to adapt to different screen sizes instead of fixed dimensions.
Globals/BgAudioPlayer.cs (3)
20-23: Add fade-in capability for smoother music transitions.The current implementation starts playing music abruptly. Adding fade-in would create a more polished experience.
Consider implementing a fade-in feature:
public void PlayLevelMusic(float volume = -10f, float fadeInDuration = 0f) { if (fadeInDuration > 0) { // Start at a very low volume PlayMusic(_levelMusic, -40f); // Create a tween for fading in var tween = CreateTween(); tween.TweenProperty(this, "volume_db", volume, fadeInDuration); } else { PlayMusic(_levelMusic, volume); } }
25-29: Add fade-out capability for smoother music transitions.Similar to the fade-in suggestion, implementing a fade-out when stopping music would create a more polished audio experience.
Consider implementing a fade-out feature:
-public void StopMusic() -{ - Stop(); - Stream = null; -} +public void StopMusic(float fadeOutDuration = 0f) +{ + if (fadeOutDuration > 0 && Playing) + { + float startVolume = VolumeDb; + + // Create a tween for fading out + var tween = CreateTween(); + tween.TweenProperty(this, "volume_db", -40f, fadeOutDuration); + tween.TweenCallback(Callable.From(() => + { + Stop(); + Stream = null; + VolumeDb = startVolume; // Reset volume for next use + })); + } + else + { + Stop(); + Stream = null; + } +}
3-30: Support multiple music tracks for different game contexts.The current implementation only supports a single music track. Adding support for multiple tracks would improve flexibility.
Consider extending the class to support multiple music tracks:
public partial class BgAudioPlayer : AudioStreamPlayer { [Export] private Dictionary<string, string> _musicPaths = new Dictionary<string, string> { { "title", "res://scenes/SceneTransitions/assets/titleSong.ogg" }, // Add more music tracks here }; private Dictionary<string, AudioStream> _musicTracks = new Dictionary<string, AudioStream>(); public override void _Ready() { // Load all music tracks foreach (var pair in _musicPaths) { _musicTracks[pair.Key] = (AudioStream)ResourceLoader.Load(pair.Value); } } // Play a specific music track by key public void PlayMusic(string trackKey, float volume = -10f) { if (!_musicTracks.ContainsKey(trackKey)) { GD.PrintErr($"Music track '{trackKey}' not found"); return; } PlayMusicStream(_musicTracks[trackKey], volume); } // Original method renamed private void PlayMusicStream(AudioStream music, float volume) { // Existing implementation } // Backwards compatibility public void PlayLevelMusic(float volume = -10f) { PlayMusic("title", volume); } // Existing StopMusic method }scenes/NoteManager/scripts/InputHandler.cs (2)
56-79: Solid implementation of visual feedback system!The
FeedbackEffectmethod provides a clear way to emit particles based on hit quality, enhancing player feedback during gameplay.Consider adding parameter validation to handle potential null references or invalid arrow types:
public void FeedbackEffect(ArrowType arrow, string text) { + if (!Enum.IsDefined(typeof(ArrowType), arrow)) + { + GD.PrintErr($"Invalid arrow type: {arrow}"); + return; + } + // Get the particle node for this arrow var particles = Arrows[(int)arrow].Node.Particles; + if (particles == null) + { + GD.PrintErr($"Particles not found for arrow: {arrow}"); + return; + } // Rest of the method remains unchanged
87-111: Good implementation of a dynamic control scheme loader.The method cleanly handles reading control schemes from project settings and reconfiguring input mappings.
Consider these improvements for better error handling and maintainability:
- Extract the default scheme as a constant
- Add error handling for the Enum parsing
- Add more debug information
private void LoadControlScheme() { + const string DEFAULT_SCHEME = "ARROWS"; string scheme = ProjectSettings.HasSetting("game/input_scheme") ? (string)ProjectSettings.GetSetting("game/input_scheme") - : "ARROWS"; + : DEFAULT_SCHEME; + + GD.Print($"Loading control scheme: {scheme}"); foreach (var arrow in Arrows) { var events = InputMap.ActionGetEvents(arrow.Key); foreach (var inputEvent in events) { InputMap.ActionEraseEvent(arrow.Key, inputEvent); } } var selectedScheme = ControlSchemes.Schemes[scheme]; foreach (var arrow in Arrows) { if (selectedScheme.ContainsKey(arrow.Key)) { InputEventKey eventKey = new InputEventKey(); + try + { eventKey.Keycode = (Key)Enum.Parse(typeof(Key), selectedScheme[arrow.Key]); InputMap.ActionAddEvent(arrow.Key, eventKey); + } + catch (ArgumentException ex) + { + GD.PrintErr($"Invalid key configuration for {arrow.Key}: {selectedScheme[arrow.Key]}. Error: {ex.Message}"); + } } + else + { + GD.PrintErr($"No key mapping found for {arrow.Key} in scheme {scheme}"); + } } }scenes/Puppets/Enemies/EnemyPuppet.tscn (1)
13-17: Consider making health bar positioning more adaptable.The health bar is positioned using fixed offsets, which might not work well across different screen sizes or resolutions.
Consider using Control anchors or a more responsive positioning approach:
[node name="ProgressBar" parent="." instance=ExtResource("1_sxlrs")] - offset_left = -75.0 - offset_top = 32.0 - offset_right = 75.0 - offset_bottom = 52.0 + anchors_preset = 8 # Center + anchor_left = 0.5 + anchor_top = 0.5 + anchor_right = 0.5 + anchor_bottom = 0.5 + offset_left = -75.0 + offset_top = 32.0 + offset_right = 75.0 + offset_bottom = 52.0 + grow_horizontal = 2 + grow_vertical = 0scenes/ChestScene/ChestScene.cs (3)
7-10: Missing validation for exposed properties.The Player variable is public but not [Export] marked, and there's no validation to ensure ChestButton is properly set.
Consider applying the following improvements:
- public PlayerPuppet Player; + [Export] + public PlayerPuppet Player { get; private set; } [Export] public Button ChestButton;
29-33: Consider separating UI interaction and game logic.The current implementation tightly couples button state manipulation with game logic.
Consider separating concerns for better maintainability:
private void GetLoot() { ChestButton.Disabled = true; - RewardSelect.CreateSelection(this, Player.Stats, 3, "Relic").Selected += EndBattle; + var rewardSelection = RewardSelect.CreateSelection(this, Player.Stats, 3, "Relic"); + rewardSelection.Selected += OnRewardSelected; } + private void OnRewardSelected() + { + // Handle reward selection logic + EndBattle(); + }
35-39: Stage transition logic could be more modular.The method is directly accessing the StageProducer, making the class tightly coupled to the stage system.
Consider using a more modular approach:
+ [Export] + private NodePath _stageProducerPath = "/root/StageProducer"; + private StageProducer _stageProducer; + public override void _Ready() + { + // Existing code... + + // Get reference to StageProducer + _stageProducer = GetNode<StageProducer>(_stageProducerPath); + if (_stageProducer == null) + { + GD.PrintErr($"Failed to get StageProducer at path: {_stageProducerPath}"); + } + } private void EndBattle() { StageProducer.ChangeCurRoom(StageProducer.Config.BattleRoom); - GetNode<StageProducer>("/root/StageProducer").TransitionStage(Stages.Map); + if (_stageProducer != null) + { + _stageProducer.TransitionStage(Stages.Map); + } + else + { + GD.PrintErr("Cannot transition stage: StageProducer is null"); + } }scenes/ChartViewport/StarryNight.gdshader (1)
18-22: Consider making star parameters configurable.The code uses magic numbers for star threshold and brightness calculation.
Consider making these values configurable through uniform variables:
+ uniform float star_threshold = 0.996; + uniform float star_brightness = 0.85; + uniform float star_base_level = 0.95; void fragment() { float color = 0.0; - if (rand(SCREEN_UV.xy / 20.0) > 0.996) + if (rand(SCREEN_UV.xy / 20.0) > star_threshold) { float r = rand(SCREEN_UV.xy); - color = r * (0.85 * sin((TIME * time_scale) * (r * 5.0) + 720.0 * r) + 0.95); + color = r * (star_brightness * sin((TIME * time_scale) * (r * 5.0) + 720.0 * r) + star_base_level); }scenes/Remapping/ControlSettings.cs (1)
18-44: Consider using a more maintainable approach for control scheme initialization.The current switch statement works, but could be refactored to be more maintainable as you add more control schemes in the future.
Consider using a dictionary-based approach for better maintainability:
- switch (scheme) - { - case "ARROWS": - OnArrowButtonPressed(); - GetNode<Button>("Panel/ArrowButton").GrabFocus(); - break; - case "QWERT": - OnQWERTButtonPressed(); - GetNode<Button>("Panel/QWERTButton").GrabFocus(); - break; - case "WASD": - OnWASDButtonPressed(); - GetNode<Button>("Panel/WASDButton").GrabFocus(); - break; - } + var schemeActions = new Dictionary<string, (Action handler, string buttonPath)> + { + { "ARROWS", (OnArrowButtonPressed, "Panel/ArrowButton") }, + { "QWERT", (OnQWERTButtonPressed, "Panel/QWERTButton") }, + { "WASD", (OnWASDButtonPressed, "Panel/WASDButton") } + }; + + if (schemeActions.TryGetValue(scheme, out var action)) + { + action.handler(); + GetNode<Button>(action.buttonPath).GrabFocus(); + }scenes/ChestScene/ChestScene.tscn (3)
30-35: BackGround node uses manual positioning without responsivenessThe BackGround TextureRect has fixed offset values which could lead to layout issues on different screen resolutions. Consider using anchors or containers for better responsive layout.
- [node name="BackGround" type="TextureRect" parent="."] - z_index = -1 - offset_right = 640.0 - offset_bottom = 178.0 - texture = ExtResource("6_37nar") + [node name="BackGround" type="TextureRect" parent="."] + z_index = -1 + anchor_right = 1.0 + anchor_bottom = 0.5 + texture = ExtResource("6_37nar") + expand_mode = 2
36-44: StarShader uses manual scaling and positioningThe StarShader ColorRect uses fixed offset and scale values. For better responsiveness and maintainability, consider using anchors and relative sizing.
- [node name="StarShader" type="ColorRect" parent="."] - z_index = -1 - material = SubResource("ShaderMaterial_4lvcv") - offset_top = 180.0 - offset_right = 255.0 - offset_bottom = 262.0 - scale = Vector2(2.18, 2.18) - color = Color(0, 0, 0, 1) + [node name="StarShader" type="ColorRect" parent="."] + z_index = -1 + material = SubResource("ShaderMaterial_4lvcv") + anchor_top = 0.5 + anchor_right = 1.0 + anchor_bottom = 1.0 + color = Color(0, 0, 0, 1)
52-58: ChestButton lacks hover and pressed statesThe ChestButton is configured as a basic button with an icon but lacks custom styles for different states (normal, hover, pressed). Consider adding these for better user feedback.
You could enhance the button by setting up custom styles in a theme or by adding visual feedback through code signals like mouse_entered and mouse_exited.
Classes/Notes/Note.cs (1)
51-57: Clone method should clarify its copying behaviorWhile the comment mentions that shallow copy is preferable, it would be helpful to document this more explicitly, especially since the method creates a new Note instance with the same property values rather than using MemberwiseClone().
- //Eventually could look into something more robust, but for now shallow copy is preferable. - //We only would want val and name to be copied by value + //This is an explicit shallow copy that creates a new Note instance with the same property values. + //Deep copying is not needed since we're only transferring value types and references.scenes/Puppets/scripts/PuppetTemplate.cs (1)
71-81: Duplicate code pattern in Heal methodThe Heal method has similar text display logic as TakeDamage. Consider refactoring this common code into a separate helper method to reduce duplication.
+ private void ShowFloatingText(string text, Color color) + { + if (Sprite == null) return; + + TextParticle newText = new TextParticle(); + newText.Modulate = color; + Sprite.AddChild(newText); + newText.Text = text; + } public virtual void TakeDamage(int amount) { if (_currentHealth <= 0) return; //TEMP Only fire once. amount = Math.Max(0, amount); //Should not be able to heal from damage. _currentHealth = _healthBar.ChangeHP(-amount); if (_currentHealth <= 0) { Defeated?.Invoke(this); } if (amount == 0) return; - TextParticle newText = new TextParticle(); - newText.Modulate = Colors.Red; - Sprite.AddChild(newText); - newText.Text = $"-{amount}"; + ShowFloatingText($"-{amount}", Colors.Red); } public virtual void Heal(int amount) { _currentHealth = _healthBar.ChangeHP(amount); if (amount == 0) return; - TextParticle newText = new TextParticle(); - newText.Modulate = Colors.Green; - Sprite.AddChild(newText); - newText.Text = $"+{amount}"; + ShowFloatingText($"+{amount}", Colors.Green); }scenes/UI/scripts/RewardSelect.cs (4)
24-28: Consider class separation for different reward types.The class now handles two distinct reward types (relics and notes) with separate fields for each. As functionality grows, consider extracting shared behavior into a common base class or interface to maintain the Single Responsibility Principle.
- private RelicTemplate[] _rChoices; - private RelicTemplate _rSelection; - private Note[] _nChoices; - private Note _nSelection; + private IDisplayable[] _choices; + private IDisplayable _selection;
47-47: Simplified condition check.The visibility check could be simplified using a null-coalescing operator.
- _acceptButton.Visible = (_nSelection != null) || (_rSelection != null); + _acceptButton.Visible = _nSelection != null || _rSelection != null;
52-53: Consider using exceptions for validation.Using
GD.PushErrorlogs the error but allows execution to continue with invalid state. For critical validation like this, consider throwing an exception to prevent invalid state.if (amount < 1) - GD.PushError("Error: In RewardSelect: amount < 1"); + throw new ArgumentException("Amount must be greater than 0", nameof(amount));
116-126: Simplify selection logic.The selection logic could be simplified by using early returns for better readability.
- if (_nSelection == null && _rSelection == null) - return; - if (_nSelection != null) - { - _player.AddNote(_nSelection); - } - else if (_rSelection != null) - { - _player.AddRelic(_rSelection); - } + if (_nSelection == null && _rSelection == null) + return; + + if (_nSelection != null) + { + _player.AddNote(_nSelection); + } + else // _rSelection must be non-null here + { + _player.AddRelic(_rSelection); + }scenes/BattleDirector/test_battle_scene.tscn (1)
69-76: Consider keyboard accessibility for StartButton.The StartButton has a good modulate value for visibility, but ensure it has keyboard focus on scene load for better accessibility.
You should consider adding
focus_mode = 2to ensure the button can receive focus, and possiblygrab_focus = trueif this should be the default focused control when the scene loads.Classes/MidiMaestro/MidiMaestro.cs (3)
89-93: Consider cloning SongData on return.The
GetSongDatamethod returns a direct reference to the internalsongDataobject, which could allow external code to modify it. Consider returning a clone or making the fields immutable.public SongData GetSongData() { - return songData; + // Return a new instance to prevent external modification + return new SongData + { + Bpm = songData.Bpm, + SongLength = songData.SongLength, + NumLoops = songData.NumLoops + }; }
108-113: Improve time conversion calculations.The time conversion in
GetStartTimeSecondsis complex and could be simplified.-public float GetStartTimeSeconds() => - _note.TimeAs<MetricTimeSpan>(_tempoMap).Milliseconds / 1000f - + _note.TimeAs<MetricTimeSpan>(_tempoMap).Seconds; +public float GetStartTimeSeconds() +{ + var timeSpan = _note.TimeAs<MetricTimeSpan>(_tempoMap); + return timeSpan.TotalSeconds; +}
114-116: Consider adding method to get duration in seconds.Currently, the class provides
GetDuration()which returns duration in ticks, but it might be useful to also have a method that returns duration in seconds.public long GetDuration() => _note.Length; + +// Add a method to get the duration in seconds +public float GetDurationSeconds() +{ + var startTime = _note.TimeAs<MetricTimeSpan>(_tempoMap); + var endTime = _note.EndTimeAs<MetricTimeSpan>(_tempoMap); + return (float)(endTime.TotalSeconds - startTime.TotalSeconds); +}Classes/Relics/RelicEffect.cs (3)
8-9: Encapsulate_baseValueandValue
Currently,_baseValueis private whileValueis a public field. ExposingValueas a field may make it prone to unintended modifications from external classes. Consider turningValueinto a public property to enhance encapsulation and maintain consistent C# conventions.-private int _baseValue; -public int Value; +private int _baseValue; +public int Value { get; set; }
25-29: Consider making_effectPersistsan explicit setting
OnBattleEndconditionally resetsValuebased on_effectPersists. It’s a good approach if the relic effect truly shouldn’t persist beyond a single battle, but ensure you have a plan to toggle_effectPersistsexternally for different relic behaviors (e.g., “always-on” relics).
33-33: Include null-safety checks
When calling_onRelicEffect, consider verifying that_onRelicEffectis not null (if this is ever an edge case). This prevents potentialNullReferenceExceptionif the delegate is not assigned.Globals/FunkEngineNameSpace.cs (3)
61-62: NewStagesvalues
Chest,Boss, andControlsbroaden level states. Confirm that state transitions (e.g., fromBattletoBoss) are tested and don't produce unexpected transitions in the game flow.Also applies to: 65-65
79-105: NestedRoomclass
The approach is straightforward, with ID, coordinates, and type. However, the propertyChildrencould be improved by using a more flexible data structure (likeList<int>) instead of an array to avoid frequent array resizing.-public int[] Children { get; private set; } = Array.Empty<int>(); +public List<int> Children { get; private set; } = new List<int>(); public void AddChild(int newIdx) { if (!Children.Contains(newIdx)) { - Children = Children.Append(newIdx).ToArray(); + Children.Add(newIdx); } }
159-172: Remove debug logs for final production
GD.Print("Added child on same X.");is helpful for debugging but should eventually be removed or replaced with structured logging if it’s too verbose for production.scenes/NoteManager/scripts/NoteArrow.cs (1)
62-62: Use caution with repeated multiplicative edits toModulate
NoteHitmultipliesModulateby.7f, possibly stacking withOnLoopchanges. Keep track of repeated multiplications to avoid making the arrow too transparent too quickly if multiple hits or loops occur.Need help adjusting the logic or applying an upper/lower transparency bound?
scenes/ChartViewport/scripts/ChartManager.cs (3)
28-29: Consider enforcing minimum ChartLength.
The comment notes that values below ~2000 can cause visual breaks. It may be safer to clamp or validate this value to avoid unexpected behavior.- private double ChartLength = 5000; + private double ChartLength = 5000; // If needed, clamp in code: + + public double SafeChartLength { + get => ChartLength; + set => ChartLength = Math.Max(value, 2000); + }
107-112: Validate arrow type indices.
The CreateNote method accessesIH.Arrows[(int)arrow]. Ensure any enum values beyond the valid range are handled or validated. Otherwise, this may throw an out-of-range error if the enum grows or is misused.
120-127: Consider pooling or reusing text particles.
Repeated calls to ComboText create newTextParticleinstances. This is fine for occasional use, but high-frequency calls could lead to performance overhead or memory churn. Implementing object pooling or reusing instances might be worthwhile.Globals/StageProducer.cs (2)
31-39: Potentially store seed for reproducible runs.
Randomizing the global RNG and setting_seedis fine. If you want consistent debugging or replays, consider letting users supply a seed or saving the generated seed somewhere for replayable scenarios.
91-110: Guard against missing dictionary entries.
MakeConfigchooses a random index forScribe.SongDictionary, assuming at least two songs for battles. If fewer exist, the code might fail. Consider a guard or fallback.- int songIdx = GlobalRng.RandiRange(1, 2); + if (Scribe.SongDictionary.Count >= 3) { + int songIdx = GlobalRng.RandiRange(1, 2); + // ... + } else { + // Provide fallback logic, e.g. pick 0 + }scenes/BattleDirector/scripts/NotePlacementBar.cs (1)
156-159: Fixed potential overflow bug and added particle controlThe
HitNotemethod now correctly caps the bar value atMaxValueusingMath.Min(), preventing potential overflow issues. The commented-out particle emission control suggests this feature is not yet fully implemented.Consider uncommenting and testing the particle emission line if this feature is intended to be active:
- //fullBarParticles.Emitting = CanPlaceNote(); + fullBarParticles.Emitting = CanPlaceNote();scenes/Remapping/Remap.tscn (3)
31-54: Control scheme selection buttonsThe three buttons for different control schemes (WASD, Arrow Keys, QWER) are positioned in the upper right area of the panel.
Consider reorganizing these buttons in a vertical layout for better clarity and to avoid potential confusion with their current staggered arrangement. For example:
-[node name="WASDButton" type="Button" parent="Panel"] -layout_mode = 2 -offset_left = 450.0 -offset_top = 50.0 -offset_right = 555.0 -offset_bottom = 81.0 -text = "WASD" +[node name="WASDButton" type="Button" parent="Panel"] +layout_mode = 2 +offset_left = 470.0 +offset_top = 50.0 +offset_right = 570.0 +offset_bottom = 81.0 +text = "WASD" -[node name="ArrowButton" type="Button" parent="Panel"] -layout_mode = 0 -offset_left = 399.0 -offset_top = 94.0 -offset_right = 495.0 -offset_bottom = 125.0 -text = "Arrow Keys" +[node name="ArrowButton" type="Button" parent="Panel"] +layout_mode = 0 +offset_left = 470.0 +offset_top = 90.0 +offset_right = 570.0 +offset_bottom = 121.0 +text = "Arrow Keys" -[node name="QWERTButton" type="Button" parent="Panel"] -layout_mode = 0 -offset_left = 507.0 -offset_top = 94.0 -offset_right = 603.0 -offset_bottom = 125.0 -text = "QWER" +[node name="QWERTButton" type="Button" parent="Panel"] +layout_mode = 0 +offset_left = 470.0 +offset_top = 130.0 +offset_right = 570.0 +offset_bottom = 161.0 +text = "QWER"
125-143: Key sprite initializationThe directional key sprites are properly initialized with the correct textures and scaling. However, I notice they all use fixed positions.
Consider using a container-based layout (like VBoxContainer) rather than fixed positioning to make the UI more adaptable to different screen sizes and resolutions. This would make the UI more maintainable and responsive.
12-143: Add keyboard navigation between control optionsThe control scheme selection UI would benefit from explicit keyboard navigation setup between buttons to ensure users can navigate the interface using keyboard or controller.
Add focus neighbor relationships between the buttons to ensure proper keyboard/controller navigation:
[node name="WASDButton" type="Button" parent="Panel"] layout_mode = 2 offset_left = 450.0 offset_top = 50.0 offset_right = 555.0 offset_bottom = 81.0 text = "WASD" +focus_neighbor_bottom = NodePath("../ArrowButton") [node name="ArrowButton" type="Button" parent="Panel"] layout_mode = 0 offset_left = 399.0 offset_top = 94.0 offset_right = 495.0 offset_bottom = 125.0 text = "Arrow Keys" +focus_neighbor_top = NodePath("../WASDButton") +focus_neighbor_right = NodePath("../QWERTButton") +focus_neighbor_bottom = NodePath("../TitleButton") [node name="QWERTButton" type="Button" parent="Panel"] layout_mode = 0 offset_left = 507.0 offset_top = 94.0 offset_right = 603.0 offset_bottom = 125.0 text = "QWER" +focus_neighbor_top = NodePath("../WASDButton") +focus_neighbor_left = NodePath("../ArrowButton") +focus_neighbor_bottom = NodePath("../TitleButton") [node name="TitleButton" type="Button" parent="Panel"] layout_mode = 0 offset_left = 232.0 offset_top = 320.0 offset_right = 408.0 offset_bottom = 351.0 text = "Return to Title Screen" script = ExtResource("2_oippk") +focus_neighbor_top = NodePath("../ArrowButton")scenes/BattleDirector/scripts/BattleDirector.cs (9)
23-23: Public field might violate encapsulation.
ExposingNotePlacementBarpublicly can be necessary for scene or inspector references, but consider using[Export]and a private setter or a property if you want to avoid unintended external modifications.
83-88: Dynamic instantiation of theEnemyPuppet.
UsingStageProducer.Config.EnemyScenePathis flexible. If multiple enemies are expected, consider a robust loading mechanism or pooling.
96-103: Initial focus on_focusedButton.
Automatically grabbing focus is convenient, but re-check that other interactive UI elements do not conflict if the user is tabbing around.
113-117: EndBattle
Changing the scene to theMapstage might cause abrupt transitions if you plan to show post-battle results. Ensure all necessary logic (e.g., enemy cleanup, reward screen finalization) occurs first.
140-140: Debug key usage
UsingKey.Key0for debug might conflict if you plan to map that key for actual game input. Consider restricting debug kills to a development build or environment variable.
198-207: Battle status checks
Confirm thatMultiple enemiesscenario is handled in the future. Right now,BattleWon()triggers if the singleEnemyis defeated, but consider multi-target logic if you scale up.
216-219:BattleLost
Spawning anEndScreenis a direct approach. If user wants to replay or continue from a checkpoint, consider unpausing the scene or transitioning to a relevant screen.
237-262: Event-based design
Attaching battle events to triggers likeNotePlacedorOnLoopis a neat approach. Confirm no memory leaks by removing or nullifying these delegates once the battle ends if the node remains in memory.
289-289: DebugKillEnemy
Commented out approach is usable for quick debug. If you reintroduce it, consider a debug console command for maintainability.scenes/NoteManager/note_manager.tscn (2)
40-50: arrowDown additions
Position changed to Vector2(0, 112), plus Outline and particles. This is consistent with the directional layout. Watch out for central alignment if you plan symmetrical spacing between up/down arrows.
69-69: Divider positions
You’ve moved DivA, DivB, and DivC. If they remain invisible, no functional issue arises, but ensure the new coordinates align with your design if you re-enable them.Also applies to: 75-75, 81-81
scenes/SceneTransitions/TitleScreen.tscn (9)
20-25: Shader materials
You assignedtransparentStars.gdshaderandTitleFont.gdshader. Looks good for stylized effects. Confirm performance on lower-end devices or if toggling effects is needed in settings.
36-44: Particle process material
Adjusting gravity, hue variation, and turbulence can deliver pleasing background effects. Keep an eye on performance if you expand or intensify the particle system.
45-57: Augmenting theTitleControl
You added aTextLightnode path reference and changed modulate. If the text should be more readable, consider color contrast for different user color profiles.
73-101: Background layers
Using multiple parallax-ish sprites (BackTree, MidTree, FrontTree, Moon) can produce a scenic effect. Just confirm Z-index ordering or custom drawing is correct for the final layering.
102-107: Hidden sprite:Rabb
You setvisible = false. If this sprite is meant to appear later, ensure a reveal logic is added in the script. Otherwise, remove if not needed.
147-147: Hidden Godot sprite
Line 147 sets visible to false. If it’s purely a decorative placeholder, you can remove it from production builds to reduce overhead.
150-163: Title label tweaks
Applied a wave shader, an outline, shadow, custom font, and set text to "Midnight Riff." The ephemeral style fits the theme. If the user needs high contrast, consider offering a toggle or a less wavy fallback.
218-218: "Change Controls" button
Nice addition for user customization. Ensure the corresponding scene or script is fully implemented to handle control remapping.Also applies to: 220-225
226-232: HiddenEnemPuppetandEnemPuppet2
They’re both invisible and presumably for menu background animations. Provide an introduction or animation logic, or remove if not used.Also applies to: 233-238
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (43)
Audio/335571__magntron__gamemusic_120bpm.mp3is excluded by!**/*.mp3Audio/Song1.oggis excluded by!**/*.oggAudio/Song2.oggis excluded by!**/*.oggAudio/Song3.oggis excluded by!**/*.oggClasses/Notes/assets/heal_note.pngis excluded by!**/*.pngClasses/Notes/assets/quarter_note.pngis excluded by!**/*.pngClasses/Notes/assets/vampire_note.pngis excluded by!**/*.pngClasses/Relics/assets/Auroboros.pngis excluded by!**/*.pngClasses/Relics/assets/Colorboros.pngis excluded by!**/*.pngscenes/BattleDirector/assets/BattleFrame1.pngis excluded by!**/*.pngscenes/BattleDirector/assets/CoolBG.jpgis excluded by!**/*.jpgscenes/BattleDirector/assets/bgupdate.pngis excluded by!**/*.pngscenes/BattleDirector/assets/temp_note_queue.pngis excluded by!**/*.pngscenes/ChartViewport/LoopMarker.pngis excluded by!**/*.pngscenes/ChestScene/assets/Chest.pngis excluded by!**/*.pngscenes/Maps/assets/BattleIcon.pngis excluded by!**/*.pngscenes/Maps/assets/BossIcon.pngis excluded by!**/*.pngscenes/Maps/assets/ChestIcon.pngis excluded by!**/*.pngscenes/Maps/assets/Player.pngis excluded by!**/*.pngscenes/NoteManager/assets/arrow_outline.pngis excluded by!**/*.pngscenes/NoteManager/assets/new_arrow.pngis excluded by!**/*.pngscenes/Puppets/Enemies/BossBlood/assets/Enemy1EmissionShape.pngis excluded by!**/*.pngscenes/Puppets/Enemies/Parasifly/assets/Parasifly.pngis excluded by!**/*.pngscenes/Puppets/Enemies/TheGWS/assets/GhostWolfSnake.pngis excluded by!**/*.pngscenes/Remapping/assets/A_Key_Light.pngis excluded by!**/*.pngscenes/Remapping/assets/Arrow_Down_Key_Light.pngis excluded by!**/*.pngscenes/Remapping/assets/Arrow_Left_Key_Light.pngis excluded by!**/*.pngscenes/Remapping/assets/Arrow_Right_Key_Light.pngis excluded by!**/*.pngscenes/Remapping/assets/Arrow_Up_Key_Light.pngis excluded by!**/*.pngscenes/Remapping/assets/D_Key_Light.pngis excluded by!**/*.pngscenes/Remapping/assets/E_Key_Light.pngis excluded by!**/*.pngscenes/Remapping/assets/Q_Key_Light.pngis excluded by!**/*.pngscenes/Remapping/assets/R_Key_Light.pngis excluded by!**/*.pngscenes/Remapping/assets/S_Key_Light.pngis excluded by!**/*.pngscenes/Remapping/assets/T_Key_Light.pngis excluded by!**/*.pngscenes/Remapping/assets/W_Key_Light.pngis excluded by!**/*.pngscenes/SceneTransitions/assets/backTree.pngis excluded by!**/*.pngscenes/SceneTransitions/assets/background.pngis excluded by!**/*.pngscenes/SceneTransitions/assets/font.TTFis excluded by!**/*.ttfscenes/SceneTransitions/assets/frontTree.pngis excluded by!**/*.pngscenes/SceneTransitions/assets/midTree.pngis excluded by!**/*.pngscenes/SceneTransitions/assets/moon.pngis excluded by!**/*.pngscenes/SceneTransitions/assets/titleSong.oggis excluded by!**/*.ogg
📒 Files selected for processing (111)
.gitignore(1 hunks)Audio/335571__magntron__gamemusic_120bpm.mp3.import(0 hunks)Audio/Song1.ogg.import(1 hunks)Audio/Song2.ogg.import(1 hunks)Audio/Song3.ogg.import(1 hunks)Classes/MidiMaestro/MidiMaestro.cs(1 hunks)Classes/MidiMaestro/SongTemplate.cs(1 hunks)Classes/Notes/Note.cs(3 hunks)Classes/Notes/assets/heal_note.png.import(1 hunks)Classes/Notes/assets/quarter_note.png.import(1 hunks)Classes/Notes/assets/vampire_note.png.import(1 hunks)Classes/Relics/RelicEffect.cs(1 hunks)Classes/Relics/RelicTemplate.cs(1 hunks)Classes/Relics/assets/Auroboros.png.import(1 hunks)Classes/Relics/assets/Colorboros.png.import(1 hunks)Funk Engine.csproj(2 hunks)Funk Engine.sln.DotSettings.user(0 hunks)Globals/BGAudioPlayer.tscn(1 hunks)Globals/BgAudioPlayer.cs(1 hunks)Globals/FunkEngineNameSpace.cs(2 hunks)Globals/Scribe.cs(6 hunks)Globals/StageProducer.cs(2 hunks)Globals/TimeKeeper.cs(1 hunks)README.md(1 hunks)project.godot(3 hunks)scenes/BattleDirector/NotePlacementBar.tscn(4 hunks)scenes/BattleDirector/assets/BattleFrame1.png.import(1 hunks)scenes/BattleDirector/assets/bgupdate.png.import(1 hunks)scenes/BattleDirector/scripts/BattleDirector.cs(5 hunks)scenes/BattleDirector/scripts/Conductor.cs(4 hunks)scenes/BattleDirector/scripts/NotePlacementBar.cs(5 hunks)scenes/BattleDirector/scripts/TextParticle.cs(1 hunks)scenes/BattleDirector/test_battle_scene.tscn(1 hunks)scenes/ChartViewport/ChartViewport.tscn(1 hunks)scenes/ChartViewport/LoopMarker.png.import(1 hunks)scenes/ChartViewport/Loopable.cs(0 hunks)scenes/ChartViewport/StarryNight.gdshader(1 hunks)scenes/ChartViewport/hit_particles.tscn(1 hunks)scenes/ChartViewport/scripts/ChartManager.cs(3 hunks)scenes/ChartViewport/scripts/HitParticles.cs(1 hunks)scenes/ChartViewport/scripts/Loopable.cs(1 hunks)scenes/ChestScene/ChestScene.cs(1 hunks)scenes/ChestScene/ChestScene.tscn(1 hunks)scenes/ChestScene/assets/Chest.png.import(1 hunks)scenes/Maps/assets/BattleIcon.png.import(1 hunks)scenes/Maps/assets/BossIcon.png.import(1 hunks)scenes/Maps/assets/ChestIcon.png.import(1 hunks)scenes/Maps/assets/Player.png.import(1 hunks)scenes/Maps/cartographer.tscn(1 hunks)scenes/Maps/scripts/Cartographer.cs(3 hunks)scenes/NoteManager/assets/arrow_outline.png.import(1 hunks)scenes/NoteManager/assets/new_arrow.png.import(1 hunks)scenes/NoteManager/note.tscn(1 hunks)scenes/NoteManager/note_manager.tscn(1 hunks)scenes/NoteManager/scripts/InputHandler.cs(1 hunks)scenes/NoteManager/scripts/NoteArrow.cs(3 hunks)scenes/NoteManager/scripts/NoteChecker.cs(2 hunks)scenes/Puppets/Enemies/BossBlood/Boss1.tscn(1 hunks)scenes/Puppets/Enemies/BossBlood/P_BossBlood.cs(1 hunks)scenes/Puppets/Enemies/BossBlood/assets/Boss1EmissionShape.tres(1 hunks)scenes/Puppets/Enemies/BossBlood/assets/Enemy1.png.import(1 hunks)scenes/Puppets/Enemies/BossBlood/assets/Enemy1EmissionShape.png.import(1 hunks)scenes/Puppets/Enemies/EnemyEffect.cs(1 hunks)scenes/Puppets/Enemies/EnemyPuppet.cs(1 hunks)scenes/Puppets/Enemies/EnemyPuppet.tscn(1 hunks)scenes/Puppets/Enemies/Parasifly/P_Parasifly.cs(1 hunks)scenes/Puppets/Enemies/Parasifly/Parasifly.tscn(1 hunks)scenes/Puppets/Enemies/Parasifly/assets/Parasifly.png.import(1 hunks)scenes/Puppets/Enemies/TheGWS/GWS.tscn(1 hunks)scenes/Puppets/Enemies/TheGWS/P_TheGWS.cs(1 hunks)scenes/Puppets/Enemies/TheGWS/assets/GhostWolfSnake.png.import(1 hunks)scenes/Puppets/PlayerPuppet.tscn(1 hunks)scenes/Puppets/scripts/PlayerPuppet.cs(1 hunks)scenes/Puppets/scripts/PlayerStats.cs(2 hunks)scenes/Puppets/scripts/PuppetTemplate.cs(3 hunks)scenes/Remapping/ControlSchemes.cs(1 hunks)scenes/Remapping/ControlSettings.cs(1 hunks)scenes/Remapping/Remap.tscn(1 hunks)scenes/Remapping/assets/A_Key_Light.png.import(1 hunks)scenes/Remapping/assets/Arrow_Down_Key_Light.png.import(1 hunks)scenes/Remapping/assets/Arrow_Left_Key_Light.png.import(1 hunks)scenes/Remapping/assets/Arrow_Right_Key_Light.png.import(1 hunks)scenes/Remapping/assets/Arrow_Up_Key_Light.png.import(1 hunks)scenes/Remapping/assets/D_Key_Light.png.import(1 hunks)scenes/Remapping/assets/E_Key_Light.png.import(1 hunks)scenes/Remapping/assets/Q_Key_Light.png.import(1 hunks)scenes/Remapping/assets/R_Key_Light.png.import(1 hunks)scenes/Remapping/assets/S_Key_Light.png.import(1 hunks)scenes/Remapping/assets/T_Key_Light.png.import(1 hunks)scenes/Remapping/assets/W_Key_Light.png.import(1 hunks)scenes/SceneTransitions/TitleScreen.tscn(6 hunks)scenes/SceneTransitions/assets/2d_lights_and_shadows_neutral_point_light.webp.import(1 hunks)scenes/SceneTransitions/assets/TitleFont.gdshader(1 hunks)scenes/SceneTransitions/assets/backTree.png.import(1 hunks)scenes/SceneTransitions/assets/background.png.import(1 hunks)scenes/SceneTransitions/assets/font.TTF.import(1 hunks)scenes/SceneTransitions/assets/frontTree.png.import(1 hunks)scenes/SceneTransitions/assets/midTree.png.import(1 hunks)scenes/SceneTransitions/assets/moon.png.import(1 hunks)scenes/SceneTransitions/assets/titleSong.ogg.import(1 hunks)scenes/SceneTransitions/assets/transparentStars.gdshader(1 hunks)scenes/SceneTransitions/scripts/SceneChange.cs(0 hunks)scenes/SceneTransitions/scripts/TitleScreen.cs(1 hunks)scenes/SceneTransitions/testTransition.tscn(0 hunks)scenes/UI/EndScreen.tscn(1 hunks)scenes/UI/Pause.tscn(1 hunks)scenes/UI/RewardSelectionUI.tscn(5 hunks)scenes/UI/inventory.tscn(1 hunks)scenes/UI/scripts/EndScreen.cs(1 hunks)scenes/UI/scripts/MenuModule.cs(1 hunks)scenes/UI/scripts/RewardSelect.cs(1 hunks)
💤 Files with no reviewable changes (5)
- scenes/SceneTransitions/scripts/SceneChange.cs
- Audio/335571__magntron__gamemusic_120bpm.mp3.import
- scenes/SceneTransitions/testTransition.tscn
- scenes/ChartViewport/Loopable.cs
- Funk Engine.sln.DotSettings.user
✅ Files skipped from review due to trivial changes (44)
- .gitignore
- Audio/Song1.ogg.import
- scenes/Remapping/assets/S_Key_Light.png.import
- scenes/Maps/assets/Player.png.import
- scenes/Remapping/assets/R_Key_Light.png.import
- Audio/Song2.ogg.import
- scenes/Puppets/Enemies/TheGWS/assets/GhostWolfSnake.png.import
- Classes/Notes/assets/vampire_note.png.import
- scenes/SceneTransitions/assets/backTree.png.import
- scenes/Maps/assets/ChestIcon.png.import
- scenes/SceneTransitions/assets/background.png.import
- scenes/Puppets/Enemies/Parasifly/assets/Parasifly.png.import
- scenes/SceneTransitions/assets/frontTree.png.import
- Classes/Notes/assets/heal_note.png.import
- scenes/Remapping/assets/W_Key_Light.png.import
- scenes/Remapping/assets/D_Key_Light.png.import
- scenes/SceneTransitions/assets/moon.png.import
- scenes/SceneTransitions/assets/midTree.png.import
- scenes/BattleDirector/assets/BattleFrame1.png.import
- scenes/Remapping/assets/E_Key_Light.png.import
- scenes/Remapping/assets/Q_Key_Light.png.import
- scenes/Maps/assets/BattleIcon.png.import
- scenes/Maps/assets/BossIcon.png.import
- scenes/Remapping/assets/A_Key_Light.png.import
- scenes/NoteManager/assets/arrow_outline.png.import
- scenes/BattleDirector/assets/bgupdate.png.import
- Classes/Relics/assets/Auroboros.png.import
- scenes/Remapping/assets/Arrow_Up_Key_Light.png.import
- Globals/BGAudioPlayer.tscn
- scenes/SceneTransitions/assets/titleSong.ogg.import
- scenes/Remapping/assets/Arrow_Left_Key_Light.png.import
- scenes/Puppets/Enemies/BossBlood/assets/Enemy1.png.import
- scenes/Remapping/assets/T_Key_Light.png.import
- scenes/NoteManager/assets/new_arrow.png.import
- scenes/UI/inventory.tscn
- scenes/Remapping/assets/Arrow_Right_Key_Light.png.import
- Classes/Notes/assets/quarter_note.png.import
- scenes/Puppets/Enemies/BossBlood/assets/Enemy1EmissionShape.png.import
- scenes/Remapping/assets/Arrow_Down_Key_Light.png.import
- scenes/ChartViewport/LoopMarker.png.import
- scenes/Puppets/Enemies/BossBlood/assets/Boss1EmissionShape.tres
- scenes/ChestScene/assets/Chest.png.import
- Audio/Song3.ogg.import
- Classes/Relics/assets/Colorboros.png.import
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
18-18: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
🔇 Additional comments (166)
scenes/BattleDirector/scripts/TextParticle.cs (4)
10-10: LGTM! Higher Z-Index ensures text is visible above other elements.Setting ZIndex to 2 ensures the text particle will render above most other UI elements, improving visibility.
12-12: Nice animation enhancement with Elastic transition.Changing from Quad to Elastic transition gives the text a more bouncy, dynamic feel which makes feedback more visually engaging and attention-grabbing.
14-15: Good performance optimization with faster animation timings.The animation speed changes (0.5s → 0.25s for up movement and 0.5s → 0.1s for down movement) combined with relative positioning creates a snappier, more responsive feel while maintaining visual clarity.
17-18: Improved fade-out timing for better user experience.Reducing the fade-out duration from 1s to 0.15s and explicitly setting the transition type to Quad creates a cleaner, more responsive visual effect that doesn't linger unnecessarily.
scenes/Puppets/scripts/PlayerPuppet.cs (2)
10-10: Good defensive programming with null-coalescing operator.Using the null-coalescing operator ensures Stats is always initialized even if StageProducer.PlayerStats is null, preventing potential NullReferenceExceptions.
15-15:Details
❓ Verification inconclusive
Verify that visual initialization is handled elsewhere.
The code for loading textures and setting positions has been removed, with only the UniqName being set. Make sure these initialization steps are properly handled elsewhere in the codebase.
🏁 Script executed:
#!/bin/bash # Description: Check for texture loading and position initialization for PlayerPuppet # Looking for calls to Init() method or other initialization methods that might have replaced the removed code # Search for initialization in StageProducer or similar classes rg -A 5 -B 5 "PlayerPuppet.*Init|Init.*PlayerPuppet" --type cs # Search for any other method that might be handling player initialization rg -A 3 -B 3 "InitPlayer|PlayerInit|SetupPlayer" --type cs # Check if there's a parent class method that might be handling initialization ast-grep --pattern $'class PuppetTemplate : \w+ { $$$ public void Init($$$) { $$$ } $$$ }'Length of output: 229
Manual verification required: Confirm visual initialization for PlayerPuppet
The executed commands didn’t yield any output confirming that texture loading and position initialization have been relocated. Please verify manually that these tasks are indeed managed elsewhere in the codebase (for example, via the parent class or other initialization modules such as StageProducer).
- Check if the Parent class (likely PuppetTemplate) includes an Init method handling visual aspects.
- Verify that any external initialization (like in StageProducer or related classes) properly sets up texture loading and position for PlayerPuppet.
- Ensure that removing the visual initialization from PlayerPuppet.cs is intentional and that its responsibilities have been migrated appropriately.
scenes/SceneTransitions/assets/TitleFont.gdshader (4)
3-10: Good use of uniform parameters for shader customization.The uniform variables allow for customization of the animation and appearance without changing the shader code, making it reusable and easy to tweak from the Godot editor.
12-14: LGTM! Standard GLSL random function implementation.This is a standard pseudo-random function implementation in GLSL using the common dot product and sine approach.
16-19: Effective vertex animation for wave effect.The sine wave displacement on the Y-axis creates a nice dynamic wave effect that will make text feel more alive and engaging.
48-51: LGTM! Commented light function is appropriately documented.The commented light function with explanation is a good practice, making it clear this is intentionally not implemented.
Globals/TimeKeeper.cs (1)
11-14: LGTM! Useful positive modulo implementation.The
PosModmethod ensures a positive modulo result even for negative inputs, which is important for time cycle calculations. This is a common utility function in game development for handling cyclical values.For reference, this implementation handles cases where:
- When
iis positive, it behaves like a regular modulo operation- When
iis negative, it returns a positive value in the expected range [0, mod)This is particularly useful for:
- Wrapping time positions in looping audio/animation
- Circular/cyclical calculations
- Array index wrapping
scenes/Puppets/Enemies/TheGWS/P_TheGWS.cs (1)
4-17: Initialize battle events for this enemy type.The
P_TheGWSclass doesn't initialize the_battleEventsarray from the base class, unlike theP_BossBloodclass.Check if this enemy is intended to have no battle effects, or if this is an oversight. If it should have effects, consider adding initialization similar to what's done in
P_BossBlood.cs.scenes/Puppets/Enemies/BossBlood/P_BossBlood.cs (1)
19-30: Verify the value of the healing effect.A healing value of 5 per loop may be unbalanced depending on player damage output and loop frequency.
Review the healing value in context of:
- Player's average damage output
- How often the loop trigger occurs
- Total boss health
For example, if the animation loops every 2 seconds, and the boss gains 5 HP each time, this could be very challenging if player damage is low.
Funk Engine.csproj (1)
11-11:Details
✅ Verification successful
Adding Melanchall.DryWetMidi package - good choice for MIDI file handling.
This is a good addition to support MIDI file operations in the project. The DryWetMidi library provides comprehensive capabilities for working with MIDI files, including reading, writing, and manipulating MIDI data.
🌐 Web query:
Is Melanchall.DryWetMidi 7.2.0 the latest version available? Are there any known security issues with this version?💡 Result:
Melanchall.DryWetMidi 7.2.0 is not the latest version available. The most recent stable release is 7.2.1-prerelease1 (listed in the version history as updated on September 15, 2024) [1][5]. However, this appears to be a prerelease version, making 7.2.0 the latest stable release as of September 5, 2024 [1][2][5].
Security Considerations
No security vulnerabilities are explicitly mentioned in:
- The GitHub issues [4]
- Release notes [5]
- Official documentation [6][7]
- Community discussions [9][10]
Recent updates focus on:
- Device support improvements for macOS/Apple Silicon [5]
- API refinements in object management [5]
- MIDI file sanitization features [5]
While no security flaws are documented, users should note:
- Some functional bugs exist in device connection handling (Unity editor crashes reported) [4]
- The native package version contains platform-specific dependencies, while the nativeless variant removes these [3]
- Recent updates improved MIDI event validation during file operations [5][9]
For production use, version 7.2.0 remains the recommended stable release despite newer prerelease versions existing. Developers using Unity should reference the official Unity Asset Store integration guidelines [6][8].
Citations:
- 1: https://www.nuget.org/packages/Melanchall.DryWetMidi/5.2.1
- 2: https://www.nuget.org/packages/Melanchall.DryWetMidi/
- 3: https://www.nuget.org/packages/Melanchall.DryWetMidi.Nativeless
- 4: https://github.com/melanchall/drywetmidi/issues
- 5: https://github.com/melanchall/drywetmidi/releases
- 6: https://melanchall.github.io/drywetmidi/articles/dev/Using-in-Unity.html
- 7: https://github.com/melanchall/drywetmidi
- 8: https://assetstore.unity.com/packages/tools/audio/drywetmidi-222171
- 9: https://www.codeproject.com/Articles/1200014/DryWetMIDI-High-Level-Processing-of-MIDI-Files
- 10: https://forums.ni.com/t5/Example-Code/Communicating-with-MIDI-Device-in-LabVIEW/ta-p/4393774
MIDI Package Addition Verified: Stable Version Confirmed
The inclusion of the Melanchall.DryWetMidi package at version 7.2.0 is appropriate for production. Although a newer prerelease (7.2.1-prerelease1) exists, version 7.2.0 remains the latest stable release, and there are no reported security issues with this release.
- File: Funk Engine.csproj, Line 11
- Action: No changes required—retain the current package version.
scenes/SceneTransitions/scripts/TitleScreen.cs (2)
6-7: LGTM: Exported PointLight2D for title screen effect.The TextLight field is properly exported, allowing it to be set in the editor.
24-27: Ensure BgAudioPlayer exists before accessing it.The code directly accesses the BgAudioPlayer node without checking if it exists, which could lead to null reference exceptions.
Add null checking before accessing the audio player:
public override void _EnterTree() { - GetNode<BgAudioPlayer>("/root/BgAudioPlayer").PlayLevelMusic(); + var audioPlayer = GetNode<BgAudioPlayer>("/root/BgAudioPlayer"); + if (audioPlayer != null) + { + audioPlayer.PlayLevelMusic(); + } + else + { + GD.PrintErr("BgAudioPlayer not found at /root/BgAudioPlayer"); + } }scenes/Puppets/Enemies/EnemyEffect.cs (1)
13-25: The constructor initialization looks good!The constructor correctly initializes all properties and captures the needed delegate for later execution.
scenes/ChartViewport/hit_particles.tscn (1)
5-17: Particle configuration looks good!The particle effect is well-configured with appropriate values for a hit effect. The z-index being set to -1 ensures it appears behind other elements, and the parameters for spread, lifetime, and velocity create a nice explosive effect.
scenes/Puppets/Enemies/TheGWS/GWS.tscn (3)
7-11: The scene structure looks good.The EnemPuppet node is properly structured with correct paths to the health bar and sprite.
13-15: Sprite setup is appropriate.The Sprite node is positioned correctly and uses the referenced texture.
17-21: ProgressBar positioning looks good.The ProgressBar is properly instantiated and positioned with appropriate offsets.
scenes/SceneTransitions/assets/transparentStars.gdshader (1)
5-7: Well-implemented random function.The
randfunction uses a common and efficient pseudo-random number generation technique for shaders.Classes/MidiMaestro/SongTemplate.cs (1)
3-25:Details
✅ Verification successful
Why is this class marked as partial?
The class is marked as
partialbut there doesn't seem to be any other part of the class in the provided code. If there's no need for it to be partial, consider removing the partial modifier.
🏁 Script executed:
#!/bin/bash # Search for other parts of the SongTemplate class echo "Searching for other parts of the SongTemplate class..." rg -A 5 "class SongTemplate" --glob "!Classes/MidiMaestro/SongTemplate.cs"Length of output: 195
Action: Verify the necessity of the
partialmodifier inSongTemplate.The initial search (excluding the file itself) did not return any additional parts of the
SongTemplateclass. However, to be extra certain, please run an extended search across the entire repository with the command below:If the output confirms that the only occurrence is in
Classes/MidiMaestro/SongTemplate.cs, then it’s likely that thepartialmodifier is redundant. In that case, please consider removing thepartialkeyword unless there is a specific plan to split the class implementation or if code generation is anticipated.
🏁 Script executed:
#!/bin/bash echo "Searching for all occurrences of 'partial class SongTemplate' in the repository..." rg -n "partial class SongTemplate"Length of output: 289
Action: Consider Removing the
partialModifier from SongTemplateAfter verifying the repository, it appears that
SongTemplateis declared only once (inClasses/MidiMaestro/SongTemplate.cs). There are no additional partial implementations detected. Unless you have plans for future code-splitting or code-generation that would require a partial class, it’s recommended to remove thepartialmodifier to simplify the class declaration.scenes/ChartViewport/scripts/Loopable.cs (1)
16-24:Details
✅ Verification successful
Dependency on TimeKeeper needs verification.
The class relies on a
TimeKeeperclass that isn't defined here, creating an implicit dependency. Ensure that TimeKeeper is properly initialized before this class is used.
🏁 Script executed:
#!/bin/bash # Find TimeKeeper class definition to understand its properties and methods rg -t cs "class TimeKeeper" -A 10 rg -t cs "ChartLength|LoopLength|CurrentTime|PosMod" -A 3 -B 3Length of output: 6540
Dependency on TimeKeeper: Confirmed but Verify Initialization Order
After reviewing the codebase, the dependency is correctly implemented:
- TimeKeeper is defined in
Globals/TimeKeeper.cswith the expected static properties and methods (e.g.,ChartLength,LoopLength,CurrentTime,PosMod).- Initialization is handled externally, for example in
scenes/ChartViewport/scripts/ChartManager.csandscenes/BattleDirector/scripts/BattleDirector.cs, where the TimeKeeper’s properties are set prior to use.Please ensure that the initialization sequence guarantees that these TimeKeeper properties are set before any calls to the
_Processfunction inLoopable.csare made. If there’s any risk of the execution order causing uninitialized values, consider adding explicit safeguards (or initialization checks) to further protect against potential runtime issues.scenes/Puppets/PlayerPuppet.tscn (2)
16-20:Details
❓ Verification inconclusive
Health bar positioning uses absolute offsets.
The progress bar uses absolute offset values which may not adapt well to different screen sizes or if the character sprite changes.
Check if there's a more flexible positioning approach available, such as using anchors or containers:
🏁 Script executed:
#!/bin/bash # Check if other progress bars in the project use different positioning approaches echo "Checking progress bar positioning in other scenes" rg -t tscn "ProgressBar" -A 10Length of output: 168
Action: Verify Flexible Progress Bar Positioning
The progress bar inscenes/Puppets/PlayerPuppet.tscnis currently positioned using hardcoded absolute offsets—which may not adapt well for varying screen sizes or sprite changes. An initial automated search for alternative flexible approaches (e.g., using anchors or container nodes) was inconclusive due to file type recognition issues. Please verify manually whether other scenes implement a more adaptive layout method, or consider refactoring this positioning to utilize anchors/containers where feasible.
3-5:Details
❓ Verification inconclusive
Check resource path organization.
The texture resource is loaded from the BattleDirector folder while the script and packed scene are from the Puppets folder. Consider organizing assets more consistently.
🏁 Script executed:
#!/bin/bash # Check asset organization structure echo "Checking file organization structure" echo "====================================" # Check current asset locations echo "Current texture location:" fd -p "scenes/BattleDirector/assets/Character1.png" # Check if there's a Puppets/assets directory echo -e "\nPuppets assets directory structure:" fd -p "scenes/Puppets/assets" -t d # Check if other character textures follow the same pattern echo -e "\nOther character textures:" fd -p "Character.*\.png"Length of output: 682
Consistent Asset Organization Review
The texture resource (
Character1.png) is still located underscenes/BattleDirector/assets, while both the script and the scene reside in thescenes/Puppetsfolder. Please verify if this mixed organization is intentional. If the texture is used exclusively by Puppets, consider moving it (or its associated import file) into a dedicated Puppets assets directory to maintain consistency.
- File under review:
scenes/Puppets/PlayerPuppet.tscn(lines 3-5)- Current texture location:
scenes/BattleDirector/assets/Character1.png- Suggestion: Confirm whether this organization meets the project’s asset management strategy or if relocating the texture asset will improve maintainability.
scenes/SceneTransitions/assets/font.TTF.import (1)
38-40:Details
✅ Verification successful
Language and OpenType features support are empty.
The import file has empty dictionaries for language support, script support, and OpenType features, which might limit font functionality for international text.
If your game supports multiple languages, consider configuring these properties:
🏁 Script executed:
#!/bin/bash # Check if the project has localization files that might indicate multi-language support echo "Checking for localization files in the project" fd "\.translation$|strings\.json|locale|i18n"Length of output: 150
Attention: Internationalization Setup in Font Import
The file
scenes/SceneTransitions/assets/font.TTF.importcurrently defines empty dictionaries forlanguage_support,script_support, andopentype_features. Our verification—with a search for localization files—did not reveal any evidence of multi-language support in the repository. If international text functionality is not a project requirement at this time, leaving these dictionaries empty appears intentional. However, if you plan to implement multi-language support in the future, please ensure these properties are configured accordingly.scenes/Puppets/Enemies/BossBlood/Boss1.tscn (1)
25-30:Details
❓ Verification inconclusive
Verify health management implementation.
The scene uses a health bar component, but it's important to ensure that the P_BossBlood script properly initializes and updates this health bar during gameplay.
🏁 Script executed:
#!/bin/bash # Check if the P_BossBlood script properly initializes health values # Search for health initialization in the P_BossBlood script rg -i "health|hp" "scenes/Puppets/Enemies/BossBlood/P_BossBlood.cs" # Search for health bar update methods rg -i "update.*health|set.*health" "scenes/Puppets/Enemies/BossBlood/P_BossBlood.cs" # Check base EnemyPuppet class for health-related methods rg -i "health|hp" "scenes/Puppets/Enemies/EnemyPuppet.cs"Length of output: 210
Action Required: Manually Verify Health Bar Updates in Boss Scripts
The scene file correctly assigns a health bar component, but our automated search did not find clear health management logic in either the
P_BossBlood.csor theEnemyPuppet.csfiles (e.g., no matches for keywords like “health”, “hp”,_healthBaror “ProgressBar”). Please verify the following manually:
- Confirm that the
P_BossBloodscript properly initializes and updates the health bar during gameplay.- Check if the base class
EnemyPuppetcontains the necessary health management methods that are expected to be used or overridden byP_BossBlood.- Ensure that the integration between the scene’s health bar component and the script logic is functioning as intended.
README.md (1)
13-14: Welcome new team members!Good update adding new artists to the team.
scenes/UI/EndScreen.tscn (1)
5-9:Details
✅ Verification successful
Validate button array indices in the EndScreen.cs script.
The buttons array is defined with indices that must be correctly referenced in the script. Ensure the script correctly matches buttons to their expected functions.
🏁 Script executed:
#!/bin/bash # Check if the EndScreen script correctly uses button indices # Check the EndScreen script for button setup rg -A 10 "buttons\[" "scenes/UI/scripts/EndScreen.cs"Length of output: 439
Subject: Button Array Indices Verified in EndScreen Script
After reviewing the mapping between the scene file and the script, the indices are correctly referenced:
buttons[0]: Maps to the Restart node and is correctly invoked by both theRestarthandler and for grabbing focus.buttons[1]: Although named "Title" in the scene file, it is used for theQuitToMainMenuaction as intended.buttons[2]: Correctly corresponds to the Quit node and its associated handler.No changes are required at this time. Please ensure that any future modifications to the node ordering or naming in the scene file are reflected in the script accordingly.
scenes/UI/RewardSelectionUI.tscn (1)
94-104: Ensure button states are properly handled in the UI.The buttons should provide clear visual feedback when hovered, pressed, or disabled. Make sure that proper theme states are defined for these buttons.
Check if these buttons have appropriate styling for different states (normal, hover, pressed, disabled) to ensure good user experience. This may require examining the theme resources applied to these buttons.
scenes/Puppets/Enemies/EnemyPuppet.tscn (2)
6-9:Details
❓ Verification inconclusive
Node naming appears to have a typo.
The main node is named "EnemPuppet" instead of "EnemyPuppet", which may cause confusion or reference issues.
🏁 Script executed:
#!/bin/bash # Check if this node name is referenced elsewhere in the codebase find . -type f -name "*.cs" -o -name "*.tscn" -o -name "*.tres" | xargs grep -l "EnemPuppet" | grep -v "EnemyPuppet.tscn"Length of output: 299
Attention: Verify Node Naming Consistency
It appears that the node is named "EnemPuppet" in
scenes/Puppets/Enemies/EnemyPuppet.tscn, which might be a typo. However, our search shows that this naming is referenced in several other scenes:
scenes/SceneTransitions/TitleScreen.tscnscenes/Puppets/Enemies/TheGWS/GWS.tscnscenes/Puppets/Enemies/Parasifly/Parasifly.tscnscenes/Puppets/Enemies/BossBlood/Boss1.tscnBefore proceeding, please verify whether the current spelling ("EnemPuppet") is intentional. If it is indeed a typo, all these references will need to be updated accordingly.
11-11:Details
❓ Verification inconclusive
Missing sprite texture assignment.
The Sprite2D node doesn't have a texture assigned, which means the enemy will be invisible in game.
🏁 Script executed:
#!/bin/bash # Check if textures for enemies exist somewhere in the project find ./assets -type f -name "*.png" | grep -i enemyLength of output: 97
Sprite2D Texture Assignment Missing – Please Verify Asset Location
In
scenes/Puppets/Enemies/EnemyPuppet.tscn(line 11), theSprite2Dnode lacks a texture assignment. Without a texture, the enemy sprite will not be visible at runtime. The initial search command targeted an./assetsdirectory but returned an error (directory not found), so it’s unclear if enemy textures are stored elsewhere in the repository.
- Action Required: Manually verify where enemy textures are located. If they are stored in a different folder or meant to be loaded dynamically, update the scene file with the correct texture assignment.
scenes/ChartViewport/StarryNight.gdshader (3)
4-9: Well-structured shader with appropriate uniform variables.The shader provides good customization options through uniform variables for colors, gradient ratio, and time scale.
11-13: Standard random function implementation for shader.The random function is correctly implemented using the common dot product technique.
24-26: Clean implementation of background gradient.The gradient blending and final color composition are well implemented.
scenes/Remapping/ControlSettings.cs (5)
4-17: Class structure looks good with clear separation of concerns.The
ControlSettingsclass is well-structured with properly exported Sprite2D fields for visual representation of control keys. The design allows for easy modification in the Godot editor.
46-52: Good consistent pattern for handler methods.The handler method follows a consistent pattern with the others, which is good for maintainability.
54-60: Good consistent pattern for handler methods.Handler maintains the same structure as the others.
62-68: Good consistent pattern for handler methods.Handler maintains the same structure as the others.
70-77:Details
✅ Verification successful
Verify that ControlSchemes.SpriteMappings exists and contains required keys.
The
ChangeKeySpritesmethod assumes thatControlSchemes.SpriteMappings[scheme]exists and contains keys for "left", "right", "up", and "down". Ensure that this is always the case to avoid runtime errors.
🏁 Script executed:
#!/bin/bash # Check for the existence of ControlSchemes class and its SpriteMappings dictionary # Look for the ControlSchemes.cs file echo "Searching for ControlSchemes class definition..." fd "ControlSchemes.cs" | xargs cat # Look for the SpriteMappings dictionary and its initialization echo -e "\nSearching for SpriteMappings dictionary in ControlSchemes class..." ast-grep --pattern $'class ControlSchemes { $$$ public static Dictionary<string, Dictionary<string, string>> SpriteMappings $$$ $$$ }'Length of output: 2963
ControlSchemes.SpriteMappings Verified
The
ControlSchemes.csfile properly defines the staticSpriteMappingsdictionary with expected mappings for schemes ("WASD", "ARROWS", "QWERT"). For each scheme, the inner dictionaries include the required keys: "left", "right", "up", and "down", which confirms that theChangeKeySpritesmethod will retrieve valid resource paths.
- The dictionary is correctly initialized in
ControlSchemes.cs.- Each supported scheme ("WASD", "ARROWS", "QWERT") has entries for "left", "right", "up", and "down".
I recommend, however, that you consider adding input validation or error handling in
ChangeKeySpritesto gracefully handle the scenario when an unsupported scheme is passed, thereby avoiding potential runtime errors.scenes/BattleDirector/scripts/Conductor.cs (4)
48-49: Logic might allow overwriting existing notes.The logic now allows adding a note if the lane is already occupied at the specified beat, which seems intentional but could lead to unexpected behavior if not handled carefully.
- if (beat == 0 || _laneData[(int)type][beat] != null) + if (beat == 0 || (_laneData[(int)type][beat] != null && _laneData[(int)type][beat].IsActive))
86-96: Improved note generation from MIDI data.The refactored
AddExampleNotesmethod is a good improvement, using dynamic MIDI data instead of hardcoded notes. This enhances maintainability and flexibility.
61-62:Details
❓ Verification inconclusive
Validate the NoteHit method before calling.
The code now calls
arrow.NoteHit()whenisActiveis false, which is different from the previous behavior. Ensure this doesn't cause unintended side effects.
🏁 Script executed:
#!/bin/bash # Check how NoteHit method is implemented and used elsewhere echo "Checking the implementation of NoteHit method in NoteArrow class..." ast-grep --pattern 'class NoteArrow { $$$ public void NoteHit() { $$$ } $$$ }' echo -e "\nChecking other usages of NoteHit method..." ast-grep --pattern '$$$NoteHit($$$)'Length of output: 377
Action Required: Verify NoteHit Behavior on Inactive Arrows
The current change calls
arrow.NoteHit()whenisActiveis false, which is a departure from the previous behavior. The automated search did not reveal the implementation ofNoteHitin the expected location, so please double-check that its behavior is correct and that calling it under these conditions does not introduce any side effects. In particular, verify that:
- The implementation of
NoteHitin theNoteArrowclass (or its equivalent) safely handles being invoked when an arrow is inactive.- Any changes in behavior stemming from this activation state are intentional and documented.
- Other usages of
NoteHitremain consistent with this new call site behavior.
100-100:Details
✅ Verification successful
Update method signature impacts existing calls.
The
CheckMissmethod signature has changed to accept arealBeatparameter. Ensure all calling code has been updated accordingly.
🏁 Script executed:
#!/bin/bash # Find all calls to CheckMiss method to verify they've been updated echo "Searching for calls to CheckMiss method..." ast-grep --pattern 'CheckMiss($$$)' echo -e "\nChecking for implementations of the CheckMiss method..." ast-grep --pattern 'public void CheckMiss($$$)'Length of output: 309
Method Signature Update Verified
The updated signature forCheckMiss(now accepting therealBeatparameter) has been verified using static analysis. No outdated call sites were detected, so all references appear to have been appropriately updated. Please remain mindful of any dynamic invocations (e.g., via reflection) that static analysis might not capture.project.godot (9)
13-13: Application name changed to "ProjectFunkEngine".The application name has been updated from "Funk Engine" to "ProjectFunkEngine". Ensure this change is reflected in other places where the application name is used.
16-16: Updated application icon to use a character sprite.The icon has been changed from "res://icon.svg" to "res://scenes/BattleDirector/assets/Character1.png". Consider if a character sprite is appropriate as an application icon versus a dedicated logo.
23-23: New BgAudioPlayer autoload added.Added a new global audio player for background music, which is a good practice for managing game audio.
36-38: New game settings section with input scheme.Added a new [game] section with input_scheme setting defaulting to "ARROWS". This aligns with the new control remapping functionality.
46-47: Enhanced key mappings for arrowUp action.Added additional key mappings (X and E keys) for the arrowUp action to support the QWERT control scheme.
54-55: Enhanced key mappings for arrowDown action.Added additional key mappings (C and W keys) for the arrowDown action to support the QWERT control scheme.
62-63: Enhanced key mappings for arrowLeft action.Added additional key mappings (Z and Q keys) for the arrowLeft action to support the QWERT control scheme.
70-71: Enhanced key mappings for arrowRight action.Added additional key mappings (V and R keys) for the arrowRight action to support the QWERT control scheme.
84-88:Details
✅ Verification successful
Added new Secondary action with Shift key mapping.
A new Secondary action has been added with the Shift key as the input event. Ensure this action is properly utilized in the game code.
🏁 Script executed:
#!/bin/bash # Check for usages of the new Secondary action in the codebase echo "Searching for usages of the Secondary action..." rg -i "Secondary" --type csLength of output: 242
Confirmed Secondary Action Integration
- The new “Secondary” action is now correctly referenced in the codebase (e.g., in
scenes/BattleDirector/scripts/NotePlacementBar.csviaInput.IsActionPressed("Secondary")).- The input mapping in
project.godot(lines 84–88) reflects the new action configuration with the expected settings.- Please take a brief moment to verify that the intended Shift key behavior is correctly implemented in-game (i.e., that the key event with shift functionality is working as expected).
scenes/NoteManager/note.tscn (3)
1-5: Updated scene with new texture resources and structure.The scene has been enhanced with additional resources, including a new arrow texture and an outline texture.
7-11: Enhanced Node2D with references to child sprites.The Right-arrow node now includes references to OutlineSprite and IconSprite, which enables better control over these visual elements from the NoteArrow script.
13-16: Added Outline and Icon sprites for improved visual feedback.These new sprites will provide better visual feedback for the arrows, enhancing the game's user experience.
scenes/ChestScene/ChestScene.tscn (3)
23-25: Audio autoplay feature confirmationThe AudioStreamPlayer is set to autoplay a song, which is appropriate for background music. Ensure this is the intended behavior for the chest scene and that the audio levels are properly balanced.
27-28:Details
❓ Verification inconclusive
UILayer uses MenuModule without specific configuration
The UILayer uses the MenuModule script but doesn't have any unique properties or connections configured. Verify that the MenuModule correctly interfaces with this scene and provides the expected functionality.
🏁 Script executed:
#!/bin/bash # Verify how MenuModule.cs is used in the project # Check the MenuModule.cs implementation echo "Examining MenuModule.cs implementation..." rg -A 10 "public partial class MenuModule" "scenes/UI/scripts/MenuModule.cs" || echo "MenuModule.cs not found or class definition not available" # Check for references to ChestScene in MenuModule echo "Checking for ChestScene references in MenuModule..." rg "ChestScene" "scenes/UI/scripts/MenuModule.cs" || echo "No explicit references to ChestScene found in MenuModule.cs"Length of output: 842
Action Required: Verify MenuModule Functionality in ChestScene
The UILayer is using the MenuModule script with its default configuration—there are no additional properties or scene-specific connections set up. Our verification shows that the MenuModule implementation (in
scenes/UI/scripts/MenuModule.cs) does not reference or tailor its behavior specifically for ChestScene. As a result, please confirm that the generic functionality provided by MenuModule meets the requirements when used in ChestScene. In particular, ensure that its input handling (e.g., for "Pause" and "Inventory") functions correctly in the context of ChestScene during runtime.
- Verify that no additional configuration is needed for MenuModule to reliably interface with ChestScene.
- Confirm that the input event handling and related actions in MenuModule operate as expected when this scene is active.
18-21:Details
❌ Incorrect review comment
ChestButton reference lacks signal connections
The ChestButton is referenced in the script via NodePath, but there are no visible signal connections set up for this button (like button.pressed). This may result in the button not triggering any actions when clicked.
🏁 Script executed:
#!/bin/bash # Verify if there are signal connections in the ChestScene.cs script # Check for signal connections in the ChestScene.cs script echo "Checking for button signal connections in ChestScene.cs..." rg -A 5 -B 5 "ChestButton" "scenes/ChestScene/ChestScene.cs" || echo "No ChestButton signal connections found or file not available" # Look for common signal connection patterns echo "Checking for common signal connection patterns..." rg -A 3 "Connect\(" "scenes/ChestScene/ChestScene.cs" || echo "No Connect() calls found or file not available" rg -A 3 "Pressed \+=" "scenes/ChestScene/ChestScene.cs" || echo "No Pressed += patterns found or file not available"Length of output: 1316
Revised Review Comment: Signal Connection Already Implemented
After verifying the code in
scenes/ChestScene/ChestScene.cs, it's clear that theChestButtonsignal is connected in the_Ready()method via the line:ChestButton.Pressed += GetLoot;This establishes the necessary signal connection, so the concern about a missing connection is not applicable. Please disregard the earlier comment.
Likely an incorrect or invalid review comment.
Classes/Notes/Note.cs (3)
12-14: Good property encapsulationConverting public fields to properties with getters and setters improves encapsulation, particularly the private setter for CostModifier which prevents unwanted changes after initialization.
Also applies to: 17-18
20-28: Constructor updated with new CostModifier parameterThe constructor now includes a costModifier parameter with a sensible default value of 1.0f. This maintains backward compatibility while allowing for cost adjustments when needed.
Also applies to: 43-43
9-9:Details
✅ Verification successful
IDisplayable interface implementation verification needed
The class now implements IDisplayable, but the interface definition isn't shown in the provided code. Ensure all required interface members are properly implemented.
🏁 Script executed:
#!/bin/bash # Check the IDisplayable interface and its implementation # Find the IDisplayable interface definition echo "Searching for IDisplayable interface definition..." rg -A 10 "interface IDisplayable" || echo "IDisplayable interface definition not found" # Check if Note implements all required members of IDisplayable echo "Checking for properties required by IDisplayable..." rg "Name.*get;.*set;" "Classes/Notes/Note.cs" rg "Tooltip.*get;.*set;" "Classes/Notes/Note.cs" rg "Texture.*get;.*set;" "Classes/Notes/Note.cs"Length of output: 842
IDisplayable Implementation Verified
The IDisplayable interface (defined in
Globals/FunkEngineNameSpace.cs) requires the propertiesName,Tooltip, andTextureand all of these are properly implemented inClasses/Notes/Note.cs.scenes/Maps/cartographer.tscn (1)
16-20:Details
❓ Verification inconclusive
Background color modification may affect visibility
The BG sprite has a specific modulate color (0.462, 0.462, 0.66) that creates a blueish tint. Ensure this color choice doesn't negatively impact visibility of other elements in the scene.
🏁 Script executed:
#!/bin/bash # Check if color modulation is consistent across scenes # Check for similar color modulation in other scenes echo "Checking for similar color modulation patterns in other scenes..." rg "modulate = Color\(" --type=tscn || echo "No similar color modulation patterns found"Length of output: 302
Attention: Verify Visibility Impact of BG Sprite's Modulate Color
The BG sprite in
scenes/Maps/cartographer.tscn(lines 16–20) uses a specific modulate color (Color(0.462, 0.462, 0.66, 1)) that introduces a blueish tint. A search for similar modulate patterns did not yield any hits, though note that the tool flagged an unrecognized file type for.tscnfiles, so the search may not have been comprehensive.
- Action Items:
- Manually verify that this blueish tint does not reduce the visibility of other critical scene elements.
- If the modulate effect is intended to be unique, confirm through in-game testing across different environments and lighting conditions.
scenes/Puppets/scripts/PuppetTemplate.cs (2)
12-25: Exported properties improve configurabilityThe addition of exported properties for Sprite, StartPos, InitScale, and hideHealth allows for better configuration in the Godot editor, providing more flexibility in scene setup.
52-69: Improved damage handling logicThe TakeDamage method now ensures damage is non-negative and has more efficient early returns for edge cases. This is a good improvement to the function logic.
However, there's no null check for Sprite when adding the TextParticle, which could cause runtime errors:
if (amount == 0) return; TextParticle newText = new TextParticle(); newText.Modulate = Colors.Red; -Sprite.AddChild(newText); +if (Sprite != null) +{ + Sprite.AddChild(newText); +} +else +{ + AddChild(newText); // Fallback to add to puppet itself if Sprite is null +} newText.Text = $"-{amount}";scenes/UI/scripts/RewardSelect.cs (5)
19-20: Good implementation of event pattern for selection notification.The delegate and event pattern is well-implemented here, providing a clean way for other components to subscribe to selection events.
86-100: Good factory method implementation.The static factory method provides a convenient way to create and initialize the reward selection UI, and handles adding it to the scene and pausing the game tree properly.
102-106: Consistent handling for note selection.The
OnNoteSelectedmethod follows the same pattern asOnRelicSelected, which is good for consistency.
127-127: Null-conditional event invocation.Good use of the null-conditional operator to safely invoke the event.
56-56:Details
✅ Verification successful
Verify user feedback on relic selection.
The function is now using
Scribe.GetRandomRelicswith_player.CurRelicsto avoid selecting already owned relics. Confirm this logic works correctly by testing with a player who already owns relics.
🏁 Script executed:
#!/bin/bash # Check how relic uniqueness is enforced in the game ast-grep --pattern $'AddRelic($_)'Length of output: 35
I'll need to verify further that the relic selection prevents duplicate relics by confirming the implementation of
Scribe.GetRandomRelicsand ensuring it properly excludes already owned relics. The initial check via an AST grep forAddRelic($_)returned no output, so it's unclear if that's the correct indicator.To help us get a clearer picture, please run the following shell script, which searches for the definition of
GetRandomRelics(likely implemented in a file like Scribe.cs). This will help confirm that the uniqueness logic is in place:Once you review the output:
- If the method clearly excludes already owned relics from the selection, then the use of
_player.CurRelicsin the call can be considered safe.- Otherwise, further manual verification by testing with a player who has existing relics may be necessary.
Please verify the results manually and confirm that the logic meets the desired behavior.
Relic Uniqueness Verification – Please Review and Confirm
🏁 Script executed:
#!/bin/bash # Search for the definition of the GetRandomRelics method to review its logic for relic uniqueness. rg "public static .*GetRandomRelics" -nLength of output: 153
Below is a new shell script to retrieve more context from the definition of
GetRandomRelicsin Globals/Scribe.cs. This will let us inspect the filtering logic (for example, how it checks and excludes relics already owned) to verify that duplicate relics are not returned when using_player.CurRelics:Once you review the output:
- Confirm that the method properly excludes already owned relics.
- If it does, please verify with testing using a player model that owns relics.
Based on those findings, the review comment can be finalized. For now, please run this script and check the snippet to verify that the intended uniqueness behavior is correctly implemented.
Relic Uniqueness Verification Pending – Please Confirm Implementation Details
🏁 Script executed:
#!/bin/bash # Retrieve a broader snippet (30 context lines) for GetRandomRelics to inspect its uniqueness logic. rg "public static RelicTemplate\\[\\] GetRandomRelics\\(RelicTemplate\\[\\] ownedRelics, int count\\)" -n -C 30 Globals/Scribe.csLength of output: 2155
Relic Selection Logic Verified
The implementation of
Scribe.GetRandomRelicsproperly filters out already owned relics by checking that none of the owned relics share the same name as those in the available pool. The code randomizes the eligible relics and applies a fallback if there aren’t enough, ensuring the function’s intended uniqueness behavior. This confirms that using_player.CurRelicsin the call meets the desired behavior.scenes/BattleDirector/test_battle_scene.tscn (4)
20-27: Good UI organization with focused button reference.The
_focusedButtonproperty is now properly referenced to theStartButtonnode, which helps with keyboard navigation and accessibility.
39-49: Well-structured NotePlacementBar with proper z-indexing.The addition of a
NPBBackingtexture with a negative z-index ensures the background displays properly behind the content.
62-67:Details
❓ Verification inconclusive
Verify BattleFrame's sizing constraints.
The BattleFrame sits at z-index 1 (above everything), but ensure it doesn't obscure important UI elements.
🏁 Script executed:
#!/bin/bash # Check for overlapping UI elements based on z-index rg -A 5 "z_index = 1" | grep -E "offset_(top|left|right|bottom)"Length of output: 427
Review: Verify BattleFrame's sizing constraints
The automated check confirms that in
scenes/BattleDirector/test_battle_scene.tscnthe BattleFrame has the configured offsets (offset_top = 178.0,offset_right = 640.0,offset_bottom = 360.0), and similar offset values are seen inscenes/ChestScene/ChestScene.tscn. Although these values appear consistent, please manually verify that positioning a UI element atz_index = 1does not inadvertently obscure any other important UI components during actual gameplay.
56-60:Details
❓ Verification inconclusive
Consider specifying stretch mode for background texture.
The BackGround TextureRect might benefit from a specific stretch mode to handle different screen sizes properly.
🏁 Script executed:
#!/bin/bash # Check if TextureRect objects in other scenes have stretch_mode set ast-grep --pattern $'[node name=$_ type="TextureRect"] $$$ stretch_mode = $_ $$$'Length of output: 82
Stretch Mode Specification for BackGround Node
- In
scenes/BattleDirector/test_battle_scene.tscn(lines 56–60), theBackGroundTextureRect node does not currently specify astretch_mode. This property is often used to ensure proper scaling of textures across different screen sizes.- A repository-wide search did not reveal any TextureRect nodes with a
stretch_modeassignment—which suggests that adding one here might be beneficial for uniform responsiveness.- Consider explicitly setting a stretch mode (for example, one of Godot’s options like
STRETCH_SCALEorSTRETCH_KEEP_ASPECT) to see if it improves the background scaling on various resolutions. Please verify the chosen mode aligns with your overall design and doesn’t introduce unexpected layout issues.Globals/Scribe.cs (10)
4-4: Good import organization.Adding MidiMaestro import is appropriate for the SongDictionary implementation.
17-22: Damage calculation change may impact game balance.The damage calculation for the
EnemyBasenote has been reduced from4 - (int)timingto3 - (int)timing. This reduces the maximum damage by 1, which could affect game balance.Has this change been playtested to ensure it doesn't make the game too easy? Consider documenting the rationale for this change.
49-59: Good implementation of healing mechanic.The new
PlayerHealnote provides a nice counterbalance to damage-dealing notes, giving players more strategic options.
72-83: Good use of cost modifier for balance.The
PlayerQuarternote uses a cost modifier of 0.25f to balance its effect, which is a good approach to game balance.
107-119: Simple buff to Good Vibes relic.The healing value for the "Good Vibes" relic has been increased from 1 to 2, and the description simplified. This makes the relic more valuable.
Confirm that this buff is intentional and doesn't create balance issues when combined with other healing effects.
198-215: Good improvement to relic selection logic.Changing from
AnytoAllin the relic filtering logic ensures that only available relics (those not already owned) are selected.
157-193:Details
✅ Verification successful
Verify song asset paths.
The SongDictionary references audio files and MIDI files. Ensure these paths are correct and the files exist.
🏁 Script executed:
#!/bin/bash # Check if the audio files referenced in SongDictionary exist echo "Checking for Audio/Song1.ogg..." [ -f "Audio/Song1.ogg" ] && echo "Found" || echo "Not found" echo "Checking for Audio/Song2.ogg..." [ -f "Audio/Song2.ogg" ] && echo "Found" || echo "Not found" echo "Checking for Audio/Song3.ogg..." [ -f "Audio/Song3.ogg" ] && echo "Found" || echo "Not found" echo "Checking for Audio/midi/Song1.mid..." [ -f "Audio/midi/Song1.mid" ] && echo "Found" || echo "Not found" echo "Checking for Audio/midi/Song2.mid..." [ -f "Audio/midi/Song2.mid" ] && echo "Found" || echo "Not found" echo "Checking for Audio/midi/Song3.mid..." [ -f "Audio/midi/Song3.mid" ] && echo "Found" || echo "Not found"Length of output: 770
Asset paths in SongDictionary successfully verified. All referenced audio and MIDI files exist as expected.
- Audio files ("Audio/Song1.ogg", "Audio/Song2.ogg", "Audio/Song3.ogg") were found.
- MIDI files ("Audio/midi/Song1.mid", "Audio/midi/Song2.mid", "Audio/midi/Song3.mid") were confirmed to exist.
No further changes are required.
60-71:Details
❓ Verification inconclusive
Verify balancing of vampire note.
The
PlayerVampirenote provides both healing and damage in a single action, which could be very powerful. Ensure this is balanced against the cost or availability of this note.
🏁 Script executed:
#!/bin/bash # Check how notes are balanced through cost modifiers ast-grep --pattern $'new Note($$$, $_, $_)'Length of output: 44
Action Required: Manual Verification of
PlayerVampireBalancingThe automated search did not yield evidence of additional cost modifiers or balancing logic related to the
PlayerVampirenote. As the current instantiation sets a cost of1while combining healing and damage effects, please manually verify that:
- The configured cost and availability of the note are in line with the game’s overall balance.
- Any external configuration or game design parameters (elsewhere in the codebase) that adjust note balance are correctly calibrated for this note.
Review file
Globals/Scribe.cs(Lines 60–71) to ensure that the note’s aggregated effects are appropriately counterbalanced.
138-154:Details
❓ Verification inconclusive
Verify charging mechanics with Colorboros.
The Colorboros relic charges the freestyle bar every loop. Ensure this interaction works properly with the charging system.
🏁 Script executed:
#!/bin/bash # Check how the NotePlacementBar.IncreaseCharge method is implemented ast-grep --pattern $'IncreaseCharge($_)'Length of output: 41
Action Required: Verify Colorboros Relic Charging Mechanism
Automated checks did not return results confirming the implementation of
IncreaseCharge(specifically, its invocation viadirector.NotePlacementBar.IncreaseCharge(val)). Please manually verify that the NotePlacementBar’sIncreaseChargemethod is correctly updating the freestyle bar as intended when the Colorboros relic is triggered.
- Ensure
IncreaseCharge(val)properly increases the charge as expected.- Check for any side effects or discrepancies within the charging mechanism.
121-137:Details
❓ Verification inconclusive
Ensure loop mechanics work properly with Auroboros.
The Auroboros relic increases the combo multiplier every loop, potentially creating exponential power scaling. Verify that this scales appropriately in longer battles.
🏁 Script executed:
#!/bin/bash # Check how the NotePlacementBar.IncreaseBonusMult method is implemented ast-grep --pattern $'IncreaseBonusMult($_)'Length of output: 44
Action: Verify Loop Scaling in Auroboros Relic Implementation
The current implementation in
Globals/Scribe.cs(lines 121–137) callsdirector.NotePlacementBar.IncreaseBonusMult(val);every loop cycle. Our initial automated search for the implementation of
IncreaseBonusMultdid not return any results. This makes it essential that you manually verify that the method’s logic within theNotePlacementBarclass properly handles bonus multiplier increments without causing unintended exponential scaling over extended battles. Please review the implementation to ensure that the loop mechanics remain balanced, especially in longer-running battles.Classes/MidiMaestro/MidiMaestro.cs (3)
9-22: Well-structured MIDI handling class.The
MidiMaestroclass is well-organized with clear private fields for note storage by direction.
78-88: Good switch expression usage.The
GetNotesmethod uses a switch expression, which is clean and concise.
97-106: Good encapsulation of MIDI note information.The
midiNoteInfoclass provides a clean interface to the underlying MIDI note data.Classes/Relics/RelicEffect.cs (3)
10-11: Clarify the role of_effectPersists
_effectPersistsdefaults tofalse, but there's no visible mechanism for changing its value. If a relic effect is intended to persist across battles, you may want to make_effectPersistsconfigurable from outside or internally after certain conditions are met.
16-16: Keep parameter consistency
You addedRelicEffectas a parameter in the delegate. Ensure you update all relevant event handling code that references the old delegate signature to avoid mismatches.
19-20: Constructor logic looks sound
You correctly initialize_baseValue,Value, and_onRelicEffect. This approach ensures the essential state is set up front.Also applies to: 22-23
Globals/FunkEngineNameSpace.cs (8)
1-3: New imports are fine
The additions ofSystem,System.Linq, andFunkEngine.Classes.MidiMaestrolook appropriate for the new functionalities.
8-13: ValidateSongDatausage
TheSongDatastruct correctly captures BPM, length, and loop count. Ensure the rest of the codebase or theMidiMaestroreferences these fields consistently (e.g., handling fractional BPM or double precision forSongLength).
15-21: Consider makingArrowData.Nodea weaker reference
Holding a direct reference to aNoteCheckernode might cause unexpected coupling if the node’s lifecycle is different. Double-check that the node isn't destroyed prematurely or replaced, which might lead to a stale reference.
40-46: BattleConfig struct modifications
StoringCurSongandEnemyScenePathis beneficial for linking battle settings. Consider verifying if all usage sites properly handle a null or uninitializedSongTemplate.
48-55: Additional trigger:OnLoop
TheOnLoopaddition seems aligned with cyclical gameplay. Ensure the relevant event handlers register for it to avoid silent no-ops if unimplemented.
68-72: MapGrid fields initialization
_map,_rooms, and_curIdxare used extensively. Double-check any expansions or re-initializations do not inadvertently discard data, especially ifInitMapGridis called multiple times.
107-124: Check for potential infinite recursion
GeneratePath_rcalls itself recursively. The logic seems bounded byy < height - 2, but ensure that edge cases (like extremely highpaths) do not create overshadowed recursion that might cause performance overhead or unexpected infinite loops.
195-200: IDisplayable interface
Using an interface for name, tooltip, and texture is a good approach to standardize UI display for multiple relics, items, etc. Keep an eye on usage patterns to ensure these are consistently populated.scenes/NoteManager/scripts/NoteArrow.cs (4)
13-13:IsActivedefaulting totrue
InitializingIsActivetotruemakes sense for an arrow that starts on screen. Just ensure the arrow’s state is switched off if it’s created in an inactive scenario.
[approve]
16-20: ExportingOutlineSpriteandIconSprite
Exporting these asSprite2Dreferences improves flexibility. Double-check their usage in the scene to ensure they’re assigned and properly updated in tandem with the arrow's movement.
22-33: EnhancedInitmethod
PassingNote notebroadens the arrow’s customization. This is good for dynamic textures. EnsureRotationlogic is correct if note icons aren’t oriented consistently.
51-56: Refined loop logic
TheOnLoopmethod checks!IsActivebefore adjustingModulate. This helps maintain a visually distinct “inactive” arrow. Confirm thatModulatechanges actually convey the desired effect in gameplay (especially if layering multiple.7fmultiplications).scenes/ChartViewport/scripts/ChartManager.cs (2)
56-78: Ensure cleanup of created tweens.
The new BeginTweens method introduces a recurring animation that loops indefinitely. Confirm you have a plan to stop or dispose of these tweens when transitioning away or freeing the node, to prevent potential memory leaks or unexpected behavior.
96-101: Assign a NoteRef for loop arrows if needed.
The logic correctly creates two NoteArrow instances for visual looping. However, only the primary arrow’sNoteRefis set. If the loop arrow also requires a reference to the same Note for consistency or future lookups, assign it similarly.scenes/ChartViewport/ChartViewport.tscn (1)
1-50: Resource references and layout changes look good.
All newly added resources and shader parameters appear consistent. The updated offsets, camera position, and viewport size suggest a refined layout. Just confirm that the negative left offset and the newLoopOffsetproperty on theLoopMarkernode are used or documented as intended.Globals/StageProducer.cs (3)
13-23: Be cautious with static fields and concurrency.
MakingIsInitializedandConfigstatic can cause unexpected behavior if multiple game sessions or tests run in parallel. Ensure they’re reset or managed correctly to prevent conflicts.
40-48: Check for invalid indexes in room transitions.
TransitionFromRoomusesMap.GetRooms()[nextRoomIdx]. EnsurenextRoomIdxis in range to avoid out-of-bounds errors.
50-89: State transitions are clear but ensure stage re-initialization logic is pinned down.
TheTransitionStagemethod togglesIsInitializedfor certain stages and re-initializes the game if uninitialized. Verify that partial re-initialization can’t lead to partial data states.scenes/BattleDirector/NotePlacementBar.tscn (7)
1-1: Note the increased complexity in scene loadingThe
load_stepsparameter has increased from 9 to 12, indicating the addition of new resources. This is expected given the new particle systems added to the scene.
13-14: Texture dimensions adjusted for better visual appearanceThe gradient texture width has been increased (from 26 to 34), which should provide a better visual appearance for the progress bar.
Also applies to: 22-22
17-18: Color gradient configuration enhancedThe gradient offsets and colors have been modified to create a smoother transition between colors, which should create better visual feedback.
26-50: New particle system configuration looks goodThe ParticleProcessMaterial has been properly configured with appropriate parameters for the particle effect, including randomness, emission shape, and gravity. This will enhance visual feedback.
78-89: Well-configured GPU particle systemThe Rock particle system is properly configured with appropriate position, amount, and parameters. The node will emit particles when triggered from the script, creating a visual effect when the progress bar is filled.
90-102: CPU particles as an alternative visual effectThe CPUParticles2D node is configured with appropriate parameters for a different visual effect. Using CPU particles for this smaller effect is a good choice as it's less resource-intensive than GPU particles.
124-127: Node positions adjusted for better layoutThe positions of the NextNote and CurrentNote sprites have been adjusted to better align with the queue visual. This change is consistent with the swapped node references in lines 62-65.
Also applies to: 129-132
scenes/BattleDirector/scripts/NotePlacementBar.cs (11)
12-12: New bonus multiplier addedThe
bonusMultinteger field has been added to track additional multipliers, which enhances gameplay by allowing for more dynamic scoring beyond just combo-based multipliers.
21-23: Particle system references properly exportedThe script now properly references the particle systems that were added to the scene file, allowing for programmatic control of visual effects based on gameplay events.
Also applies to: 31-33
37-44: Juice effect implementationGood implementation of "juice" (visual feedback) using shake effects. The comment with the YouTube reference is helpful for understanding the purpose of this code.
45-64: Well-implemented screen shake effectThe
ProcessShakemethod implements a controlled screen shake effect with appropriate dampening. The implementation includes:
- A limiter to control shake frequency
- Intensity based on bar value
- Proper dampening using Lerp
- Random offsets for natural-looking shake
This adds good visual feedback for players when they reach the maximum bar value.
75-75: Properly initialized shake position and processingThe
_barInitPositionis correctly initialized in_Ready()and the shake processing is properly set up in the_Process()method, ensuring the visual effect will work as expected.Also applies to: 78-81
165-166: Reset bonus multiplier on missed notesThe
MissNotemethod now correctly resets both the combo and the bonus multiplier when a note is missed, which is appropriate for gameplay balance.
171-183: New helper methods for game mechanicsThe addition of
IncreaseBonusMultandIncreaseChargemethods provides clean ways to manipulate the game state from other components. Both methods include appropriate safeguards and UI updates.
188-192: Improved note placement mechanicsThe
PlacedNotemethod now decreases the bar value based on the note's cost modifier instead of resetting it completely. This provides a more nuanced gameplay mechanic that rewards efficient play. Also, the method now returns the appropriate note based on whether the secondary action is pressed.
202-203: Updated combo multiplier calculationThe
DetermineComboMultmethod now includes the bonus multiplier in the calculation, properly integrating the new gameplay mechanic.
205-208: Added getter for combo valueThe addition of
GetCurrentCombo()method allows other components to access the current combo value, which is useful for UI displays or gameplay mechanics.
213-213: Particle emission tied to bar stateThe
UpdateNotePlacementBarmethod now controls the emission of particles based on whether the bar is at maximum value, providing appropriate visual feedback.scenes/Maps/scripts/Cartographer.cs (9)
8-16: New properties for enhanced map interactionThe addition of the
PlayerSprite,_validButtonsarray,_focusedButton, and_bgPlayerfields enhances the map interaction system with visual representation and audio integration.
20-26: Updated _Ready with focus handling and win conditionThe
_Readymethod now subscribes to theGuiFocusChangedevent and checks for win conditions. The win condition check is appropriate as it verifies if the current room is a boss room with no children.
28-32: Background audio initializationThe
_EnterTreemethod correctly initializes the background audio player and starts the level music, ensuring audio begins at the appropriate time in the scene lifecycle.
34-46: Focus management for UI navigationThe added focus management in
_Processand the newUpdateFocusmethod ensure that the user can navigate the map UI effectively, maintaining focus on valid buttons even when focus is lost.
56-70: Improved DrawMap with ordered buttons and neighbor relationshipsThe
DrawMapmethod now properly orders the buttons by x-position and establishes focus neighbor relationships, creating a more intuitive navigation experience.
72-109: Enhanced map drawing with interactive buttonsThe
DrawMapSpritemethod has been significantly improved to:
- Create buttons instead of sprites for better interaction
- Handle disabled states for inaccessible rooms
- Set up event handlers for room selection
- Apply appropriate icons based on room type
- Position the player sprite appropriately
This creates a much more interactive and user-friendly map navigation system.
111-120: Focus neighbors for keyboard/controller navigationThe
AddFocusNeighborsmethod creates a circular navigation pattern for the buttons, allowing users to navigate between map locations using keyboard or controller. This is a good accessibility feature.
122-140: Smooth stage transition with visual feedbackThe
EnterStagemethod provides a smooth transition between stages by:
- Disabling all buttons to prevent further interaction
- Creating a tween animation for the player sprite
- Using appropriate easing for natural movement
- Stopping music and transitioning to the new room when complete
This creates a polished user experience during stage transitions.
142-150: Win stage handlingThe
WinStagemethod properly handles the win condition by displaying an end screen with appropriate text and pausing the game tree.scenes/Remapping/Remap.tscn (5)
12-17: Control scheme mapping setupThe main node correctly exports references to the key sprites that will be updated when the control scheme changes. This provides a clean way to visualize the current control mappings.
19-30: Basic UI layout with titleThe panel and title label provide a clear structure for the remapping UI. The title "Choose Control Scheme" effectively communicates the purpose of this screen to the user.
55-63: Return to title button with scriptThe "Return to Title Screen" button is appropriately positioned at the bottom of the screen and has the SceneChange script attached for handling navigation.
70-92: Visual layout for control visualizationThe control visualization uses sprite nodes with appropriate positioning and rotation to represent directional controls (left, up, down, right). The layout is clear and provides a good visual representation of the controls.
93-123: Equal signs for mapping visualizationThe labels with "=" signs create a clear visual representation of the mapping between controls and their assigned keys. This makes the purpose of the remapping screen immediately clear to users.
scenes/BattleDirector/scripts/BattleDirector.cs (14)
31-33: Validate the initial_focusedButton.
Exporting_focusedButtonis convenient for hooking it up in the editor, but ensure_focusedButtonis never null at runtime in case the scene node is missing or renamed.
34-35: Revisit timing interval.
_timingInterval = 0.1seems small. Confirm that the user input detection or note accuracy mechanics align with this interval for the desired difficulty.
42-50: Good addition of return flags forPlayerAddNote.
Returningfalsewhen note placement fails helps the calling logic handle attempts properly. Make sure to test boundary cases (like maximum notes or special arrow types) to confirm no hidden edge-case.
67-72: Confirm valid audio resource paths.
Loading the audio stream fromStageProducer.Config.CurSong.AudioLocationis appropriate, but ensure the value is never null or an invalid string. Also, consider handling the case where.Stream.GetLength()might throw an error if the resource is not an audio file.
75-77: Instantiating Player Puppet
Looks good. Remember to check any references to the old initialization approach if replaced. Ensure thePlayernode is not duplicated in the scene.
89-95: Chart and conductor prep.
Linking upCM.PrepChart,CD.Prep, and input event connections are aligned with the normal Godot flow. Verify that all signals are properly disconnected if the node is freed early (rare but possible in dynamic transitions).
109-109: Initiating tweens
CM.BeginTweens();is an elegant way to start your chart animations. If you rely on timeline sync with the audio, confirm they remain in sync when the audio starts at line 110.
156-161: Miss logic upon empty note
WhenPlayerAddNotefails, you trigger a miss, combo text, and damage. This is consistent with typical rhythm game mechanics. Double-check that theMissNote()call also resets any sub-combo counters if you have them.
164-175: Timing feedback and combos
note.OnHit(this, timed)plus a combo text display is good. Keep an eye on the concurrency or potential for multiple notes at the same beat; ensure only correct hits are registered for each note.
209-214:BattleWonflow
Pausing the audio stream and cleaning relics is good. Ensure the relics or item logic doesn't conflict with any scene transitions.
223-231: Reward selection
ShowRewardSelection(3)is a convenient method. Validate that callingrewardSelect.Selected += EndBattle;does not conflict if multiple events set the same signal.
270-272: Add events for relics
CallingAddEvent(effect)for relic effects is a nice extension point. Make sure relic references remain valid if the relic is removed or changed mid-battle.
275-284: Cleanup
CleanUpRelics()properly ends relic effects. Ensure that any external references or listeners are unhooked if those relic effects trigger methods on other singletons.
17-18:Details
✅ Verification successful
Check for consistent usage of
EnemyPuppet.
Changing the type fromPuppetTemplatetoEnemyPuppetis logical if you need enemy-specific methods or properties. Just ensure that all references toEnemyacross the codebase are updated accordingly so that no type mismatches remain.Run a repository-wide search for
Enemyreferences to confirm the type usage:
🏁 Script executed:
#!/bin/bash rg -A 3 'Enemy[^A-Za-z0-9_]'Length of output: 3061
Consistent usage of
EnemyPuppetconfirmed.The repository-wide search shows that all references to the
Enemyproperty (e.g., calls toTakeDamage(), comparisons, and other interactions) are being correctly handled with the updated type. No type mismatches or outdated references were found.scenes/NoteManager/note_manager.tscn (5)
1-1: Updated resource paths and load steps
You've increasedload_stepsto 7, added new textures (new_arrow.png,arrow_outline.png), and a newhit_particles.tscn. Confirm these resources exist in the specified paths to avoid broken references in the final build.Also applies to: 4-4, 6-6, 8-8
15-25: Implementation of arrowLeft with Outline and HitParticles
Good approach: separating the main arrow texture from an outline node clarifies the visual layering. If further customizations or effects are needed, consider grouping them under a single parent node to prevent clutter.
28-38: arrowUp additions
Similar to arrowLeft, you introduced an Outline and HitParticles. Ensure the rotation-1.5708is correct for the arrow sprite orientation. If the arrow looks off, double-check the pivot or transform.
52-60: arrowRight
Similarly enhanced with position, new texture, outline, and particles. The approach is consistent across all directions.
65-65:dividersnode turned invisible
Hiding these UI elements might streamline the note area. Confirm that this was intentional and not a leftover debugging step.scenes/SceneTransitions/TitleScreen.tscn (6)
1-1:load_stepsincreased to 21
More external resources are incorporated. Ensure you’ve committed and included all new assets so that the loading sequence completes successfully.
4-6: New external resources
Multiple textures, shaders, and scenes are introduced. Keep your project folder organized, and validate correct licensing or usage constraints for any third-party assets.Also applies to: 9-13, 15-18
58-72: MoonLight & TextLight
ThesePointLight2Dnodes add nice ambiance. Validate that your 2D lighting engine is configured to handle them. If you add more lights, watch out for 2D performance.
108-118: StarShader overlay
Solid approach for starry backgrounds. The large offset/scale transformations might produce unexpected visuals on certain screen resolutions. Consider a dynamic approach if you handle varying window sizes.
184-184: Scene transition index
ScenePath = 5presumably loads a scene when "Start Game" is clicked. Confirm it references the correct scene for your start level.
239-247: GPUParticles2D
Nice starlike or floating particle effect.lifetime = 15.0with full randomness can produce interesting patterns. Keep an eye on performance in mobile or low-spec scenarios.
| if (COLOR.rgb == vec3(1)){ | ||
| if (rand(SCREEN_UV.xy / 20.0) > 0.996){ | ||
| COLOR.rbg = vec3(1); | ||
| }else { | ||
| vec3 gradient = mix(bg_top_color, bg_bottom_color, SCREEN_UV.y / gradient_ratio); | ||
| COLOR.rgb = gradient; | ||
| } |
There was a problem hiding this comment.
Fix RGB component order in the color assignment.
There's a typo in the color component order on line 27 - "rbg" should be "rgb".
- COLOR.rbg = vec3(1);
+ COLOR.rgb = vec3(1);📝 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.
| if (COLOR.rgb == vec3(1)){ | |
| if (rand(SCREEN_UV.xy / 20.0) > 0.996){ | |
| COLOR.rbg = vec3(1); | |
| }else { | |
| vec3 gradient = mix(bg_top_color, bg_bottom_color, SCREEN_UV.y / gradient_ratio); | |
| COLOR.rgb = gradient; | |
| } | |
| if (COLOR.rgb == vec3(1)){ | |
| if (rand(SCREEN_UV.xy / 20.0) > 0.996){ | |
| COLOR.rgb = vec3(1); | |
| }else { | |
| vec3 gradient = mix(bg_top_color, bg_bottom_color, SCREEN_UV.y / gradient_ratio); | |
| COLOR.rgb = gradient; | |
| } |
There was a problem hiding this comment.
So true. Also, we should remove the commented out stuff
There was a problem hiding this comment.
🧩 Analysis chain
@collectioncard I'll fix both issues! Looking at the file to find all commented-out code to remove.
🏁 Script executed:
#!/bin/bash
# View the complete content of the TitleFont.gdshader file
cat scenes/SceneTransitions/assets/TitleFont.gdshader
Length of output: 1316
@collectioncard I see the issues. The shader has both the rbg typo and several commented-out sections that should be removed. Here's the complete solution that fixes both:
shader_type canvas_item;
uniform float height = 5.0;
uniform float speed = 5.0f;
uniform float freq = 10.0f;
uniform vec3 bg_top_color;
uniform vec3 bg_bottom_color;
uniform float gradient_ratio;
uniform float time_scale;
float rand(vec2 st) {
return fract(sin(dot(st.xy, vec2(12.9898,78.233))) * 43758.5453123);
}
void vertex() {
VERTEX.y -= height * sin((TIME * speed) + (freq * UV.x));
}
void fragment() {
float color = 0.0f;
if (COLOR.rgb == vec3(1)){
if (rand(SCREEN_UV.xy / 20.0) > 0.996){
COLOR.rgb = vec3(1);
} else {
vec3 gradient = mix(bg_top_color, bg_bottom_color, SCREEN_UV.y / gradient_ratio);
COLOR.rgb = gradient;
}
}
}I've fixed the rbg typo at line 26 and removed all commented-out code, including the unused light() function and other comments. The shader should now be cleaner and work correctly.
| if (@event.IsActionPressed("Pause")) | ||
| { | ||
| var pauseMenu = GD.Load<PackedScene>("res://scenes/UI/Pause.tscn"); | ||
| AddChild(pauseMenu.Instantiate()); | ||
| GetTree().Paused = true; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Check if a pause menu already exists before creating a new one.
The current implementation creates a new pause menu every time the "Pause" action is pressed, potentially leading to multiple overlapping menus.
Add a check to prevent multiple pause menus from being created:
if (@event.IsActionPressed("Pause"))
{
+ // Check if a pause menu already exists
+ foreach (Node child in GetChildren())
+ {
+ if (child.Name.Contains("Pause"))
+ {
+ return; // Don't create another pause menu
+ }
+ }
var pauseMenu = GD.Load<PackedScene>("res://scenes/UI/Pause.tscn");
AddChild(pauseMenu.Instantiate());
GetTree().Paused = 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.
| if (@event.IsActionPressed("Pause")) | |
| { | |
| var pauseMenu = GD.Load<PackedScene>("res://scenes/UI/Pause.tscn"); | |
| AddChild(pauseMenu.Instantiate()); | |
| GetTree().Paused = true; | |
| } | |
| if (@event.IsActionPressed("Pause")) | |
| { | |
| + // Check if a pause menu already exists | |
| + foreach (Node child in GetChildren()) | |
| + { | |
| + if (child.Name.Contains("Pause")) | |
| + { | |
| + return; // Don't create another pause menu | |
| + } | |
| + } | |
| var pauseMenu = GD.Load<PackedScene>("res://scenes/UI/Pause.tscn"); | |
| AddChild(pauseMenu.Instantiate()); | |
| GetTree().Paused = true; | |
| } |
| if (@event.IsActionPressed("Inventory")) | ||
| { | ||
| var invenMenu = GD.Load<PackedScene>("res://scenes/UI/inventory.tscn") | ||
| .Instantiate<Inventory>(); | ||
| AddChild(invenMenu); | ||
| invenMenu.Display(StageProducer.PlayerStats ?? new PlayerStats()); //For now work around for testing | ||
| GetTree().Paused = true; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Check if an inventory is already open before creating a new one.
Similar to the pause menu, the current implementation creates a new inventory every time the "Inventory" action is pressed, potentially leading to multiple overlapping inventories.
Add a check to prevent multiple inventories from being created and consider safer scene loading:
if (@event.IsActionPressed("Inventory"))
{
+ // Check if an inventory already exists
+ foreach (Node child in GetChildren())
+ {
+ if (child is Inventory)
+ {
+ return; // Don't create another inventory
+ }
+ }
+
+ var inventoryScene = GD.Load<PackedScene>("res://scenes/UI/inventory.tscn");
+ if (inventoryScene == null)
+ {
+ GD.PrintErr("Failed to load inventory scene");
+ return;
+ }
- var invenMenu = GD.Load<PackedScene>("res://scenes/UI/inventory.tscn")
- .Instantiate<Inventory>();
+ var invenMenu = inventoryScene.Instantiate<Inventory>();
AddChild(invenMenu);
- invenMenu.Display(StageProducer.PlayerStats ?? new PlayerStats()); //For now work around for testing
+
+ // Use the null-coalescing operator to handle the null case
+ PlayerStats stats = StageProducer.PlayerStats ?? new PlayerStats();
+ invenMenu.Display(stats); //For now work around for testing
GetTree().Paused = 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.
| if (@event.IsActionPressed("Inventory")) | |
| { | |
| var invenMenu = GD.Load<PackedScene>("res://scenes/UI/inventory.tscn") | |
| .Instantiate<Inventory>(); | |
| AddChild(invenMenu); | |
| invenMenu.Display(StageProducer.PlayerStats ?? new PlayerStats()); //For now work around for testing | |
| GetTree().Paused = true; | |
| } | |
| if (@event.IsActionPressed("Inventory")) | |
| { | |
| // Check if an inventory already exists | |
| foreach (Node child in GetChildren()) | |
| { | |
| if (child is Inventory) | |
| { | |
| return; // Don't create another inventory | |
| } | |
| } | |
| var inventoryScene = GD.Load<PackedScene>("res://scenes/UI/inventory.tscn"); | |
| if (inventoryScene == null) | |
| { | |
| GD.PrintErr("Failed to load inventory scene"); | |
| return; | |
| } | |
| var invenMenu = inventoryScene.Instantiate<Inventory>(); | |
| AddChild(invenMenu); | |
| // Use the null-coalescing operator to handle the null case | |
| PlayerStats stats = StageProducer.PlayerStats ?? new PlayerStats(); | |
| invenMenu.Display(stats); //For now work around for testing | |
| GetTree().Paused = true; | |
| } |
| Particles = GetNode<HitParticles>("HitParticles"); | ||
| Particles.Modulate = color; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move node retrieval to _Ready() method for better performance.
Retrieving the HitParticles node in SetColor() causes an unnecessary node lookup each time the color is set. This is inefficient, especially if called frequently. Consider retrieving this node once during initialization.
public partial class NoteChecker : Sprite2D
{
private bool _isPressed;
private Color _color;
private float _fadeTime = 2.0f;
- public HitParticles Particles;
+ public HitParticles Particles { get; private set; }
+ public override void _Ready()
+ {
+ Particles = GetNode<HitParticles>("HitParticles");
+ }
// ... existing methods
public void SetColor(Color color)
{
_color = color;
- Particles = GetNode<HitParticles>("HitParticles");
Particles.Modulate = color;
SelfModulate = new Color(_color.R * 0.5f, _color.G * 0.5f, _color.B * 0.5f, 1);
}
}📝 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.
| Particles = GetNode<HitParticles>("HitParticles"); | |
| Particles.Modulate = color; | |
| public partial class NoteChecker : Sprite2D | |
| { | |
| private bool _isPressed; | |
| private Color _color; | |
| private float _fadeTime = 2.0f; | |
| public HitParticles Particles { get; private set; } | |
| public override void _Ready() | |
| { | |
| Particles = GetNode<HitParticles>("HitParticles"); | |
| } | |
| // ... existing methods | |
| public void SetColor(Color color) | |
| { | |
| _color = color; | |
| Particles.Modulate = color; | |
| SelfModulate = new Color(_color.R * 0.5f, _color.G * 0.5f, _color.B * 0.5f, 1); | |
| } | |
| } |
| public void Emit(int particleAmount) | ||
| { | ||
| // Apply the particle amount and start emitting | ||
| Amount = particleAmount; | ||
| Emitting = true; | ||
|
|
||
| // Stop particles after a short delay using a Timer | ||
| Timer timer = new Timer(); | ||
| timer.WaitTime = 0.25f; // Stop emitting after 0.5 seconds | ||
| timer.OneShot = true; | ||
| timer.Timeout += () => | ||
| { | ||
| Emitting = false; | ||
| timer.QueueFree(); // Clean up the timer | ||
| }; | ||
|
|
||
| AddChild(timer); | ||
| timer.Start(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address potential memory leaks and timing inconsistencies.
There are several issues with the timer handling:
- The comment on line 14 mentions 0.5 seconds, but the actual wait time is 0.25 seconds.
- The timer creates a potential memory leak through the lambda closure.
- If the HitParticles node is freed before the timer completes, accessing
Emittingcould cause errors.
public void Emit(int particleAmount)
{
// Apply the particle amount and start emitting
Amount = particleAmount;
Emitting = true;
// Stop particles after a short delay using a Timer
Timer timer = new Timer();
- timer.WaitTime = 0.25f; // Stop emitting after 0.5 seconds
+ timer.WaitTime = 0.25f; // Stop emitting after 0.25 seconds
timer.OneShot = true;
- timer.Timeout += () =>
- {
- Emitting = false;
- timer.QueueFree(); // Clean up the timer
- };
+ timer.Timeout += TimerTimeout;
AddChild(timer);
timer.Start();
}
+ private void TimerTimeout()
+ {
+ if (IsInstanceValid(this))
+ {
+ Emitting = false;
+ }
+
+ // Get reference to the timer that triggered this callback
+ Timer timer = GetNode<Timer>("Timer");
+ if (timer != null)
+ {
+ timer.QueueFree();
+ }
+ }Alternatively, consider using CreateTween() which is more modern and handles cleanup automatically:
public void Emit(int particleAmount)
{
// Apply the particle amount and start emitting
Amount = particleAmount;
Emitting = true;
- // Stop particles after a short delay using a Timer
- Timer timer = new Timer();
- timer.WaitTime = 0.25f; // Stop emitting after 0.5 seconds
- timer.OneShot = true;
- timer.Timeout += () =>
- {
- Emitting = false;
- timer.QueueFree(); // Clean up the timer
- };
-
- AddChild(timer);
- timer.Start();
+ // Stop particles after a short delay using a Tween
+ Tween tween = CreateTween();
+ tween.TweenCallback(Callable.From(() =>
+ {
+ Emitting = false;
+ })).SetDelay(0.25f);
}|
|
||
| //Populate the song data | ||
| songData = new SongData | ||
| { | ||
| //TODO: Allow for changes in this data | ||
| Bpm = 120, | ||
| //Fudge the numbers a bit if we have a really short song | ||
| SongLength = | ||
| _midiFile.GetDuration<MetricTimeSpan>().Seconds < 20 | ||
| ? 20 | ||
| : _midiFile.GetDuration<MetricTimeSpan>().Seconds, | ||
| NumLoops = 1, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract BPM from MIDI file instead of hardcoding.
The code currently hardcodes the BPM to 120, but it would be better to extract this information from the MIDI file's tempo map.
//Populate the song data
songData = new SongData
{
- //TODO: Allow for changes in this data
- Bpm = 120,
+ // Extract BPM from the MIDI file's tempo map
+ Bpm = (int)_midiFile.GetTempoMap().GetTempoAtTime(0).BeatsPerMinute,
//Fudge the numbers a bit if we have a really short song
SongLength =
_midiFile.GetDuration<MetricTimeSpan>().Seconds < 20
? 20
: _midiFile.GetDuration<MetricTimeSpan>().Seconds,
NumLoops = 1,
};📝 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.
| //Populate the song data | |
| songData = new SongData | |
| { | |
| //TODO: Allow for changes in this data | |
| Bpm = 120, | |
| //Fudge the numbers a bit if we have a really short song | |
| SongLength = | |
| _midiFile.GetDuration<MetricTimeSpan>().Seconds < 20 | |
| ? 20 | |
| : _midiFile.GetDuration<MetricTimeSpan>().Seconds, | |
| NumLoops = 1, | |
| }; | |
| } | |
| //Populate the song data | |
| songData = new SongData | |
| { | |
| // Extract BPM from the MIDI file's tempo map | |
| Bpm = (int)_midiFile.GetTempoMap().GetTempoAtTime(0).BeatsPerMinute, | |
| //Fudge the numbers a bit if we have a really short song | |
| SongLength = | |
| _midiFile.GetDuration<MetricTimeSpan>().Seconds < 20 | |
| ? 20 | |
| : _midiFile.GetDuration<MetricTimeSpan>().Seconds, | |
| NumLoops = 1, | |
| }; | |
| } |
|
|
||
| //Strip out the notes from the midi file | ||
| foreach (var track in _midiFile.GetTrackChunks()) | ||
| { | ||
| string trackName = track.Events.OfType<SequenceTrackNameEvent>().FirstOrDefault()?.Text; | ||
| midiNoteInfo[] noteEvents = track | ||
| .GetNotes() | ||
| .Select(note => new midiNoteInfo(note, _midiFile.GetTempoMap())) | ||
| .ToArray(); | ||
|
|
||
| switch (trackName) | ||
| { | ||
| case "Up": | ||
| _upNotes = noteEvents; | ||
| break; | ||
| case "Down": | ||
| _downNotes = noteEvents; | ||
| break; | ||
| case "Left": | ||
| _leftNotes = noteEvents; | ||
| break; | ||
| case "Right": | ||
| _rightNotes = noteEvents; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider handling missing MIDI tracks.
The code assumes that the MIDI file will have tracks named "Up", "Down", "Left", and "Right", but doesn't handle the case where some tracks might be missing.
foreach (var track in _midiFile.GetTrackChunks())
{
string trackName = track.Events.OfType<SequenceTrackNameEvent>().FirstOrDefault()?.Text;
+ if (string.IsNullOrEmpty(trackName))
+ {
+ GD.PrintWarn($"Track without name found in MIDI file, skipping");
+ continue;
+ }
midiNoteInfo[] noteEvents = track
.GetNotes()
.Select(note => new midiNoteInfo(note, _midiFile.GetTempoMap()))
.ToArray();
switch (trackName)
{
case "Up":
_upNotes = noteEvents;
break;
case "Down":
_downNotes = noteEvents;
break;
case "Left":
_leftNotes = noteEvents;
break;
case "Right":
_rightNotes = noteEvents;
break;
+ default:
+ GD.PrintWarn($"Unknown track name '{trackName}' in MIDI file, skipping");
+ break;
}
}
+
+// Initialize any missing tracks with empty arrays
+_upNotes ??= Array.Empty<midiNoteInfo>();
+_downNotes ??= Array.Empty<midiNoteInfo>();
+_leftNotes ??= Array.Empty<midiNoteInfo>();
+_rightNotes ??= Array.Empty<midiNoteInfo>();📝 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.
| //Strip out the notes from the midi file | |
| foreach (var track in _midiFile.GetTrackChunks()) | |
| { | |
| string trackName = track.Events.OfType<SequenceTrackNameEvent>().FirstOrDefault()?.Text; | |
| midiNoteInfo[] noteEvents = track | |
| .GetNotes() | |
| .Select(note => new midiNoteInfo(note, _midiFile.GetTempoMap())) | |
| .ToArray(); | |
| switch (trackName) | |
| { | |
| case "Up": | |
| _upNotes = noteEvents; | |
| break; | |
| case "Down": | |
| _downNotes = noteEvents; | |
| break; | |
| case "Left": | |
| _leftNotes = noteEvents; | |
| break; | |
| case "Right": | |
| _rightNotes = noteEvents; | |
| break; | |
| } | |
| } | |
| //Strip out the notes from the midi file | |
| foreach (var track in _midiFile.GetTrackChunks()) | |
| { | |
| string trackName = track.Events.OfType<SequenceTrackNameEvent>().FirstOrDefault()?.Text; | |
| if (string.IsNullOrEmpty(trackName)) | |
| { | |
| GD.PrintWarn($"Track without name found in MIDI file, skipping"); | |
| continue; | |
| } | |
| midiNoteInfo[] noteEvents = track | |
| .GetNotes() | |
| .Select(note => new midiNoteInfo(note, _midiFile.GetTempoMap())) | |
| .ToArray(); | |
| switch (trackName) | |
| { | |
| case "Up": | |
| _upNotes = noteEvents; | |
| break; | |
| case "Down": | |
| _downNotes = noteEvents; | |
| break; | |
| case "Left": | |
| _leftNotes = noteEvents; | |
| break; | |
| case "Right": | |
| _rightNotes = noteEvents; | |
| break; | |
| default: | |
| GD.PrintWarn($"Unknown track name '{trackName}' in MIDI file, skipping"); | |
| break; | |
| } | |
| } | |
| // Initialize any missing tracks with empty arrays | |
| _upNotes ??= Array.Empty<midiNoteInfo>(); | |
| _downNotes ??= Array.Empty<midiNoteInfo>(); | |
| _leftNotes ??= Array.Empty<midiNoteInfo>(); | |
| _rightNotes ??= Array.Empty<midiNoteInfo>(); |
| public override void _ExitTree() | ||
| { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Stop only relevant tweens.
In _ExitTree(), you currently stop all processed tweens in the scene tree, which could inadvertently halt other tweens. Storing a reference to your own tween(s) and stopping only those is safer.
- foreach (var tween in GetTree().GetProcessedTweens())
- {
- tween.Stop();
- }
+ // Store the tween in a member variable when creating it:
+ // _myTween = GetTree().CreateTween();
+
+ _myTween?.Stop();📝 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.
| public override void _ExitTree() | |
| { | |
| public override void _ExitTree() | |
| { | |
| // Store the tween in a member variable when creating it: | |
| // _myTween = GetTree().CreateTween(); | |
| _myTween?.Stop(); | |
| // ... any remaining _ExitTree cleanup code | |
| } |
| _particles = NodePath("ProgressBar/Rock") | ||
| _currentNote = NodePath("NoteQueueSprite/NextNote") | ||
| _nextNote = NodePath("NoteQueueSprite/CurrentNote") | ||
| fullBarParticles = NodePath("ProgressBar/CPUParticles2D") |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the swapped note references
The _currentNote and _nextNote node paths appear to have been swapped, which may affect functionality if not properly addressed in the accompanying script.
🏁 Script executed:
#!/bin/bash
# Description: Check if there are references to these nodes in other scripts that might need updating
# Search for references to _currentNote and _nextNote in CS files
echo "Checking references to _currentNote and _nextNote in CS files:"
rg -A 1 "_currentNote" --glob "*.cs"
rg -A 1 "_nextNote" --glob "*.cs"Length of output: 1789
Action Required: Correct the NodePath Assignments
It appears that the assignments in scenes/BattleDirector/NotePlacementBar.tscn are reversed. The scene file currently sets:
_currentNote = NodePath("NoteQueueSprite/NextNote")_nextNote = NodePath("NoteQueueSprite/CurrentNote")
However, the corresponding C# file (scenes/BattleDirector/scripts/NotePlacementBar.cs) uses _currentNote and _nextNote in a way that clearly expects _currentNote to reference the "CurrentNote" node and _nextNote to reference the "NextNote" node.
Suggested Correction:
- Update the assignments in the scene file so that:
_currentNote = NodePath("NoteQueueSprite/CurrentNote")_nextNote = NodePath("NoteQueueSprite/NextNote")
This change will align the scene with the expectations in the code and ensure the node references are correct.
| _focusedButton?.GrabFocus(); | ||
| TimeKeeper.CurrentTime = Audio.GetPlaybackPosition(); | ||
| CD.CheckMiss(); | ||
| if (_battleEnd) | ||
| GetNode<StageProducer>("/root/StageProducer").TransitionStage(Stages.Map); | ||
| double realBeat = TimeKeeper.CurrentTime / (60 / (double)TimeKeeper.Bpm) % CM.BeatsPerLoop; | ||
| CD.CheckMiss(realBeat); | ||
| if (realBeat < _lastBeat) | ||
| ChartLooped?.Invoke(this); | ||
| _lastBeat = realBeat; | ||
| } |
There was a problem hiding this comment.
Focus grab loop
For _Process, repeatedly calling _focusedButton?.GrabFocus() might overshadow user interactions if it continues throughout the battle. Consider gating this to happen once or until the user transitions.
- _focusedButton?.GrabFocus();
+ if (!_focusedButtonHasFocused)
+ {
+ _focusedButton?.GrabFocus();
+ _focusedButtonHasFocused = 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.
| _focusedButton?.GrabFocus(); | |
| TimeKeeper.CurrentTime = Audio.GetPlaybackPosition(); | |
| CD.CheckMiss(); | |
| if (_battleEnd) | |
| GetNode<StageProducer>("/root/StageProducer").TransitionStage(Stages.Map); | |
| double realBeat = TimeKeeper.CurrentTime / (60 / (double)TimeKeeper.Bpm) % CM.BeatsPerLoop; | |
| CD.CheckMiss(realBeat); | |
| if (realBeat < _lastBeat) | |
| ChartLooped?.Invoke(this); | |
| _lastBeat = realBeat; | |
| } | |
| if (!_focusedButtonHasFocused) | |
| { | |
| _focusedButton?.GrabFocus(); | |
| _focusedButtonHasFocused = true; | |
| } | |
| TimeKeeper.CurrentTime = Audio.GetPlaybackPosition(); | |
| double realBeat = TimeKeeper.CurrentTime / (60 / (double)TimeKeeper.Bpm) % CM.BeatsPerLoop; | |
| CD.CheckMiss(realBeat); | |
| if (realBeat < _lastBeat) | |
| ChartLooped?.Invoke(this); | |
| _lastBeat = realBeat; | |
| } |
There was a problem hiding this comment.
We might want to change this to something other than webp in the future
collectioncard
left a comment
collectioncard
left a comment
There was a problem hiding this comment.
Looks mostly good but title screen font shader should be cleaned up
| if (COLOR.rgb == vec3(1)){ | ||
| if (rand(SCREEN_UV.xy / 20.0) > 0.996){ | ||
| COLOR.rbg = vec3(1); | ||
| }else { | ||
| vec3 gradient = mix(bg_top_color, bg_bottom_color, SCREEN_UV.y / gradient_ratio); | ||
| COLOR.rgb = gradient; | ||
| } |
There was a problem hiding this comment.
So true. Also, we should remove the commented out stuff
Sprint 3 complete. Pog
Summary by CodeRabbit
New Features
UI & UX Enhancements