Sprint 2 Complete #52
Conversation
Initial base template for battle entities Also some file reorganization
Moved enums and structs to common namespace Removed note resource class needing beat and type (moving forward note data shouldn't hold that.) Added IsNoteActive() Removed unused code and parameters Removed unnecessary textparticle packedscene
Made a scene with two buttons; one will transition to our test battle scene, the other exits the game. The battle scene now has a button in the upper left that will return to the main menu. Makes use of a SceneChange script, which allows for a path to be set for the button in the inspector(doesn't need quotation marks), or exit to make the button quit the game. I noticed a infinite loop warning when exiting battle scene (the tweens don;t seem to stop if we exit), had to make minor change to ChartManager with an exitTree function to stop the tweens when we leave
Replaced old arrow graphic with new custom one Player arrow is unique color Reverted CheckMiss(), upon looping, without input no notes would miss Player puppet with WIP player stats object. RelicTemplate class to placehold for inventory
Created battle event interface and relic handling Moved lane and note handling into conductor BattleDirector is moving towards being more oriented towards handling battle events and handling
Note handling is interesting.
Created Scribe to hold dictionaries made note dictionary Timing enum
Scene transition
Also played with control nodes, surprisingly didn't take an hour to do what I wanted.
two notes will be displayed to the player, the one in the circle will be the next note that will be placed. currently supports "single" and "double" notes, where double notes are supposed to eventually give double points.
Begin merging upstream: NotesnRelics into Inventory
Inventory into Sprint 2
SaveData.json can store all our game's save data (unless we want to split the save data amongst different files). NoteQueue now pulls notes from the SaveData.
made a static rewards class that should assign a new relic, made minor changes to battle director to check health of puppets and change the apporpiate bool when they die, added a debug function to instantly kill the enemy to make testing the reward drop easier, added dummy relic
New approach that allows for player to choose from a selection of relics after the battle is won
NoteArrow has been given a variable of type Note to represent what special note is held in there. Notes created by the chart or enemy are assigned the type enemy, while notes placed by the player are assigned based on the noteQueue. Also, the queue doesn't "loop" so when the player runs out, they can no longer place notes.
Reward system
Distributed functions to more cohesive classes Removed Reward class Added PlayerStats into StageProducer, so its global and can be persistent. Refined battle status checking
Added note queue functionality into note placement bar, for better cohesivity Simplified loading and setting notequeue Replaced overall note array and noteIdx from conductor for note arrows holding a reference to their notes. Saving save system for later, for possible reuse
Start battle on start button press
Winning battle-> relic selection continue Lose -> Reset and return to title screen Changed clone functions, to fix crash on GC.
Tweaked wip relics and map for better functionality to get a feel for
Begin Sprint 2 wrap-up
There was a problem hiding this comment.
Variables are fine. Will use them later
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis update removes several legacy classes such as the old Note class and BattleDirector components, and introduces new implementations for note handling, relic effects, stage management, and various UI components. New asset import configurations and project file modifications are applied. In addition, new classes for managing game stages, player stats, and inventory, as well as updated scene structures for battles, maps, and transitions, have been added while several redundant files have been removed. Changes
Sequence Diagram(s)sequenceDiagram
participant P as Player
participant TS as TitleScreen (SceneChange)
participant SP as StageProducer
participant MG as MapGrid/Transition
P->>TS: Click "Start" button
TS->>SP: Call OnButtonPressed() with ScenePath
SP->>MG: TransitionStage() invoked
MG-->>P: New scene loaded
sequenceDiagram
participant N as Note (new)
participant BD as BattleDirector (scripts)
participant C as Conductor
participant NP as NotePlacementBar
N->>BD: Trigger OnHit()
BD->>C: Forward note timing & event processing
C->>NP: Update UI and combo feedback
NP-->>BD: Confirm note processed
Poem
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (47)
Globals/StageProducer.cs (2)
31-31: Remove or utilize_curRoomif it's no longer needed.The
_curRoomfield is assigned but never used, as indicated by static analysis. Consider removing it or using it in your logic to avoid confusion and keep the code clean.- private int _curRoom = 0;🧰 Tools
🪛 GitHub Check: build
[warning] 31-31:
The field 'StageProducer.MapGrid._curRoom' is assigned but its value is never used
[warning] 31-31:
The field 'StageProducer.MapGrid._curRoom' is assigned but its value is never used
154-177: Consider encapsulating scene transitions in a state machine or dictionary-based approach.Relying on a switch statement is acceptable, but as more stages or transitions are added, a state machine or dictionary-based approach can simplify transition logic, reduce code duplication, and improve extensibility.
scenes/BattleDirector/scripts/BattleDirector.cs (2)
41-49: Refine note placement logging and event handling.Currently, a simple
GD.Print("Note Placed.")is used when a note is successfully placed. Consider a more robust or configurable logging mechanism for debugging or analytics, particularly if you plan to handle a large number of notes.
212-228: Plan for handling multiple enemies.The
CheckBattleStatusmethod specifically checks for a singleEnemy. If you anticipate multiple enemies in future content, consider refactoring the victory/defeat logic to iterate over or track multiple enemies to accommodate more complex battle scenarios.scenes/UI/scripts/DisplayButton.cs (1)
16-22: Avoid using the parameter namenameto prevent confusion with the base class property.Renaming the parameter can make your code clearer and reduce potential conflicts with
Button.Name.- public void Display(Texture2D texture, string description, string name) + public void Display(Texture2D texture, string description, string displayName)scenes/SceneTransitions/scripts/SceneChange.cs (3)
5-11: Follow C# naming conventions for private fields.The private field
_startFocuseduses an underscore prefix. While this is a common convention in some languages, C# typically uses camelCase for private fields without underscores.- private bool _startFocused = false; + private bool startFocused = false;
13-21: Consider removing or guarding debug prints.Debug print statements should typically be removed or guarded in production code.
- GD.Print($"[DEBUG] Scene Path: '{ScenePath}'"); +#if DEBUG + GD.Print($"[DEBUG] Scene Path: '{ScenePath}'"); +#endif
23-27: Cache the StageProducer reference for better performance.Using
GetNodein event handlers can impact performance. Consider caching theStageProducerreference in_Ready.public partial class SceneChange : Button { [Export] public Stages ScenePath; [Export] private bool _startFocused = false; + + private StageProducer stageProducer; public override void _Ready() { if (_startFocused) { GrabFocus(); } Pressed += OnButtonPressed; + stageProducer = GetNode<StageProducer>("/root/StageProducer"); GD.Print($"[DEBUG] Scene Path: '{ScenePath}'"); } private void OnButtonPressed() { GD.Print($"✅ Loading scene: {ScenePath}"); - GetNode<StageProducer>("/root/StageProducer").TransitionStage(ScenePath); + stageProducer.TransitionStage(ScenePath); } }scenes/Puppets/scripts/HealthBar.cs (1)
12-16: Add input validation for health values.Consider adding validation to ensure current health doesn't exceed max health and that values are non-negative.
public void SetHealth(int max, int current) { + if (max < 0) + throw new ArgumentException("Max health cannot be negative", nameof(max)); + if (current < 0) + throw new ArgumentException("Current health cannot be negative", nameof(current)); + if (current > max) + throw new ArgumentException("Current health cannot exceed max health", nameof(current)); MaxValue = max; Value = current; }Classes/Relics/RelicTemplate.cs (1)
13-24: Use consistent parameter naming.The parameter name
EffectTagsdiffers from the field nameEffects. Consider using consistent naming.public RelicTemplate( string name = "", string tooltip = "", Texture2D texture = null, - RelicEffect[] EffectTags = null + RelicEffect[] effects = null ) { - Effects = EffectTags; + Effects = effects; Name = name; Tooltip = tooltip; Texture = texture; }Classes/Relics/RelicEffect.cs (2)
11-20: Add input validation in constructor.Consider validating input parameters to ensure the effect is properly initialized.
public RelicEffect( BattleEffectTrigger trigger, int val, Action<BattleDirector, int> onRelicEffect ) { + if (onRelicEffect == null) + throw new ArgumentNullException(nameof(onRelicEffect)); + BaseValue = val; Trigger = trigger; OnRelicEffect = onRelicEffect; }
27-30: Consider exposing Trigger as a public property instead of using a getter method.The GetTrigger method is redundant when you could simply expose the Trigger property as public.
-private BattleEffectTrigger Trigger { get; set; } +public BattleEffectTrigger Trigger { get; set; } -public BattleEffectTrigger GetTrigger() -{ - return Trigger; -}scenes/Puppets/scripts/PlayerPuppet.cs (1)
8-19: Reorder _Ready method for proper initialization.The base._Ready() call should come before custom initialization to ensure proper setup order.
public override void _Ready() { + base._Ready(); + Stats = StageProducer.PlayerStats; _currentHealth = Stats.CurrentHealth; _maxHealth = Stats.MaxHealth; Init(GD.Load<Texture2D>("res://scenes/BattleDirector/assets/Character1.png"), "Player"); SetPosition(new Vector2(80, 0)); Sprite.Position += Vector2.Down * 30; //TEMP - base._Ready(); }Globals/FunkEngineNameSpace.cs (3)
5-11: Add XML documentation for enums.Adding XML documentation will improve code maintainability and IDE support.
+/// <summary> +/// Represents the different types of arrows in the game. +/// </summary> public enum ArrowType { + /// <summary>Up arrow type.</summary> Up = 0, + /// <summary>Down arrow type.</summary> Down = 1, + /// <summary>Left arrow type.</summary> Left = 2, + /// <summary>Right arrow type.</summary> Right = 3, }
53-57: Enhance IBattleEvent interface with additional context.The interface could be more robust by including event context and validation.
public interface IBattleEvent { - void OnTrigger(BattleDirector BD); + /// <summary> + /// Triggered when a battle event occurs. + /// </summary> + /// <param name="director">The battle director instance.</param> + /// <returns>True if the event was handled successfully.</returns> + bool OnTrigger(BattleDirector director); + + /// <summary> + /// Gets the trigger type for this battle event. + /// </summary> BattleEffectTrigger GetTrigger(); + + /// <summary> + /// Validates if the event can be triggered in the current context. + /// </summary> + bool CanTrigger(BattleDirector director); }
45-51: Consider making ArrowData immutable.The struct should be immutable to prevent unintended modifications and improve thread safety.
public struct ArrowData { - public Color Color; - public string Key; - public NoteChecker Node; - public ArrowType Type; + public Color Color { get; } + public string Key { get; } + public NoteChecker Node { get; } + public ArrowType Type { get; } + + public ArrowData(Color color, string key, NoteChecker node, ArrowType type) + { + Color = color; + Key = key; + Node = node; + Type = type; + } }scenes/SceneTransitions/testTransition.tscn (2)
11-11: Remove unused Node2D.The
Node2Dnode appears to serve no purpose in this scene.-[node name="Node2D" type="Node2D" parent="."]
13-22: Consider using Container nodes for better layout.The current implementation uses absolute positioning which may not scale well across different screen sizes. Consider using Container nodes (e.g., VBoxContainer, MarginContainer) for better layout management.
-[node name="StartButton" type="Button" parent="."] -layout_mode = 0 -offset_left = 120.0 -offset_top = 56.0 -offset_right = 128.0 -offset_bottom = 64.0 -scale = Vector2(2.48, 2.48) +[node name="VBoxContainer" type="VBoxContainer" parent="."] +layout_mode = 1 +anchors_preset = 8 +anchor_left = 0.5 +anchor_top = 0.5 +anchor_right = 0.5 +anchor_bottom = 0.5 + +[node name="StartButton" type="Button" parent="VBoxContainer"] +layout_mode = 2 +size_flags_horizontal = 4scenes/Puppets/scripts/PlayerStats.cs (1)
15-15: Address the Clone method TODO comment.Consider implementing a proper deep clone method or using a factory pattern for Note creation.
Would you like me to help implement a more robust solution for Note creation?
scenes/Puppets/HealthBar.tscn (2)
9-11: Consider using relative dimensions for better scaling.The fixed dimensions (150x20 and 146x16) might not scale well across different resolutions. Consider using relative sizing or container-based layouts.
Also applies to: 17-19
13-14: Enhance color accessibility.The current color scheme (green to white) might not be optimal for colorblind users. Consider using a more accessible color palette and potentially adding a visual indicator beyond color.
[sub_resource type="Gradient" id="Gradient_soqhm"] -colors = PackedColorArray(0, 1, 0.0999999, 1, 1, 1, 1, 1) +colors = PackedColorArray(0.2, 0.8, 0.2, 1, 0.8, 0.2, 0.2, 1) # More distinct colorsSaveData/SaveSystem.cs (2)
6-6: TODO: Implement saving functionality.The class currently only handles loading data. Consider implementing the save functionality to complete the save system.
Would you like me to help implement the save functionality or create an issue to track this task?
10-10: Consider making the save path configurable.The hard-coded save path might not be suitable for all deployment scenarios. Consider making it configurable through project settings or environment variables.
- private static string SavePath => "res://SaveData/SaveData.json"; // Update if needed + private static string SavePath => ProjectSettings.GetSetting("game/save_data_path", "res://SaveData/SaveData.json").ToString();Classes/Notes/Note.cs (1)
48-54: Consider implementing deep copy in Clone method.The current implementation uses shallow copy, which might cause issues if the note effect or owner references need to be unique per clone.
Consider implementing a deep copy mechanism if needed:
public Note Clone() { - //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 - Note newNote = new Note(Name, Tooltip, Texture, Owner, _baseVal, NoteEffect); + Note newNote = new Note( + Name, + Tooltip, + Texture, + Owner?.Clone(), // Assuming PuppetTemplate has Clone method + _baseVal, + NoteEffect?.Clone() as Action<BattleDirector, Note, Timing> // Deep copy the delegate if needed + ); return newNote; }scenes/Maps/scripts/Cartographer.cs (2)
13-16: Extract magic numbers into constants.The position calculation uses hard-coded values that should be extracted into named constants for better maintainability.
+ private const float ROOM_WIDTH_SCALE = 640f; + private const float ROOM_X_OFFSET = 64f; + private const float ROOM_Y_SCALE = 48f; + private const float ROOM_Y_OFFSET = 16f; + private Vector2 GetPosition(int x, int y) { - return new Vector2((float)x * 640 / StageProducer.MapSize.X - 1 + 64, y * 48 + 16); + return new Vector2( + x * ROOM_WIDTH_SCALE / StageProducer.MapSize.X - 1 + ROOM_X_OFFSET, + y * ROOM_Y_SCALE + ROOM_Y_OFFSET + ); }
52-52: TODO: Implement room type icons.The current implementation uses a default icon. Room type-specific icons should be implemented.
Would you like me to help implement the room type icons system or create an issue to track this task?
scenes/NoteManager/scripts/InputHandler.cs (1)
67-67: Consider using strongly-typed events instead of integer casting.Casting
arrow.Typeto int in signal emission could lead to type safety issues. Consider updating the signal delegates to useArrowTypedirectly instead of integers.-EmitSignal(nameof(NotePressed), (int)arrow.Type); +EmitSignal(nameof(NotePressed), arrow.Type); -EmitSignal(nameof(NoteReleased), (int)arrow.Type); +EmitSignal(nameof(NoteReleased), arrow.Type);Also applies to: 72-72
scenes/UI/scripts/RewardSelect.cs (1)
39-41: Consider implementing dynamic relic rewards.The TODO comment suggests implementing variable relic rewards based on enemy type. This is a good enhancement to consider for gameplay balance.
Would you like me to help implement a reward scaling system based on enemy difficulty?
scenes/UI/scripts/Inventory.cs (1)
63-63: Review the inventory cleanup approach.The comment suggests that using QueueFree() might be hacky. Consider implementing a proper cleanup method that handles resource disposal and UI state.
Would you like me to propose a more robust cleanup solution that integrates with Godot's scene management?
scenes/Puppets/scripts/PuppetTemplate.cs (3)
4-6: Consider implementing IHealth interface.The TODO comment about interfaces is valid. Creating an IHealth interface would improve code flexibility and allow for different implementations of health-based entities.
+public interface IHealth +{ + int MaxHealth { get; } + int CurrentHealth { get; } + void TakeDamage(int amount); + void Heal(int amount); + event Action<IHealth> OnDefeated; +} -public partial class PuppetTemplate : Node2D +public partial class PuppetTemplate : Node2D, IHealth
15-16: Extract magic numbers to constants or configuration.The health values are hardcoded. Consider moving these to constants or making them configurable through the editor.
- protected int _maxHealth = 100; - protected int _currentHealth = 100; + [Export] + protected int _maxHealth = 100; + + protected int _currentHealth; + + public override void _Ready() + { + _currentHealth = _maxHealth; + // ... rest of the code + }
29-29: Consider implementing scene-based puppet system.The TODO comment suggests moving to packed scenes or robust subclasses. This would improve maintainability and allow for better puppet customization.
Would you like me to propose a scene-based architecture for different puppet types?
scenes/BattleDirector/NotePlacementBar.tscn (2)
4-6: Consider reorganizing asset paths.Note-related assets are split between two directories:
scenes/BattleDirector/assets/Classes/Notes/assets/Consider consolidating them in a single location for better maintainability.
63-74: Good implementation of the note queue preview.The note queue visualization with current and next notes improves gameplay readability and helps players prepare for upcoming patterns.
However, consider adding tooltips or visual indicators to help new players understand the queue system.
[node name="CurrentNote" type="Sprite2D" parent="NoteQueueSprite"] position = Vector2(-14, -1) texture = ExtResource("3_6ylx6") +tooltip_text = "Current Note"scenes/UI/inventory.tscn (1)
91-96: Consider enhancing description readability.The description label has good text wrapping and clipping settings, but consider:
- Adding minimum padding
- Adjusting text size for better readability
[node name="Description" type="Label" parent="InvenVBox/DescBox"] layout_mode = 2 size_flags_vertical = 1 +theme_override_font_sizes/font_size = 14 +theme_override_constants/margin_left = 5 +theme_override_constants/margin_right = 5 autowrap_mode = 2scenes/UI/Pause.tscn (1)
73-77: Consider removing or implementing the placeholder button.The "Quit to Title" button is marked as a placeholder. Either implement its functionality or remove it to avoid confusing users.
Would you like me to help implement the "Quit to Title" functionality or create an issue to track this task?
scenes/UI/RewardSelectionUI.tscn (2)
32-34: Consider adjusting ScrollContainer size properties.The ScrollContainer might need horizontal size flags to ensure proper content width adjustment.
[node name="ScrollContainer" type="ScrollContainer" parent="MarginContainer/PanelContainer/VBoxContainer"] layout_mode = 2 size_flags_vertical = 3 +size_flags_horizontal = 3
57-63: Refine text overflow handling for Description label.The current text overflow behavior might truncate important information. Consider using text wrapping with ellipsis.
[node name="Description" type="Label" parent="MarginContainer/PanelContainer/VBoxContainer/DescBox"] layout_mode = 2 size_flags_vertical = 1 autowrap_mode = 2 clip_text = true -text_overrun_behavior = 1 +text_overrun_behavior = 3 # Use ellipsisscenes/SceneTransitions/TitleScreen.tscn (1)
75-91: Review experimental feature visibility.The Load Game button is hidden but still contains a full implementation. Consider:
- Removing the button completely if the feature isn't ready
- Using a feature flag system for experimental features
- Adding a visual indicator for experimental features when visible
Globals/Scribe.cs (2)
54-55: Address TODO comment about relic descriptions.The TODO comment indicates that relic descriptions should include values. Consider implementing a description format that includes the effect values.
Would you like me to help implement a description formatter that includes the relic effect values?
89-106: Optimize GetRandomRelics method.The current implementation creates multiple arrays and has potential memory allocation overhead.
Consider using a more efficient approach:
public static RelicTemplate[] GetRandomRelics(RelicTemplate[] ownedRelics, int count) { - var availableRelics = Scribe - .RelicDictionary.Where(r => !ownedRelics.Any(o => o.Name == r.Name)) - .ToArray(); + var ownedNames = new HashSet<string>(ownedRelics.Select(r => r.Name)); + var availableRelics = RelicDictionary + .Where(r => !ownedNames.Contains(r.Name)) + .OrderBy(_ => StageProducer.GlobalRng.Randi()) + .Take(count) + .Select(r => r.Clone()) + .ToList(); - availableRelics = availableRelics - .OrderBy(_ => StageProducer.GlobalRng.Randi()) - .Take(count) - .Select(r => r.Clone()) - .ToArray(); while (availableRelics.Count < count) { - availableRelics = availableRelics.Append(RelicDictionary[0].Clone()).ToArray(); + availableRelics.Add(RelicDictionary[0].Clone()); } - return availableRelics; + return availableRelics.ToArray(); }project.godot (1)
61-70: Add controller support for Pause and Inventory actions.Consider adding gamepad button mappings for better controller support.
Add these events to the respective input mappings:
- Pause: Add gamepad "Start" button
- Inventory: Add gamepad "Select" button
scenes/BattleDirector/scripts/NotePlacementBar.cs (2)
69-84: Add thread safety to queue operations.The
ProgressQueuemethod has potential race conditions when accessing the note queue. Consider adding synchronization.+private readonly object _queueLock = new object(); private void ProgressQueue() { + lock (_queueLock) + { if (_noteQueue.Count == 0) { ShuffleNoteQueue(); } if (_currentNoteInstance == null) { _currentNoteInstance = _noteQueue.Dequeue(); if (_noteQueue.Count == 0) { ShuffleNoteQueue(); } } UpdateNoteQueue(); + } }
115-123: Consider caching combo multiplier calculations.The combo multiplier calculation could be optimized by caching results and only recalculating when necessary.
+private int _lastComboForMult = -1; public void HitNote() { _currentCombo++; - DetermineComboMult(); + if (_currentCombo / notesToIncreaseCombo != _lastComboForMult) + { + DetermineComboMult(); + _lastComboForMult = _currentCombo / notesToIncreaseCombo; + } _currentBarValue += comboMult; UpdateNotePlacementBar(_currentBarValue); UpdateComboMultText(); }scenes/ChartViewport/ChartManager.cs (2)
131-140: Improve tween cleanup logging.The debug logging in
_ExitTreecould be more informative by including tween identifiers.public override void _ExitTree() { - GD.Print("[DEBUG] Stopping tweens before exiting the scene..."); + GD.Print($"[DEBUG] Stopping {GetTree().GetProcessedTweens().Count()} tweens before exiting the scene..."); foreach (var tween in GetTree().GetProcessedTweens()) { tween.Stop(); - GD.Print("[DEBUG] Stopped tween."); + GD.Print($"[DEBUG] Stopped tween {tween.GetInstanceId()}"); } }
58-75: Consider pooling tweens for better performance.Creating new tweens frequently can impact performance. Consider implementing a tween pool.
Would you like me to provide an implementation of a tween pool system to improve performance?
scenes/BattleDirector/scripts/Conductor.cs (1)
134-136: Remove or enhance debug print statement.The debug print statement should either be removed or enhanced with more context for better debugging.
-GD.Print("Cur beat " + curBeat + "Real: " + realBeat.ToString("#.###")); +if (OS.IsDebugBuild()) +{ + GD.Print($"[Conductor] Beat timing - Current: {curBeat}, Real: {realBeat:F3}, Difference: {Math.Abs(realBeat - curBeat):F3}"); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
Classes/Notes/assets/double_note.pngis excluded by!**/*.pngClasses/Notes/assets/single_note.pngis excluded by!**/*.pngClasses/Relics/assets/relic_Breakfast.pngis excluded by!**/*.pngClasses/Relics/assets/relic_GoodVibes.pngis excluded by!**/*.pngscenes/BattleDirector/assets/Character1.pngis excluded by!**/*.pngscenes/BattleDirector/assets/CoolBG.jpgis excluded by!**/*.jpgscenes/BattleDirector/assets/temp_note_queue.pngis excluded by!**/*.pngscenes/NoteManager/assets/outline_white.pngis excluded by!**/*.pngscenes/NoteManager/assets/right-arrow.pngis excluded by!**/*.png
📒 Files selected for processing (54)
Classes/Note.cs(0 hunks)Classes/Notes/Note.cs(1 hunks)Classes/Notes/assets/double_note.png.import(1 hunks)Classes/Notes/assets/single_note.png.import(1 hunks)Classes/Relics/RelicEffect.cs(1 hunks)Classes/Relics/RelicTemplate.cs(1 hunks)Classes/Relics/assets/relic_Breakfast.png.import(1 hunks)Classes/Relics/assets/relic_GoodVibes.png.import(1 hunks)Funk Engine.csproj(1 hunks)Globals/FunkEngineNameSpace.cs(1 hunks)Globals/Scribe.cs(1 hunks)Globals/StageProducer.cs(1 hunks)README.md(0 hunks)SaveData/SaveData.json(1 hunks)SaveData/SaveSystem.cs(1 hunks)project.godot(2 hunks)scenes/BattleDirector/BattleDirector.cs(0 hunks)scenes/BattleDirector/HealthBar.cs(0 hunks)scenes/BattleDirector/HealthBar.tscn(0 hunks)scenes/BattleDirector/NotePlacementBar.cs(0 hunks)scenes/BattleDirector/NotePlacementBar.tscn(4 hunks)scenes/BattleDirector/TextParticle.tscn(0 hunks)scenes/BattleDirector/assets/temp_note_queue.png.import(1 hunks)scenes/BattleDirector/scripts/BattleDirector.cs(1 hunks)scenes/BattleDirector/scripts/Conductor.cs(1 hunks)scenes/BattleDirector/scripts/NotePlacementBar.cs(1 hunks)scenes/BattleDirector/scripts/TextParticle.cs(0 hunks)scenes/BattleDirector/test_battle_scene.tscn(2 hunks)scenes/ChartViewport/ChartManager.cs(4 hunks)scenes/Maps/cartographer.tscn(1 hunks)scenes/Maps/scripts/Cartographer.cs(1 hunks)scenes/NoteManager/assets/outline_white.png.import(1 hunks)scenes/NoteManager/note.tscn(1 hunks)scenes/NoteManager/note_manager.tscn(2 hunks)scenes/NoteManager/scripts/InputHandler.cs(1 hunks)scenes/NoteManager/scripts/NoteArrow.cs(2 hunks)scenes/Puppets/HealthBar.tscn(1 hunks)scenes/Puppets/scripts/HealthBar.cs(1 hunks)scenes/Puppets/scripts/PlayerPuppet.cs(1 hunks)scenes/Puppets/scripts/PlayerStats.cs(1 hunks)scenes/Puppets/scripts/PuppetTemplate.cs(1 hunks)scenes/SceneTransitions/TitleScreen.tscn(1 hunks)scenes/SceneTransitions/scripts/SceneChange.cs(1 hunks)scenes/SceneTransitions/testTransition.tscn(1 hunks)scenes/UI/Pause.tscn(1 hunks)scenes/UI/RewardSelectionUI.tscn(1 hunks)scenes/UI/display_button.tscn(1 hunks)scenes/UI/inventory.tscn(1 hunks)scenes/UI/scripts/DisplayButton.cs(1 hunks)scenes/UI/scripts/Inventory.cs(1 hunks)scenes/UI/scripts/PauseMenu.cs(1 hunks)scenes/UI/scripts/RewardSelect.cs(1 hunks)scenes/main.tscn(0 hunks)scripts/Main.cs(0 hunks)
💤 Files with no reviewable changes (10)
- scripts/Main.cs
- README.md
- scenes/BattleDirector/scripts/TextParticle.cs
- scenes/main.tscn
- scenes/BattleDirector/HealthBar.tscn
- scenes/BattleDirector/TextParticle.tscn
- scenes/BattleDirector/HealthBar.cs
- scenes/BattleDirector/BattleDirector.cs
- Classes/Note.cs
- scenes/BattleDirector/NotePlacementBar.cs
✅ Files skipped from review due to trivial changes (8)
- scenes/Maps/cartographer.tscn
- scenes/NoteManager/note.tscn
- scenes/NoteManager/assets/outline_white.png.import
- Classes/Relics/assets/relic_GoodVibes.png.import
- scenes/UI/display_button.tscn
- SaveData/SaveData.json
- scenes/BattleDirector/assets/temp_note_queue.png.import
- Classes/Relics/assets/relic_Breakfast.png.import
🧰 Additional context used
🪛 GitHub Check: build
Globals/StageProducer.cs
[warning] 31-31:
The field 'StageProducer.MapGrid._curRoom' is assigned but its value is never used
[warning] 31-31:
The field 'StageProducer.MapGrid._curRoom' is assigned but its value is never used
🔇 Additional comments (19)
scenes/NoteManager/note_manager.tscn (2)
13-34: LGTM! Efficient implementation using rotated sprites.The texture updates are consistently applied across all arrow sprites. The implementation efficiently reuses the same texture with different rotations to create directional arrows:
- Up: -90 degrees
- Left: 180 degrees
- Down: 90 degrees
- Right: 0 degrees (default)
4-4:Details
❓ Verification inconclusive
Verify the new texture asset.
The texture resource has been changed from
right-arrow.pngtooutline_white.png. Please ensure that this new texture maintains visual clarity and matches the game's art style.
🏁 Script executed:
#!/bin/bash # Verify the texture asset exists and check its import configuration fd "outline_white.png" --type fLength of output: 126
Verification Success: The texture asset exists.
The updated texture resource in
scenes/NoteManager/note_manager.tscnnow referencesoutline_white.pnginstead ofright-arrow.png. The filescenes/NoteManager/assets/outline_white.pngalong with its corresponding import file exists. Please verify that the new texture maintains the intended visual clarity and consistency with the game's art style, and confirm that the arrow node textures update accordingly.Globals/StageProducer.cs (1)
84-102: Verify boundary conditions in recursive path generation.While the recursion limit check (
if (y < height - 2)) helps safeguard against out-of-bounds indexing, verifying all call paths for_map[nextX, y + 1]is prudent to ensure it's never accessed wheny + 1 >= height. Confirm that deeper code paths do not invokeGeneratePath_rwithy == height - 1.scenes/Puppets/scripts/HealthBar.cs (1)
4-9: LGTM!Good initialization of the progress bar value.
Classes/Notes/assets/double_note.png.import (1)
1-34: LGTM! Import configuration is appropriate for a UI texture.The texture import settings are well-configured for a 2D game UI element, with appropriate compression settings and disabled VRAM texture.
Classes/Notes/assets/single_note.png.import (1)
1-34: LGTM! Import configuration is appropriate for a UI texture.The texture import settings are well-configured for a 2D game UI element, with appropriate compression settings and disabled VRAM texture.
scenes/NoteManager/scripts/NoteArrow.cs (2)
10-14: LGTM! Properties are well-structured.The new properties
Type,Beat, andNoteRefare properly encapsulated and align with the new note handling system.
16-25: LGTM! Init method properly initializes new properties.The updated
Initmethod correctly sets theTypeandBeatproperties from the provided parameters.SaveData/SaveSystem.cs (1)
42-48: LGTM! SaveData class is well-structured.The
SaveDataclass provides a clean structure for storing different types of game data with appropriate property types.Classes/Notes/Note.cs (1)
14-14: Address TODO comment about timing handling.The comment indicates uncertainty about where/how to handle timing. This should be clarified and documented.
Could you provide more context about how timing should be handled in the note effect system?
scenes/Maps/scripts/Cartographer.cs (1)
46-46:Details
❓ Verification inconclusive
Review button focus behavior.
Calling
GrabFocus()on every child room's button might lead to focus issues. Consider implementing a more controlled focus management system.
🏁 Script executed:
#!/bin/bash # Search for other focus management implementations rg -A 5 "GrabFocus|FocusMode" --type csLength of output: 2098
Focus Management Behavior: Revisit and Unify Focus Handling
It appears that the use of
GrabFocus()is pervasive across several UI components (for example, inscenes/Maps/scripts/Cartographer.cs,scenes/SceneTransitions/scripts/SceneChange.cs,scenes/UI/scripts/PauseMenu.cs, among others). InCartographer.cs, the conditional use ofGrabFocus()(after selectively settingFocusMode = Control.FocusModeEnum.None) suggests mixed focus management strategies that could lead to unpredictable behavior when multiple buttons vie for focus.
- Cartographer.cs (line 46): Conditionally calls
newSprite.GrabFocus()despite setting a non-focus mode in the alternate branch.- Other Modules: Similar focus calls in other scenes (e.g.,
SceneChange.cs,PauseMenu.cs, etc.) indicate that this issue might be more widespread across the project.I recommend reviewing the overall focus management strategy to ensure a consistent and controlled user experience. Consider centralizing focus logic or coordinating focus behavior across these components.
scenes/NoteManager/scripts/InputHandler.cs (1)
3-3: LGTM! Good refactoring of the ArrowData struct.The addition of the FunkEngine namespace and moving the ArrowData struct to a centralized location improves code organization.
scenes/BattleDirector/test_battle_scene.tscn (3)
24-26: LGTM! Good addition of the Conductor node.The Conductor node is well-integrated with the ChartViewport, which is a common pattern in rhythm games for handling timing and synchronization.
10-17:Details
❓ Verification inconclusive
Verify the impact of removing health-related nodes.
The removal of
PlayerHPandEnemyHPnodes suggests a major UI restructuring. Ensure that health information is still accessible to players through alternative means.
🏁 Script executed:
#!/bin/bash # Search for new health UI implementation rg -l "health|hp|healthbar" --type csLength of output: 134
Health UI Restructuring – Verify New Health Display Integration
The removal of
PlayerHPandEnemyHPnodes inscenes/BattleDirector/test_battle_scene.tscnindicates a significant UI restructuring. The search results show a new health UI implementation in:
scenes/Puppets/scripts/HealthBar.cs- Also, related health logic appears in
Globals/Scribe.csandscenes/Puppets/scripts/PuppetTemplate.csPlease verify that these new components correctly provide the necessary health feedback during gameplay and that the overall UI integration is seamless.
49-54:Details
❓ Verification inconclusive
Consider z-index implications.
Setting z_index=1 for NotePlacementBar ensures it renders above the background but verify it doesn't overlap with other UI elements.
🏁 Script executed:
#!/bin/bash # Search for other nodes with z-index settings rg "z_index" --type tscn -A 2Length of output: 58
Attention: Verify z-index usage across .tscn files
It appears the initial search command for
"z_index"failed because ripgrep didn’t recognize thetscnfile type. To better assess potential conflicts, please run the revised command below that adds support for.tscnfiles:This command should help verify that setting
z_index = 1for theNotePlacementBardoes not interfere with other UI elements. Once you run this, please review the output for any potential overlaps or conflicting z-index values.
🏁 Script executed:
#!/bin/bash # Adding a custom file type for .tscn and searching for occurrences of z_index rg --type-add 'tscn:*.tscn' --type tscn "z_index" -A 2Length of output: 800
Layering Order Verification: z-index Usage in Test Scene
The search results confirm that the
NotePlacementBarnode withz_index = 1is configured to render above other UI elements in the same scene that use lowerz_indexvalues (e.g.,-1) as well as nodes in other scenes (likeChartViewport.tscnwithz_index = -2). This indicates that the intended layering is maintained, with the note placement bar appearing above background elements. However, please verify at runtime that the higher z-index does not lead to unexpected overlaps with any additional foreground elements added dynamically.
- File:
scenes/BattleDirector/test_battle_scene.tscn(lines 49-54) –NotePlacementBarusesz_index = 1- Context: Other nodes in the file have
z_index = -1and inscenes/ChartViewport/ChartViewport.tscnthe node usesz_index = -2scenes/UI/inventory.tscn (1)
5-17: Well-structured inventory system with proper node references.Good use of node paths and process mode for pause functionality.
scenes/UI/Pause.tscn (1)
45-49: Good implementation of keyboard/gamepad navigation.The focus neighbor setup ensures proper navigation between buttons using keyboard or gamepad.
scenes/SceneTransitions/TitleScreen.tscn (1)
49-52: Replace placeholder title text.The current title text is a placeholder and should be replaced with the actual game title.
-text = "Insert Title Screen Here" +text = "Project Funk Engine"Funk Engine.csproj (1)
9-11: LGTM!The addition of SaveData.json as content is correctly implemented.
| public int ChangeHP(int amount) | ||
| { | ||
| Value += amount; | ||
| return (int)Value; | ||
| } |
There was a problem hiding this comment.
Add bounds checking for health changes.
The method should clamp health values between 0 and MaxValue.
public int ChangeHP(int amount)
{
- Value += amount;
+ Value = Math.Clamp(Value + amount, 0, MaxValue);
return (int)Value;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public int ChangeHP(int amount) | |
| { | |
| Value += amount; | |
| return (int)Value; | |
| } | |
| public int ChangeHP(int amount) | |
| { | |
| Value = Math.Clamp(Value + amount, 0, MaxValue); | |
| return (int)Value; | |
| } |
| public void ChangeMax(int change) | ||
| { | ||
| MaxValue += change; | ||
| Value += change; | ||
| } |
There was a problem hiding this comment.
Add bounds checking for health changes.
The method should ensure that health values remain within valid bounds after changes.
public void ChangeMax(int change)
{
+ if (MaxValue + change < 0)
+ throw new ArgumentException("Change would result in negative max health", nameof(change));
MaxValue += change;
- Value += change;
+ // Adjust current health, but don't exceed new max
+ Value = Math.Min(Value + change, MaxValue);
+ // Ensure health doesn't go negative
+ Value = Math.Max(0, Value);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public void ChangeMax(int change) | |
| { | |
| MaxValue += change; | |
| Value += change; | |
| } | |
| public void ChangeMax(int change) | |
| { | |
| if (MaxValue + change < 0) | |
| throw new ArgumentException("Change would result in negative max health", nameof(change)); | |
| MaxValue += change; | |
| // Adjust current health, but don't exceed new max | |
| Value = Math.Min(Value + change, MaxValue); | |
| // Ensure health doesn't go negative | |
| Value = Math.Max(0, Value); | |
| } |
| public RelicTemplate Clone() | ||
| { | ||
| RelicTemplate newRelic = new RelicTemplate(Name, Tooltip, Texture, Effects); | ||
| return newRelic; | ||
| } |
There was a problem hiding this comment.
Implement deep copy for Effects array.
The current implementation performs a shallow copy of the Effects array, which could lead to shared state between clones.
public RelicTemplate Clone()
{
- RelicTemplate newRelic = new RelicTemplate(Name, Tooltip, Texture, Effects);
+ RelicTemplate newRelic = new RelicTemplate(
+ Name,
+ Tooltip,
+ Texture,
+ Effects?.Select(effect => new RelicEffect(
+ effect.GetTrigger(),
+ effect.BaseValue,
+ effect.OnRelicEffect
+ )).ToArray()
+ );
return newRelic;
}📝 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 RelicTemplate Clone() | |
| { | |
| RelicTemplate newRelic = new RelicTemplate(Name, Tooltip, Texture, Effects); | |
| return newRelic; | |
| } | |
| public RelicTemplate Clone() | |
| { | |
| RelicTemplate newRelic = new RelicTemplate( | |
| Name, | |
| Tooltip, | |
| Texture, | |
| Effects?.Select(effect => new RelicEffect( | |
| effect.GetTrigger(), | |
| effect.BaseValue, | |
| effect.OnRelicEffect | |
| )).ToArray() | |
| ); | |
| return newRelic; | |
| } |
| public partial class RelicTemplate : Resource | ||
| { | ||
| public RelicEffect[] Effects; | ||
| public string Name; | ||
|
|
||
| public Texture2D Texture; | ||
| public string Tooltip; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use properties instead of public fields and add documentation.
Consider encapsulating fields as properties and adding XML documentation for better maintainability.
+/// <summary>
+/// Represents a template for game relics, defining their properties and effects.
+/// </summary>
public partial class RelicTemplate : Resource
{
- public RelicEffect[] Effects;
- public string Name;
- public Texture2D Texture;
- public string Tooltip;
+ /// <summary>
+ /// Gets or sets the effects associated with this relic.
+ /// </summary>
+ public RelicEffect[] Effects { get; set; }
+
+ /// <summary>
+ /// Gets or sets the name of the relic.
+ /// </summary>
+ public string Name { get; set; }
+
+ /// <summary>
+ /// Gets or sets the texture used to display the relic.
+ /// </summary>
+ public Texture2D Texture { get; set; }
+
+ /// <summary>
+ /// Gets or sets the tooltip description of the relic.
+ /// </summary>
+ public string Tooltip { get; set; }📝 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 partial class RelicTemplate : Resource | |
| { | |
| public RelicEffect[] Effects; | |
| public string Name; | |
| public Texture2D Texture; | |
| public string Tooltip; | |
| /// <summary> | |
| /// Represents a template for game relics, defining their properties and effects. | |
| /// </summary> | |
| public partial class RelicTemplate : Resource | |
| { | |
| /// <summary> | |
| /// Gets or sets the effects associated with this relic. | |
| /// </summary> | |
| public RelicEffect[] Effects { get; set; } | |
| /// <summary> | |
| /// Gets or sets the name of the relic. | |
| /// </summary> | |
| public string Name { get; set; } | |
| /// <summary> | |
| /// Gets or sets the texture used to display the relic. | |
| /// </summary> | |
| public Texture2D Texture { get; set; } | |
| /// <summary> | |
| /// Gets or sets the tooltip description of the relic. | |
| /// </summary> | |
| public string Tooltip { get; set; } | |
| } |
| public partial class RelicEffect : IBattleEvent | ||
| { | ||
| private BattleEffectTrigger Trigger { get; set; } | ||
| public int BaseValue; | ||
| private Action<BattleDirector, int> OnRelicEffect; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use consistent property encapsulation and add documentation.
Consider using properties consistently and adding XML documentation for better maintainability.
+/// <summary>
+/// Represents an effect that can be triggered during battle.
+/// </summary>
public partial class RelicEffect : IBattleEvent
{
private BattleEffectTrigger Trigger { get; set; }
- public int BaseValue;
+ /// <summary>
+ /// Gets or sets the base value for this effect.
+ /// </summary>
+ public int BaseValue { get; set; }
private Action<BattleDirector, int> OnRelicEffect;
}📝 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 partial class RelicEffect : IBattleEvent | |
| { | |
| private BattleEffectTrigger Trigger { get; set; } | |
| public int BaseValue; | |
| private Action<BattleDirector, int> OnRelicEffect; | |
| /// <summary> | |
| /// Represents an effect that can be triggered during battle. | |
| /// </summary> | |
| public partial class RelicEffect : IBattleEvent | |
| { | |
| private BattleEffectTrigger Trigger { get; set; } | |
| /// <summary> | |
| /// Gets or sets the base value for this effect. | |
| /// </summary> | |
| public int BaseValue { get; set; } | |
| private Action<BattleDirector, int> OnRelicEffect; | |
| } |
| var newButton = GD.Load<PackedScene>("res://scenes/UI/display_button.tscn") | ||
| .Instantiate<DisplayButton>(); | ||
| newButton.Display(relic.Texture, relic.Tooltip, relic.Name); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize scene loading by caching the PackedScene.
Loading the same scene multiple times is inefficient. Consider caching the PackedScene as a class field.
+private PackedScene _buttonScene;
+
+public override void _Ready()
+{
+ _buttonScene = GD.Load<PackedScene>("res://scenes/UI/display_button.tscn");
+ Tabs.GetTabBar().GrabFocus();
+}
public void Display(PlayerStats playerStats)
{
foreach (RelicTemplate relic in playerStats.CurRelics)
{
- var newButton = GD.Load<PackedScene>("res://scenes/UI/display_button.tscn")
- .Instantiate<DisplayButton>();
+ var newButton = _buttonScene.Instantiate<DisplayButton>();
// ... rest of the code
}Also applies to: 33-35
| [node name="SecretLabel" type="Label" parent="."] | ||
| visible = false | ||
| layout_mode = 0 | ||
| offset_right = 49.0 | ||
| offset_bottom = 23.0 | ||
| text = "(Control nodes are fucking weird weird.)" | ||
|
|
There was a problem hiding this comment.
Remove or rephrase unprofessional debug comment.
The debug label contains inappropriate language. Consider removing it or rephrasing professionally.
-text = "(Control nodes are fucking weird weird.)"
+text = "(Note: Control nodes have unique behavior)"📝 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.
| [node name="SecretLabel" type="Label" parent="."] | |
| visible = false | |
| layout_mode = 0 | |
| offset_right = 49.0 | |
| offset_bottom = 23.0 | |
| text = "(Control nodes are fucking weird weird.)" | |
| [node name="SecretLabel" type="Label" parent="."] | |
| visible = false | |
| layout_mode = 0 | |
| offset_right = 49.0 | |
| offset_bottom = 23.0 | |
| text = "(Note: Control nodes have unique behavior)" |
| (director, note, timing) => | ||
| { | ||
| director.Player.TakeDamage(4 - (int)timing); | ||
| } | ||
| ), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract magic numbers into constants.
The damage calculation uses hardcoded values that should be constants for better maintainability.
+private const int BASE_DAMAGE = 4;
+
(director, note, timing) =>
{
- director.Player.TakeDamage(4 - (int)timing);
+ director.Player.TakeDamage(BASE_DAMAGE - (int)timing);
}📝 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.
| (director, note, timing) => | |
| { | |
| director.Player.TakeDamage(4 - (int)timing); | |
| } | |
| ), | |
| private const int BASE_DAMAGE = 4; | |
| (director, note, timing) => | |
| { | |
| director.Player.TakeDamage(BASE_DAMAGE - (int)timing); | |
| } | |
| ), |
| //Create during battle copies of player notes | ||
| _noteDeck = (Note[])playerStats.CurNotes.Clone(); //Do NOT ever change these notes directly :) | ||
| if (_noteDeck.Length <= 0) | ||
| { | ||
| GD.PushError("Imminent crash: Note Deck is empty!"); | ||
| GetTree().Paused = true; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in Setup method.
The error handling for an empty note deck could be improved. Currently, it logs an error and pauses the game, which might not be the best user experience.
Consider this implementation:
public void Setup(PlayerStats playerStats)
{
//Create during battle copies of player notes
_noteDeck = (Note[])playerStats.CurNotes.Clone(); //Do NOT ever change these notes directly :)
if (_noteDeck.Length <= 0)
{
- GD.PushError("Imminent crash: Note Deck is empty!");
- GetTree().Paused = true;
+ throw new InvalidOperationException("Cannot initialize NotePlacementBar with an empty note deck");
}
ShuffleNoteQueue();
ProgressQueue();
}📝 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.
| //Create during battle copies of player notes | |
| _noteDeck = (Note[])playerStats.CurNotes.Clone(); //Do NOT ever change these notes directly :) | |
| if (_noteDeck.Length <= 0) | |
| { | |
| GD.PushError("Imminent crash: Note Deck is empty!"); | |
| GetTree().Paused = true; | |
| } | |
| public void Setup(PlayerStats playerStats) | |
| { | |
| //Create during battle copies of player notes | |
| _noteDeck = (Note[])playerStats.CurNotes.Clone(); //Do NOT ever change these notes directly :) | |
| if (_noteDeck.Length <= 0) | |
| { | |
| throw new InvalidOperationException("Cannot initialize NotePlacementBar with an empty note deck"); | |
| } | |
| ShuffleNoteQueue(); | |
| ProgressQueue(); | |
| } |
| private void AddExampleNotes() | ||
| { | ||
| GD.Print(CM.BeatsPerLoop); | ||
| for (int i = 1; i < 15; i++) | ||
| { | ||
| AddNoteToLane(ArrowType.Up, i * 4, Scribe.NoteDictionary[0]); | ||
| } | ||
|
|
||
| for (int i = 1; i < 15; i++) | ||
| { | ||
| AddNoteToLane(ArrowType.Left, 4 * i + 1, Scribe.NoteDictionary[0]); | ||
| } | ||
|
|
||
| for (int i = 0; i < 10; i++) | ||
| { | ||
| AddNoteToLane(ArrowType.Right, 3 * i + 32, Scribe.NoteDictionary[0]); | ||
| } | ||
|
|
||
| for (int i = 0; i < 3; i++) | ||
| { | ||
| AddNoteToLane(ArrowType.Down, 8 * i + 16, Scribe.NoteDictionary[0]); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract magic numbers in AddExampleNotes.
The method contains several magic numbers that should be constants for better maintainability.
+private const int NOTES_PER_LANE = 15;
+private const int UP_LANE_INTERVAL = 4;
+private const int LEFT_LANE_OFFSET = 1;
+private const int RIGHT_LANE_START = 32;
+private const int RIGHT_LANE_INTERVAL = 3;
+private const int DOWN_LANE_START = 16;
+private const int DOWN_LANE_INTERVAL = 8;
private void AddExampleNotes()
{
GD.Print(CM.BeatsPerLoop);
- for (int i = 1; i < 15; i++)
+ for (int i = 1; i < NOTES_PER_LANE; i++)
{
- AddNoteToLane(ArrowType.Up, i * 4, Scribe.NoteDictionary[0]);
+ AddNoteToLane(ArrowType.Up, i * UP_LANE_INTERVAL, Scribe.NoteDictionary[0]);
}
// ... similar changes for other loops
}📝 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.
| private void AddExampleNotes() | |
| { | |
| GD.Print(CM.BeatsPerLoop); | |
| for (int i = 1; i < 15; i++) | |
| { | |
| AddNoteToLane(ArrowType.Up, i * 4, Scribe.NoteDictionary[0]); | |
| } | |
| for (int i = 1; i < 15; i++) | |
| { | |
| AddNoteToLane(ArrowType.Left, 4 * i + 1, Scribe.NoteDictionary[0]); | |
| } | |
| for (int i = 0; i < 10; i++) | |
| { | |
| AddNoteToLane(ArrowType.Right, 3 * i + 32, Scribe.NoteDictionary[0]); | |
| } | |
| for (int i = 0; i < 3; i++) | |
| { | |
| AddNoteToLane(ArrowType.Down, 8 * i + 16, Scribe.NoteDictionary[0]); | |
| } | |
| } | |
| private const int NOTES_PER_LANE = 15; | |
| private const int UP_LANE_INTERVAL = 4; | |
| private const int LEFT_LANE_OFFSET = 1; | |
| private const int RIGHT_LANE_START = 32; | |
| private const int RIGHT_LANE_INTERVAL = 3; | |
| private const int DOWN_LANE_START = 16; | |
| private const int DOWN_LANE_INTERVAL = 8; | |
| private void AddExampleNotes() | |
| { | |
| GD.Print(CM.BeatsPerLoop); | |
| for (int i = 1; i < NOTES_PER_LANE; i++) | |
| { | |
| AddNoteToLane(ArrowType.Up, i * UP_LANE_INTERVAL, Scribe.NoteDictionary[0]); | |
| } | |
| for (int i = 1; i < NOTES_PER_LANE; i++) | |
| { | |
| AddNoteToLane(ArrowType.Left, i * UP_LANE_INTERVAL + LEFT_LANE_OFFSET, Scribe.NoteDictionary[0]); | |
| } | |
| for (int i = 0; i < 10; i++) | |
| { | |
| AddNoteToLane(ArrowType.Right, i * RIGHT_LANE_INTERVAL + RIGHT_LANE_START, Scribe.NoteDictionary[0]); | |
| } | |
| for (int i = 0; i < 3; i++) | |
| { | |
| AddNoteToLane(ArrowType.Down, i * DOWN_LANE_INTERVAL + DOWN_LANE_START, Scribe.NoteDictionary[0]); | |
| } | |
| } |
|
Lets go!! |
If there are broken things at this point so be it, polish is still to come anyways.
Game loop is fundamentally done. Game flow is sketched out. Need to polish, test refinement, and add the flesh and blood at this point.
Summary by CodeRabbit
New Features
Refactor / Chores