Midi readin #53
Midi readin #53
Conversation
WalkthroughThe changes add new configuration files, classes, properties, and scenes to enhance the engine’s functionality. The updates include modifications to ignore files, import audio and texture assets, and introduce MIDI processing via a new MidiMaestro class. Several UI and gameplay scenes, such as battle, chart, chest, maps, note management, remapping, and scene transitions, have been reworked. Global modules were updated with new structs and utility methods, while control schemes and reward selection received additional event handling. Additional package references and project configuration changes support these new features. Changes
Sequence Diagram(s)sequenceDiagram
participant SP as StageProducer
participant BD as BattleDirector
participant E as Enemy
participant RS as RewardSelect
participant P as Player
SP->>BD: Provide BattleConfig and current song data
BD->>E: Initialize enemy from scene
BD->>BD: Monitor battle status (CheckBattleStatus)
E-->>BD: Signal enemy defeated
BD->>BD: Call EndBattle()
BD->>RS: Trigger RewardSelect.CreateSelection()
RS-->>BD: Emit Selected event (reward chosen)
BD->>SP: Initiate transition to next stage
sequenceDiagram
participant C as Conductor
participant MM as MidiMaestro
participant S as Scribe
participant CM as ChartManager
C->>MM: Instantiate with MIDI file path
MM-->>C: Return notes for each arrow type
loop For each note
C->>CM: AddNoteToLane(arrow, beat, note)
end
C->>S: Retrieve note properties from Scribe.NoteDictionary
Possibly related PRs
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Pro Plan! Simply use the command ✨ 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 (
|
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
added a seperate scene were the player can choose from the three schemes, defaults to arrows. slight tweak to inputhandler to refresh the controls. button to control scene was addded. will try to use sprites/icons to show which keys will correspend to each arrow in next push
Added icons to represent which key correspond to each direction, changes as the user changes the scheme. Fixed a typo that caused it to always default to WASD. Sprite paths are added to the ControlSchemes as a dictionary set. Icons are from this free pack: https://thoseawesomeguys.com/prompts/
removed dupe code from rebase
Added input button attributions
currently song1 is broken, I didn't have time to dive into it, but I believe it is because we created the first song using methods other than the MIDI file. Would have to either replace the song or create a midi for it.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🔭 Outside diff range comments (1)
scenes/NoteManager/scripts/NoteArrow.cs (1)
22-33: 🛠️ Refactor suggestionAdd null checks for IconSprite and note.Texture.
The current implementation doesn't check if
IconSpriteornote.Textureare null before using them, which could lead to runtime errors.public void Init(ArrowData parentArrowData, int beat, Note note) { ZIndex = 1; Type = parentArrowData.Type; Beat = beat; Position += Vector2.Down * (parentArrowData.Node.GlobalPosition.Y); RotationDegrees = parentArrowData.Node.RotationDegrees; - IconSprite.Texture = note.Texture; - IconSprite.Rotation = -Rotation; + if (IconSprite != null && note?.Texture != null) + { + IconSprite.Texture = note.Texture; + IconSprite.Rotation = -Rotation; + } }
🧹 Nitpick comments (42)
scenes/UI/scripts/MenuModule.cs (1)
4-23: Consider adding menu cleanup methodThe class handles opening menus but doesn't appear to have any mechanism for removing them when they're closed. Consider implementing a method to handle cleanup of instantiated menus.
public partial class MenuModule : CanvasLayer { + private Node currentMenu; + public override void _Input(InputEvent @event) { if (@event.IsActionPressed("Pause")) { + if (currentMenu != null) return; var pauseMenu = GD.Load<PackedScene>("res://scenes/UI/Pause.tscn"); - AddChild(pauseMenu.Instantiate()); + currentMenu = pauseMenu.Instantiate(); + AddChild(currentMenu); GetTree().Paused = true; } if (@event.IsActionPressed("Inventory")) { + if (currentMenu != null) return; var invenMenu = GD.Load<PackedScene>("res://scenes/UI/inventory.tscn") .Instantiate<Inventory>(); - AddChild(invenMenu); + currentMenu = invenMenu; + AddChild(currentMenu); invenMenu.Display(StageProducer.PlayerStats ?? new PlayerStats()); //For now work around for testing GetTree().Paused = true; } } + + public void CloseCurrentMenu() + { + if (currentMenu != null) + { + currentMenu.QueueFree(); + currentMenu = null; + GetTree().Paused = false; + } + } }scenes/Puppets/scripts/PlayerStats.cs (1)
15-16: Consider adding more context to the note initialization.The addition of a fourth note (
Scribe.NoteDictionary[3].Clone()) to theCurNotesarray makes sense, but the usage of numeric indices (1, 1, 2, 3) makes it difficult to understand what each note represents.Consider using named constants or adding comments to explain what each note represents:
public Note[] CurNotes = new Note[] { - Scribe.NoteDictionary[1].Clone(), - Scribe.NoteDictionary[1].Clone(), - Scribe.NoteDictionary[2].Clone(), - Scribe.NoteDictionary[3].Clone(), + Scribe.NoteDictionary[1].Clone(), // Basic attack note + Scribe.NoteDictionary[1].Clone(), // Duplicate for strategy variety + Scribe.NoteDictionary[2].Clone(), // Defense note + Scribe.NoteDictionary[3].Clone(), // New note type };Alternatively, consider adding constants in the
Scribeclass to reference note types by name.Classes/MidiMaestro/SongTemplate.cs (1)
3-14: Consider using properties and validation for SongTemplate class.The new class looks good for basic song metadata, but it could benefit from better encapsulation and validation.
Consider using properties instead of public fields, which would allow for future validation and ensure immutability when needed:
public partial class SongTemplate { - public string Name; - public string AudioLocation; - public string MIDILocation; + public string Name { get; private set; } + public string AudioLocation { get; private set; } + public string MIDILocation { get; private set; } public SongTemplate(string name = "", string audioLocation = "", string midiLocation = "") { - Name = name; - AudioLocation = audioLocation; - MIDILocation = midiLocation; + Name = string.IsNullOrEmpty(name) ? "Unnamed Song" : name; + AudioLocation = audioLocation; + MIDILocation = midiLocation; + + // Optional: Add validation for file paths + // if (!string.IsNullOrEmpty(audioLocation) && !File.Exists(audioLocation)) + // GD.PrintWarning($"Audio file not found: {audioLocation}"); } }scenes/BattleDirector/scripts/Conductor.cs (3)
9-9: Consider making MidiMaestro an [Export] property.The MidiMaestro instance is currently declared as a public field without the [Export] attribute, which means it can't be set through the editor. Since it seems to be an important component, consider making it configurable through the editor.
- public MidiMaestro MM; + [Export] + public MidiMaestro MM;
87-118: Add variation to note types.Currently, all arrow types use the same note (Scribe.NoteDictionary[0]), which seems like a missed opportunity for variety in gameplay.
Consider mapping different MIDI notes or channels to different note types in your game, creating a more varied and interesting gameplay experience.
120-138: Remove commented-out code.Commented-out code creates visual noise and can lead to confusion about what functionality is currently active. If this code is no longer needed, it should be removed.
-/*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]); -}*/If this code is kept for reference or future use, add a comment explaining why it's being kept.
scenes/BattleDirector/scripts/NotePlacementBar.cs (2)
136-137: Consider validating note placement cost.The current implementation subtracts the cost after checking if a note can be placed, but doesn't validate that the resulting value is non-negative. If CostModifier is large, this could potentially lead to negative values.
-_currentBarValue -= (int)(_currentNoteInstance.CostModifier * MaxValue); +// Ensure the bar value doesn't go below zero +_currentBarValue = Math.Max(0, _currentBarValue - (int)(_currentNoteInstance.CostModifier * MaxValue));
159-159: Consider using proper UI alignment instead of whitespace padding.Using leading spaces for alignment is fragile and can break with different fonts or text sizes. Godot provides better ways to align text in UI elements.
-currentComboMultText.Text = $" x{comboMult.ToString()}"; +currentComboMultText.Text = $"x{comboMult}";Then adjust the TextEdit's alignment property in the editor to achieve the desired positioning.
scenes/Puppets/Enemies/EnemyPuppet.tscn (1)
6-6: Node name inconsistency.The node is named "EnemPuppet" instead of "EnemyPuppet", which is inconsistent with the file name and could lead to confusion.
Rename the node to "EnemyPuppet" for consistency.
scenes/Puppets/PlayerPuppet.tscn (1)
7-11: Consider making StartPos configurable at runtime.The hard-coded StartPos value might not be suitable for all scene layouts or screen resolutions. Consider calculating this position dynamically based on the viewport or other scene elements.
Move the position calculation to the script's _Ready method to make it more adaptable to different viewport sizes.
scenes/Remapping/ControlSchemes.cs (2)
6-41: Consider making the Schemes dictionary readonly to prevent unintended modifications.The
Schemesdictionary is correctly structured with mapping control scheme names to their key configurations. However, since this is a static dictionary that likely shouldn't be modified at runtime, consider making it readonly for additional safety.- public static Dictionary<string, Dictionary<string, string>> Schemes = new Dictionary< + public static readonly Dictionary<string, Dictionary<string, string>> Schemes = new Dictionary<
43-78: Consider making the SpriteMappings dictionary readonly to prevent unintended modifications.Similar to the
Schemesdictionary, theSpriteMappingsdictionary contains static mapping data that shouldn't be modified at runtime. Consider making it readonly as well.- public static Dictionary<string, Dictionary<string, string>> SpriteMappings = new Dictionary< + public static readonly Dictionary<string, Dictionary<string, string>> SpriteMappings = new Dictionary<scenes/NoteManager/scripts/NoteArrow.cs (1)
14-14: Consider updating NoteRef with the provided note parameter.The class has a
NoteRefproperty but it's not being set in theInitmethod even though aNoteparameter is now passed. Consider updatingNoteRefwith the providednoteto maintain consistency.public void Init(ArrowData parentArrowData, int beat, Note note) { ZIndex = 1; + NoteRef = note; Type = parentArrowData.Type; Beat = beat;scenes/UI/scripts/RewardSelect.cs (3)
19-20: Consider a more descriptive event signature or name.You’ve introduced a delegate and event that carry no contextual information, which may limit flexibility if you need to differentiate scenarios (e.g., skipping vs. selecting) in the future. Consider passing parameters or renaming the event to something more general like
OnSelectionCompleted.
80-80: Event name might conflict with skip usage.You invoke
Selected?.Invoke()upon relic selection, which makes sense. However, you also invoke the same event on line 88 when skipping. Consider renaming it to something likeOnChoiceMadeor passing a parameter indicating whether a relic was chosen or skipped.
88-88: Reassess identical event invocation for skip.Firing the same event on skip may be confusing if other subscribers assume it means a relic was definitely selected. You could either define a separate event or pass an argument to indicate the skip.
scenes/ChestScene/ChestScene.cs (3)
5-11: Consider data encapsulation for public fields.Publishing
Playeras a plain public field is typical in Godot but can make debugging harder if it’s mutated unexpectedly. Properties or[Export]might provide more controlled access.
12-19: Add safety checks for resource loading and null references.It’s good to load and instantiate the
PlayerPuppethere; however, consider handling load failures gracefully (e.g., logging) if the PackedScene is missing.
35-39: Clarify the meaning of “EndBattle.”Method name may be misleading if the user is just collecting loot from a chest. Renaming it to better match its functionality could improve code readability.
scenes/ChartViewport/Loopable.cs (3)
19-19: Check for division by zero.Dividing by
TimeKeeper.LoopLengthcan fail ifLoopLengthis zero or undefined. Consider adding guards or logging for debugging.
23-24: Simplify or document the position calculation.The expression is both mathematically dense and tied to
TimeKeeper.PosMod. Adding brief in-code comments or splitting into steps would reduce confusion and maintain clarity.
25-26: Add a fallback for NaN positions.Simply skipping assignment if
newPos.Xis NaN may cause the sprite to freeze in an unexpected location. Logging a warning or setting a default position can help troubleshoot potential calculation issues.Globals/Scribe.cs (1)
144-156: SongDictionary implementation is clean.The new SongDictionary array provides a structured way to map song names to their audio and MIDI files. Consider adding more songs to expand the game's audio variety.
Consider moving these hardcoded song paths to a configuration file that can be more easily updated without recompiling the code.
scenes/SceneTransitions/TitleScreen.tscn (1)
108-112: New control remapping button.A new button for control remapping has been added to the title screen, linking to the new Remap scene (ScenePath 6). This provides users with direct access to control customization.
Consider adding margin settings to match the other buttons' layout constraints for visual consistency.
[node name="Button" type="Button" parent="VBoxContainer"] layout_mode = 2 +theme_override_constants/margin_left = 20 +theme_override_constants/margin_top = 20 +theme_override_constants/margin_right = 20 +theme_override_constants/margin_bottom = 20 text = "Change Controls" script = ExtResource("2_7f3m6") ScenePath = 6Classes/Notes/Note.cs (1)
5-8: Update class documentation.The class documentation should be updated to include information about the new CostModifier property.
/** * @class Note * * @brief Data structure class for holding data and methods for a battle time note. WIP + * + * CostModifier: Adjusts the resource cost of using this note. Default is 1.0f, + * values below 1.0f make the note cheaper, values above make it more expensive. */scenes/Puppets/scripts/PuppetTemplate.cs (1)
19-19: Replace comment with default value initializationThe comment indicates a default position, but it would be better to initialize the property directly.
[Export] -public Vector2 StartPos; //158, 126 +public Vector2 StartPos = new Vector2(158, 126);scenes/ChestScene/ChestScene.tscn (2)
15-16: Add autoplay configuration for background musicThe AudioStreamPlayer is set up but doesn't appear to have autoplay enabled. If this is intended to play background music, consider setting autoplay.
34-39: Add tooltip or accessibility properties to ChestButtonThe button has an icon but no tooltip or accessibility text that would help players understand its purpose.
scenes/ChartViewport/StarryNight.gdshader (2)
18-22: Potential performance optimization for star generationThe star generation logic uses randomization and sine calculations which can be performance-intensive in shaders.
if (rand(SCREEN_UV.xy / 20.0) > 0.996) { float r = rand(SCREEN_UV.xy); - color = r * (0.85 * sin((TIME * time_scale) * (r * 5.0) + 720.0 * r) + 0.95); + // Pre-compute some values to reduce calculations + float phase = (TIME * time_scale) * (r * 5.0) + 720.0 * r; + color = r * (0.85 * sin(phase) + 0.95); }
6-9: Add documentation for shader parametersThe uniform variables would benefit from comments explaining their purpose and expected value ranges.
+// Color at the top of the gradient uniform vec4 bg_top_color; +// Color at the bottom of the gradient uniform vec4 bg_bottom_color; +// Controls how much of the screen is covered by the gradient (higher values = smaller gradient) uniform float gradient_ratio; +// Controls the speed of star twinkling animation uniform float time_scale;scenes/BattleDirector/scripts/BattleDirector.cs (1)
202-213: Enhance game state handling when a battle endsThe code pauses audio but doesn't clean up or reset other battle state properly. Consider adding full cleanup logic when battles end.
if (puppet == Enemy) { Audio.StreamPaused = true; GD.Print("Enemy is dead"); + // Clean up any ongoing battle effects, animations, or timers + // Reset battle-specific state variables ShowRewardSelection(3); }scenes/Remapping/ControlSettings.cs (2)
18-30: Ensure connections are properly established.The method connects buttons to event handlers but doesn't check if the nodes exist. Consider adding null checks to prevent runtime errors if the UI structure changes.
- GetNode<Button>("Panel/WASDButton").Connect("pressed", Callable.From(OnWASDButtonPressed)); + var wasdButton = GetNode<Button>("Panel/WASDButton"); + if (wasdButton != null) + { + wasdButton.Connect("pressed", Callable.From(OnWASDButtonPressed)); + } + else + { + GD.PrintErr("WASDButton not found"); + }
32-54: Consider centralizing the control scheme update logic.The three button handler methods have very similar code. Consider refactoring to reduce duplication by creating a helper method.
+ private void UpdateControlScheme(string scheme, string displayText) + { + GetNode<Label>("Panel/Label").Text = displayText; + ProjectSettings.SetSetting("game/input_scheme", scheme); + ProjectSettings.Save(); + ChangeKeySprites(scheme); + } private void OnWASDButtonPressed() { - GetNode<Label>("Panel/Label").Text = "WASD Selected"; - ProjectSettings.SetSetting("game/input_scheme", "WASD"); - ProjectSettings.Save(); - ChangeKeySprites("WASD"); + UpdateControlScheme("WASD", "WASD Selected"); } private void OnArrowButtonPressed() { - GetNode<Label>("Panel/Label").Text = "Arrow Selected"; - ProjectSettings.SetSetting("game/input_scheme", "ARROWS"); - ProjectSettings.Save(); - ChangeKeySprites("ARROWS"); + UpdateControlScheme("ARROWS", "Arrow Selected"); } private void OnQWERTButtonPressed() { - GetNode<Label>("Panel/Label").Text = "QWERT Selected"; - ProjectSettings.SetSetting("game/input_scheme", "QWERT"); - ProjectSettings.Save(); - ChangeKeySprites("QWERT"); + UpdateControlScheme("QWERT", "QWERT Selected"); }scenes/Maps/cartographer.tscn (1)
16-20: Consider using a more descriptive name for the background sprite.While functionally correct, the name "BG" could be more descriptive to indicate its specific role in the scene (e.g., "MapBackground").
-[node name="BG" type="Sprite2D" parent="."] +[node name="MapBackground" type="Sprite2D" parent="."]Classes/MidiMaestro/MidiMaestro.cs (1)
92-110: Well-encapsulated note information wrapper.The
midiNoteInfoclass effectively encapsulates the MIDI note data with clear accessor methods. Consider making this class immutable by adding readonly modifiers to fields and making properties instead of methods for a more C#-idiomatic approach.public class midiNoteInfo { private readonly Melanchall.DryWetMidi.Interaction.Note _note; private readonly TempoMap _tempoMap; public midiNoteInfo(Melanchall.DryWetMidi.Interaction.Note note, TempoMap tempoMap) { _note = note; _tempoMap = tempoMap; } - public long GetStartTimeTicks() => _note.Time; - public int GetStartTimeSeconds() => _note.TimeAs<MetricTimeSpan>(_tempoMap).Seconds; - public long GetEndTime() => _note.EndTime; - public long GetDuration() => _note.Length; + public long StartTimeTicks => _note.Time; + public int StartTimeSeconds => _note.TimeAs<MetricTimeSpan>(_tempoMap).Seconds; + public long EndTime => _note.EndTime; + public long Duration => _note.Length; }scenes/Puppets/Enemies/Boss1.tscn (1)
36-44: Consider adding parameters for blood particles customization.The blood particles are hardcoded with specific values. Consider exposing key parameters as exported variables in the PuppetTemplate script to allow for easier customization per enemy type.
scenes/ChartViewport/ChartManager.cs (1)
28-29: Consider implementing a clamp or guard for ChartLength.
Right now, there's only a comment advising not to go below 2000, but no actual logic enforces this. A minimum threshold check would prevent accidental out-of-bounds scenarios.Globals/StageProducer.cs (2)
15-15: Consider implementing the suggested state machine.
The TODO comment indicates a more robust state machine could improve maintainability and clarity of stage transitions.
39-42: ChangeCurRoom method sets a static room reference.
Ensure the calling site handles any necessary validation to avoid mismatched or invalid room assignments.Globals/FunkEngineNameSpace.cs (3)
148-157: PickRoomType with a 10% chance for Chest.
If rooms end up too repetitive or too scarce, you may want a more controlled distribution algorithm.
159-172: CreateCommonChildren links vertically stacked rooms.
The log statement might spam the console in production builds. Consider removing or reducing to a debug-level log.
174-186: AddBossRoom ensures a final boss stage at the bottom.
If multiple branching routes exist, confirm linking them all to one boss room is intentional gameplay design.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (29)
Audio/620230__josefpres__dark-loops-220-octave-piano-with-efect-short-loop-60-bpm.wavis excluded by!**/*.wavClasses/Notes/assets/heal_note.pngis excluded by!**/*.pngClasses/Notes/assets/quarter_note.pngis excluded by!**/*.pngClasses/Notes/assets/vampire_note.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/assets/Enemy1EmissionShape.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!**/*.png
📒 Files selected for processing (72)
.gitignore(1 hunks)Audio/620230__josefpres__dark-loops-220-octave-piano-with-efect-short-loop-60-bpm.wav.import(1 hunks)Classes/MidiMaestro/MidiMaestro.cs(1 hunks)Classes/MidiMaestro/SongTemplate.cs(1 hunks)Classes/Notes/Note.cs(4 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)Funk Engine.csproj(2 hunks)Funk Engine.sln.DotSettings.user(0 hunks)Globals/FunkEngineNameSpace.cs(2 hunks)Globals/Scribe.cs(4 hunks)Globals/StageProducer.cs(4 hunks)Globals/TimeKeeper.cs(1 hunks)README.md(1 hunks)project.godot(2 hunks)scenes/BattleDirector/NotePlacementBar.tscn(3 hunks)scenes/BattleDirector/assets/BattleFrame1.png.import(1 hunks)scenes/BattleDirector/assets/bgupdate.png.import(1 hunks)scenes/BattleDirector/scripts/BattleDirector.cs(4 hunks)scenes/BattleDirector/scripts/Conductor.cs(4 hunks)scenes/BattleDirector/scripts/NotePlacementBar.cs(3 hunks)scenes/BattleDirector/scripts/TextParticle.cs(1 hunks)scenes/BattleDirector/test_battle_scene.tscn(2 hunks)scenes/ChartViewport/ChartManager.cs(2 hunks)scenes/ChartViewport/ChartViewport.tscn(1 hunks)scenes/ChartViewport/LoopMarker.png.import(1 hunks)scenes/ChartViewport/Loopable.cs(1 hunks)scenes/ChartViewport/StarryNight.gdshader(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(2 hunks)scenes/Puppets/Enemies/Boss1.tscn(1 hunks)scenes/Puppets/Enemies/EnemyPuppet.tscn(1 hunks)scenes/Puppets/Enemies/assets/Boss1EmissionShape.tres(1 hunks)scenes/Puppets/Enemies/assets/Enemy1.png.import(1 hunks)scenes/Puppets/Enemies/assets/Enemy1EmissionShape.png.import(1 hunks)scenes/Puppets/PlayerPuppet.tscn(1 hunks)scenes/Puppets/scripts/PlayerPuppet.cs(1 hunks)scenes/Puppets/scripts/PlayerStats.cs(1 hunks)scenes/Puppets/scripts/PuppetTemplate.cs(2 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(2 hunks)scenes/SceneTransitions/testTransition.tscn(0 hunks)scenes/UI/scripts/MenuModule.cs(1 hunks)scenes/UI/scripts/RewardSelect.cs(3 hunks)
💤 Files with no reviewable changes (2)
- Funk Engine.sln.DotSettings.user
- scenes/SceneTransitions/testTransition.tscn
✅ Files skipped from review due to trivial changes (28)
- README.md
- scenes/NoteManager/assets/arrow_outline.png.import
- scenes/Remapping/assets/W_Key_Light.png.import
- .gitignore
- scenes/Puppets/Enemies/assets/Enemy1EmissionShape.png.import
- scenes/ChartViewport/LoopMarker.png.import
- scenes/Remapping/assets/R_Key_Light.png.import
- Classes/Notes/assets/quarter_note.png.import
- Classes/Notes/assets/vampire_note.png.import
- scenes/Maps/assets/BossIcon.png.import
- scenes/Maps/assets/BattleIcon.png.import
- scenes/Remapping/assets/D_Key_Light.png.import
- scenes/Maps/assets/ChestIcon.png.import
- scenes/Remapping/assets/S_Key_Light.png.import
- scenes/BattleDirector/assets/bgupdate.png.import
- scenes/Maps/assets/Player.png.import
- scenes/BattleDirector/assets/BattleFrame1.png.import
- scenes/Remapping/assets/Arrow_Up_Key_Light.png.import
- scenes/Remapping/assets/E_Key_Light.png.import
- scenes/Remapping/assets/A_Key_Light.png.import
- scenes/Remapping/assets/Arrow_Down_Key_Light.png.import
- scenes/Remapping/assets/Arrow_Left_Key_Light.png.import
- scenes/Puppets/Enemies/assets/Enemy1.png.import
- Audio/620230__josefpres__dark-loops-220-octave-piano-with-efect-short-loop-60-bpm.wav.import
- scenes/Puppets/Enemies/assets/Boss1EmissionShape.tres
- scenes/NoteManager/assets/new_arrow.png.import
- scenes/Remapping/assets/T_Key_Light.png.import
- Classes/Notes/assets/heal_note.png.import
🧰 Additional context used
🪛 GitHub Check: build
Classes/MidiMaestro/MidiMaestro.cs
[warning] 19-19:
The field 'MidiMaestro.strippedSong' is never used
[warning] 19-19:
The field 'MidiMaestro.strippedSong' is never used
🔇 Additional comments (108)
scenes/Remapping/assets/Q_Key_Light.png.import (1)
1-34: LGTM - Standard Godot texture import configuration.This is an auto-generated configuration file created by the Godot engine for importing the Q_Key_Light.png texture asset. The settings are typical defaults for 2D UI textures, including no compression, no mipmaps, and appropriate processing options.
scenes/BattleDirector/scripts/TextParticle.cs (1)
10-10: Good addition for proper layering controlSetting the
ZIndexto 2 ensures that text particles render above other elements in the battle scene, improving visual clarity during gameplay.Globals/TimeKeeper.cs (1)
11-14: Well-implemented utility for positive modulo operationsThe
PosModimplementation correctly ensures the result is always non-negative, which is particularly useful in rhythm games for wrapping time values or positions within loops. The method is well-named and uses appropriate types for precise timing calculations.Funk Engine.csproj (1)
11-11:Details
✅ Verification successful
Good addition of MIDI processing library
Adding the Melanchall.DryWetMidi package aligns well with the PR title "Midi readin" and enables MIDI file processing capabilities. The reference is correctly formatted in the project file.
🌐 Web query:
Is Melanchall.DryWetMidi 7.2.0 the latest stable version?💡 Result:
Melanchall.DryWetMidi 7.2.0 is confirmed as the latest stable version of the library across multiple sources[3][5][8][9]. Key points:
- NuGet listings explicitly show version 7.2.0 as current, with September 2024 release dates[3][5]
- GitHub releases detail breaking changes and new features in 7.2.0 (November 2024)[8]
- Unity Asset Store integration instructions reference 7.2.0 specifically[9]
The library continues active development with recent updates including:
- Unified object processing methods
- Improved
PatternBuilderpiano roll support- Enhanced sanitization settings[8]
Developers can install it via NuGet (
Install-Package Melanchall.DryWetMidi -Version 7.2.0) or download binaries directly from GitHub releases[3][8]. A nativeless package variant (7.2.0) is also available for specific deployment needs[4].Citations:
- 1: https://midi.org/community/midi-software/drywetmidi-5-0-0-net-library-to-work-with-midi-files-and-midi-devices
- 2: https://melanchall.github.io/drywetmidi/api/Melanchall.DryWetMidi.Core.MidiChunk.html
- 3: https://www.nuget.org/packages/Melanchall.DryWetMidi/
- 4: https://www.nuget.org/packages/Melanchall.DryWetMidi.Nativeless
- 5: https://www.nuget.org/profiles/melanchall
- 6: https://github.com/melanchall/drywetmidi/issues
- 7: https://melanchall.github.io/drywetmidi/articles/dev/Using-in-Unity.html
- 8: https://github.com/melanchall/drywetmidi/releases
- 9: https://assetstore.unity.com/packages/tools/audio/drywetmidi-222171
- 10: https://pvs-studio.com/en/blog/posts/csharp/1022/
MIDI Processing Library Addition Verified
The addition of the Melanchall.DryWetMidi package is solid and aligns well with the PR's goal of enhancing MIDI file processing. The package reference in
Funk Engine.csprojis properly formatted, and version 7.2.0 is confirmed to be the latest stable release.
- Confirmed package version via multiple sources.
- Reference in the project file is correctly added.
scenes/Remapping/assets/Arrow_Right_Key_Light.png.import (1)
1-34: Texture import file looks good for UI element.This auto-generated import file for
Arrow_Right_Key_Light.pnghas appropriate settings for a UI texture element:
- No compression (mode=0)
- No mipmaps (mipmaps/generate=false)
- Alpha border fixing enabled
These settings are consistent with other UI texture imports mentioned in the summary.
scenes/BattleDirector/scripts/NotePlacementBar.cs (1)
120-120: Good use of Math.Min to prevent value overflow.Using Math.Min here ensures that _currentBarValue never exceeds MaxValue, which is a robust approach to value clamping.
scenes/Puppets/Enemies/EnemyPuppet.tscn (1)
13-17: Health bar positioning looks good.The health bar is properly positioned with appropriate offsets from the center of the puppet.
scenes/Puppets/PlayerPuppet.tscn (2)
13-14: Good sprite setup.The sprite node is properly configured with a texture, which is better than the enemy puppet implementation.
16-20: Health bar uses consistent layout with enemy puppet.The health bar has the same positioning offsets as the enemy puppet, which creates a consistent UI experience. This is good practice.
scenes/Remapping/ControlSchemes.cs (1)
4-79:Details
✅ Verification successful
Verify that all asset files referenced in SpriteMappings exist.
The code references several sprite assets for different key configurations. Ensure that all the referenced PNG files actually exist at the specified paths to avoid runtime errors.
🏁 Script executed:
#!/bin/bash # Check if all the referenced sprite assets exist echo "Checking if all sprite assets referenced in ControlSchemes.cs exist..." missing_files=0 # Extract all the asset paths from SpriteMappings asset_paths=( "res://scenes/Remapping/assets/A_Key_Light.png" "res://scenes/Remapping/assets/D_Key_Light.png" "res://scenes/Remapping/assets/W_Key_Light.png" "res://scenes/Remapping/assets/S_Key_Light.png" "res://scenes/Remapping/assets/Arrow_Left_Key_Light.png" "res://scenes/Remapping/assets/Arrow_Right_Key_Light.png" "res://scenes/Remapping/assets/Arrow_Up_Key_Light.png" "res://scenes/Remapping/assets/Arrow_Down_Key_Light.png" "res://scenes/Remapping/assets/Q_Key_Light.png" "res://scenes/Remapping/assets/R_Key_Light.png" "res://scenes/Remapping/assets/E_Key_Light.png" ) for path in "${asset_paths[@]}"; do # Remove the res:// prefix and check if the file exists file_path=${path#"res://"} if [ ! -f "$file_path" ]; then echo "Missing file: $file_path" missing_files=$((missing_files+1)) fi done if [ $missing_files -eq 0 ]; then echo "All sprite assets exist." else echo "Found $missing_files missing sprite assets." fiLength of output: 2467
Asset Files Verification Passed
The shell script confirmed that all sprite asset files referenced in
ControlSchemes.SpriteMappingsexist. No changes are needed.scenes/NoteManager/scripts/NoteArrow.cs (1)
16-20: Properly exported Sprite2D properties for editor configuration.The
OutlineSpriteandIconSpriteproperties are correctly exported, which allows them to be set in the Godot editor.scenes/Puppets/scripts/PlayerPuppet.cs (2)
10-10: Good use of null-coalescing operator for defensive programming.Using the null-coalescing operator (
??) to provide a fallback whenStageProducer.PlayerStatsis null improves code robustness and prevents potential null reference exceptions.
15-15: Setting UniqName for identification purposes.Setting
UniqNameto "Player" helps with identification and potentially with lookup operations elsewhere in the code. This is a straightforward and appropriate change.scenes/ChestScene/assets/Chest.png.import (2)
5-6: Proper asset UID and path updates.The changes to the UID and path correctly reflect the new texture being imported. This is an auto-generated file by Godot's import system.
13-14: Correct source and destination file paths.The source and destination file paths have been properly updated to reference the new Chest.png file instead of the previous CoolBG.jpg file.
scenes/ChestScene/ChestScene.cs (3)
1-3: Imports look fine.No issues noted with these using statements.
21-27: Confirm forced focus is desired.Calling
ChestButton.GrabFocus()each frame can override other UI elements. Ensure this is intentional for your design.
29-33: Reward acquisition flow looks correct.Disabling the chest button and creating the
RewardSelectis straightforward. Good use of event subscription to triggerEndBattleafterward.scenes/ChartViewport/Loopable.cs (3)
8-8: Ensure Sprite2D meets requirements.Switching the base class from
TextureRecttoSprite2Dcan affect signals, positioning, and drawing. Double-check that references and functionalities remain valid.
11-11: Default offset is clear.Exporting
LoopOffsetoffers easy adjustment in the inspector. This value seems logical for a loop threshold.
17-17: EnsureTimeKeeper.ChartLengthis set appropriately.If
ChartLengthis uninitialized or set after this point, you could end up with incorrect interval calculations. Confirm the initialization order.scenes/Remapping/Remap.tscn (3)
32-55: UI button layout is well-structured.The three control scheme buttons (WASD, Arrow Keys, and QWER) are well-positioned and organized. Their texts clearly indicate their purpose to the user.
124-142: Consistent sprite scaling.The directional key sprites have consistent scaling (0.62) which provides visual uniformity. The positions are appropriately spaced for clear visual representation of the control mapping.
1-16:Details
❓ Verification inconclusive
Scene structure looks good, but verify node paths.
The Remap scene is well-organized with clear parent-child relationships. The node paths defined in the root node correctly reference the sprite nodes that will be modified when changing control schemes.
🏁 Script executed:
#!/bin/bash # Verify the ControlSettings script exists and contains the necessary code to update the sprites rg -A 10 "extends Node2D" scenes/Remapping/ControlSettings.csLength of output: 63
Action: Verify ControlSettings.cs Implementation and Node Path Accuracy
The Remap scene is well-organized and the node paths in the .tscn file appear correctly set up. However, the initial verification command searching for
extends Node2Dinscenes/Remapping/ControlSettings.csreturned no output. This could indicate that the script either extends a different class or that the expected code isn’t present under that search pattern.
- Please manually verify that
scenes/Remapping/ControlSettings.cscontains the necessary logic to update the sprites (e.g., confirm which class it extends and that the sprite-update methods are implemented).- Ensure the node paths referenced in the Remap scene (
Panel/Control/LeftRemap, etc.) match the actual node structure.Globals/Scribe.cs (3)
4-4: Import for MidiMaestro added correctly.The new import for the MidiMaestro namespace aligns with the introduction of the SongDictionary array.
49-83: New note types add gameplay variety.The three new note types (PlayerHeal, PlayerVampire, PlayerQuarter) add interesting gameplay mechanics:
- PlayerHeal provides healing based on timing
- PlayerVampire combines damage and healing in one note
- PlayerQuarter costs less (0.25 instead of 1.0) while maintaining normal damage
These additions enhance gameplay depth and player strategy.
17-17:Details
❓ Verification inconclusive
Verify the null texture for EnemyBase.
The EnemyBase note's texture has been changed to null. Ensure this is intentional and won't cause rendering issues in the game.
🏁 Script executed:
#!/bin/bash # Check if the EnemyBase note is rendered anywhere in the codebase rg -A 5 "EnemyBase" --glob "*.{cs,tscn}" scenes/Length of output: 48
Action Required: Verify EnemyBase Null Texture
The grep command in the scenes directory returned no references to EnemyBase, which means its usage isn’t immediately obvious from that location. Please perform a manual check to confirm that setting the EnemyBase note's texture to null is intentional and that no dependent rendering logic assumes a non-null texture.
- Confirm that no other parts of the project require a valid texture for EnemyBase.
- Verify that any fallback mechanisms or error handling for a null texture behave as expected during runtime.
scenes/SceneTransitions/TitleScreen.tscn (2)
106-106:Details
✅ Verification successful
ScenePath value updated - verify integrity.
The ScenePath for the "Quit Game" button has changed from 2 to 4. Ensure all scene transition code references the correct scene index.
🏁 Script executed:
#!/bin/bash # Look for any other scene transition references that might need updating rg "ScenePath = [2-4]" --glob "*.{cs,tscn}" scenes/Length of output: 108
ScenePath update verified – no further changes required.
- The scan confirms that the updated reference (
ScenePath = 4) inscenes/SceneTransitions/TitleScreen.tscnis the only instance within the scanned files.- All scene transition code appears to reference the correct scene index for the "Quit Game" button.
72-72:Details
✅ Verification successful
ScenePath value updated - verify integrity.
The ScenePath for the "Start Game" button has changed from 3 to 5. Ensure all scene transition code references the correct scene index.
🏁 Script executed:
#!/bin/bash # Check the implementation of SceneChange.cs to understand how ScenePath values are used rg -A 15 "ScenePath" scenes/SceneTransitions/scripts/SceneChange.csLength of output: 544
ScenePath Change Verified: No further adjustments required.
- The
ScenePathvalue inscenes/SceneTransitions/TitleScreen.tscnhas been updated from 3 to 5.- In
scenes/SceneTransitions/scripts/SceneChange.cs, the exportedScenePathis correctly logged and passed toTransitionStage(), ensuring that the updated scene index is used consistently.Classes/Notes/Note.cs (4)
14-14: CostModifier property added with appropriate encapsulation.The new CostModifier property has a public getter and private setter, ensuring it can only be modified during note creation.
27-28: Constructor parameter with default value.Adding the costModifier parameter with a default value of 1.0f maintains compatibility with existing code while allowing creation of special notes like PlayerQuarter.
43-43: Property initialization in constructor.The CostModifier property is correctly initialized in the constructor.
55-55: Clone method updated correctly.The Clone method now passes the CostModifier to ensure that cloned notes maintain the same cost properties as the original.
scenes/Puppets/scripts/PuppetTemplate.cs (1)
34-35: Ensure consistent initialization in _ReadySince you've removed the sprite creation code and converted to exported properties, ensure all required initializations are present in the _Ready method.
Could there be other initializations that were previously handled in code that might now need to be set through the editor?
scenes/ChartViewport/StarryNight.gdshader (1)
1-3: Good attribution practice!Proper attribution to the original shader author is excellent practice. Consider adding a license reference if the original has one.
scenes/BattleDirector/scripts/BattleDirector.cs (2)
119-123: Good encapsulation of battle end logicThe addition of a dedicated
EndBattlemethod improves code organization and maintainability by centralizing the battle conclusion logic.
218-218: Good use of event-based communicationUsing the
Selectedevent from the reward selection to triggerEndBattleis a clean approach for handling sequential game flow.scenes/BattleDirector/test_battle_scene.tscn (6)
1-8: Visual enhancements added with new resources.The scene has been enriched with new scripts and textures that enhance the battle scene's visual presentation and functionality. The MenuModule.cs script integration will help manage UI interactions.
12-19: Well-defined gradient for visual styling.The gradient resource is properly configured with good color choices that likely match the game's visual theme. The GradientTexture2D implementation with custom fill parameters creates an interesting visual effect.
42-42: Ensure layout adjustments are consistent with other UI elements.The SubViewport's offset_top has been changed from 160.0 to 180.0. Verify that this adjustment aligns properly with other UI components that might depend on this positioning.
50-55: Good use of z-index for proper layer ordering.Setting the z-index to -1 ensures the NPBBacking appears behind other elements in the NotePlacementBar, which is the correct approach for a background element.
57-68: Clean implementation of background and frame elements.The BackGround and BattleFrame TextureRect nodes are properly configured with appropriate z-indices to ensure correct rendering order. The texture assets are properly referenced, creating a more visually appealing battle scene.
46-48:Details
❓ Verification inconclusive
NotePlacementBar positioning needs verification.
Setting both offset_top and offset_bottom to the same value (183.0) means the node has zero height. This may cause rendering issues if child elements depend on the parent's size.
🏁 Script executed:
#!/bin/bash # Check if the NotePlacementBar script handles zero-height nodes correctly rg -A 10 "NotePlacementBar" scenes/BattleDirector/scripts/Length of output: 7153
Verify NotePlacementBar Offsets in Test Scene
The test scene defines the NotePlacementBar with both
offset_topandoffset_bottomset to 183.0, which results in a zero-height node. My search in the NotePlacementBar script did not reveal any specific handling for zero-height nodes. Please verify that this configuration is intentional. If a visible, sized container is expected for proper placement or rendering of child elements, consider revisiting these offset values or updating the script logic accordingly.
- File:
scenes/BattleDirector/test_battle_scene.tscn(Lines 46-48)- Related Script:
scenes/BattleDirector/scripts/NotePlacementBar.cs(no special zero-height handling detected)scenes/Remapping/ControlSettings.cs (1)
4-16: Good structure with exported sprite references.The class is well-structured with properly exported Sprite2D fields that will allow them to be set in the Godot editor.
scenes/NoteManager/note.tscn (3)
1-5: Improved visual assets for note arrows.The change from the old arrow texture to the new one with an additional outline texture enhances the visual representation of notes, which is important for a rhythm game.
7-11: Well-structured node references for the sprite components.The use of node_paths to explicitly reference the OutlineSprite and IconSprite child nodes is good practice for maintaining clean relationships between nodes in a scene.
13-16: Separate sprite layers for better visual control.Splitting the arrow into separate Outline and Icon sprites allows for more flexible visual effects, such as changing colors or applying animations independently to each layer.
scenes/Maps/cartographer.tscn (4)
1-6: Enhanced scene with additional resources.The addition of background and player textures along with the MenuModule script enhances the cartographer scene's visual presentation and functionality.
8-11: Good use of node references.The Cartographer node properly references the PlayerSprite through node_paths, which is a good practice for maintaining clean node relationships.
13-14: UI layer integration enhances user experience.The addition of the UI layer with the MenuModule script provides consistency with the battle scene UI, improving the overall user experience.
22-24: Well-configured player sprite.The Player sprite is properly configured with an appropriate z-index to ensure it renders above other elements in the scene.
project.godot (3)
13-16: Project rebranding looks good.The project name has been updated to "ProjectFunkEngine" with a new icon referring to a character asset, which aligns with the game's theme.
35-37: New control scheme configuration added.The addition of the [game] section with input_scheme="QWERT" establishes the default control scheme for the game. This is a good practice to set up configuration parameters in the project settings.
45-46: Extended input mappings for different control schemes.The additional key mappings for arrows (X, E, C, W, Z, Q, V, R) expand the game's input options, likely to support the "QWERT" control scheme mentioned in the [game] section. This implementation allows for flexible control configurations.
Also applies to: 53-54, 61-62, 69-70
Classes/MidiMaestro/MidiMaestro.cs (2)
9-22: Well-structured MIDI processor class initialization.The MidiMaestro class is designed to handle MIDI file processing with clear member variables for storing parsed note data by direction. Good job with the organization.
🧰 Tools
🪛 GitHub Check: build
[warning] 19-19:
The field 'MidiMaestro.strippedSong' is never used
[warning] 19-19:
The field 'MidiMaestro.strippedSong' is never used
73-88: Clean API design for retrieving note data.The methods
GetNotesandGetSongDataprovide a clear interface for retrieving the processed MIDI data. The switch expression inGetNotesis a modern and concise approach.scenes/Puppets/Enemies/Boss1.tscn (4)
1-7: Scene setup with appropriate resources.The scene correctly loads the necessary resources including the PuppetTemplate script, enemy texture, emission shape, and health bar scene. Good job organizing these dependencies.
8-24: Well-configured particle and visual effects.The particle system is configured with appropriate emission parameters and a custom gradient for the blood effect. The visual styling fits the theme of a boss enemy.
25-31: Node structure with clear property assignments.The main EnemPuppet node is properly configured with references to its health bar and sprite components, along with appropriate starting position and scale. The use of
node_pathsfor component references is a good practice in Godot.
45-49: Health bar positioning is appropriate.The progress bar is positioned below the enemy sprite with sufficient width for visibility. The offset values create a good layout.
scenes/ChartViewport/ChartViewport.tscn (6)
1-7: Resources properly loaded and organized.The scene correctly loads necessary resources including the new LoopMarker texture and StarryNight shader. The organization follows good practices for Godot scene structure.
9-14: Well-configured shader material with appropriate parameters.The ShaderMaterial is properly set up with clear parameter names and appropriate default values. The color gradient parameters create a visually appealing background effect.
18-18: Viewport size adjustment.The viewport height has been adjusted from 200 to 180 pixels. Ensure this change is consistent with any UI elements or game logic that might depend on the viewport dimensions.
Could this change affect the positioning of notes or other elements in the chart? Please verify that all dependent UI components adjust properly to this new size.
Also applies to: 25-25
29-29: Camera position change.The camera has been moved from position (-25, 0) to (-50, 0), which shifts the viewing area. Make sure this change is coordinated with any gameplay elements that depend on the camera's field of view.
Please check that all notes and visual elements remain properly visible with this new camera position.
32-39: New background shader implementation looks good.The StarShader ColorRect with the custom shader material provides a nice visual background for the chart. The positioning and scaling are appropriate for covering the viewport area.
45-49: Loop marker feature added.The addition of a LoopMarker sprite with a LoopOffset property of 0.0 is a good enhancement for the chart system. This will help visualize loop points in the music tracks.
scenes/ChartViewport/ChartManager.cs (4)
93-94: Ensure loop arrow is properly cleaned up or reused.
Creating a second arrow for looping visuals is a good approach, but make sure it won’t remain idle if the loop doesn't come around. Verify that it’s disabled or removed when not needed.
97-98: Switch to SelfModulate aligns with Godot 4 best practices.
The new property ensures the color is set at the node level independently of children. The usage here looks correct.
104-104: Method signature updated to accept a Note object.
Ensure all callers match this updated signature and pass validNoteobjects as needed for your note creation flow.
108-109: Potential for null checks when accessing IH.Arrows.
IfArrowsis uninitialized or the index is out of range, this could result in errors. Consider adding safety checks or default fallback behavior.Globals/StageProducer.cs (7)
22-23: New static BattleConfig field introduced.
If multiple threads or coroutines might use/set this field, consider synchronizing updates or using a thread-safe pattern.
46-46: Transitioning to the next room by index.
Verify thatnextRoomIdxalways falls within the bounds of theMap.GetRooms()array, and the returned room’sTypeis valid.
49-49: Expanded TransitionStage signature to include nextRoomIdx.
This clarifies room-specific transitions and is a helpful addition to the method’s usage.
59-59: Setting Config in the Battle stage.
Ensure any additional stage data is also properly set before the scene changes. If other components rely on Config, confirm it’s fully populated here.
62-64: New stage type: Controls.
The transition is straightforward; no custom config is assigned. This looks correct given the current logic.
65-68: Introducing a Chest stage option.
Similar to Battle, a config is set for the Chest scene. Verify that the Chest scene references or uses Config as intended.
88-104: MakeConfig method centralizes battle configuration generation.
Separating this logic from TransitionStage is beneficial. Keep an eye on expansion as your config logic grows.Globals/FunkEngineNameSpace.cs (11)
1-3: New using directives introduced.
These references for System, System.Linq, and Godot seem appropriate, but confirm System.Linq is necessary.
7-12: SongData struct encapsulates BPM, length, and loop count.
Negative or zero values for SongLength could lead to unexpected behavior. Decide if you want to enforce positive values here.
14-20: ArrowData struct introduced.
Holds arrow details like color, key, and associated node. Confirm that all fields are properly initialized before use, especiallyNoteChecker Node.
39-45: BattleConfig struct provides room and song metadata.
This is a clean way to encapsulate data for combat. Should you need more fields, you can expand it without altering method signatures.
47-53: Restored BattleEffectTrigger enum.
Double-check that these triggers integrate correctly with the new or existing battle logic and events.
59-60: Added 'Chest' and 'Controls' stages to Stages enum.
Ensure switch statements and logic in other files handle these new stages.Also applies to: 63-64
66-66: New MapGrid class introduced.
Centralizes map layout and room data. This is a positive move for maintainability.
68-70: Private fields for map data and indexing.
Make sure_roomsis large enough or resizes properly if the grid grows bigger than expected.
77-103: New nested Room class.
Tracks position, type, and child connections. Consider minimal validation or checks to keep the_roomsstructure consistent.
105-122: InitMapGrid handles map setup and boss room addition.
Guard against extreme grid sizes or unreachable paths if width or height are too small or large.
124-146: Recursive GeneratePath_r method.
Random path creation can be tricky. Ensure you won't get stuck if the random x coordinate continually picks invalid spaces.scenes/BattleDirector/NotePlacementBar.tscn (7)
13-14: Texture width adjustments optimize UI displayThe width of both gradient textures has been increased (from 26 to 34 and from 24 to 32) to provide better visual representation for the note placement bar.
Also applies to: 22-23
17-18: Gradient color scheme and offset adjustmentsThe gradient offsets have been modified to create a more balanced visual progression, and the colors array has been updated accordingly. These changes will create a smoother visual transition in the progress bar.
41-44: Adjusted ProgressBar positioningThe ProgressBar's position has been adjusted for better alignment with other UI elements. The new offset values provide a more balanced layout.
51-56: TextEdit positioning and appearance updatesThe TextEdit element has been repositioned with new offset values and its text content now includes leading spaces. The addition of
z_as_relative = falseensures proper layering in the UI hierarchy.
65-68: NoteQueueSprite repositioning and rotationThe NoteQueueSprite has been significantly repositioned and rotated by -90 degrees (−1.5708 radians). This change appears to be part of a UI redesign to better accommodate note visualization.
70-73: Redesigned CurrentNote and NextNote positioningBoth the CurrentNote and NextNote sprites have been repositioned and rotated by 90 degrees (1.5708 radians) to match the new NoteQueueSprite orientation. This creates a more intuitive visualization of the note queue.
Also applies to: 75-78
36-37:Details
❓ Verification inconclusive
Verify the swapped NodePath references
The references to
_currentNoteand_nextNotehave been swapped. Ensure this change is intentional and that the NotePlacementBar.cs script handles this swap correctly.
🏁 Script executed:
#!/bin/bash # Search for references to _currentNote and _nextNote in NotePlacementBar.cs rg "_currentNote|_nextNote" scenes/BattleDirector/scripts/NotePlacementBar.cs -A 3 -B 3Length of output: 1272
Action: Confirm the swapped NodePath references in the scene file
It appears that in
scenes/BattleDirector/NotePlacementBar.tscnthe NodePath assignments are reversed:_currentNote = NodePath("NoteQueueSprite/NextNote") _nextNote = NodePath("NoteQueueSprite/CurrentNote")In the corresponding
NotePlacementBar.cs, these variables are used as follows:
_currentNote.Textureis set using_currentNoteInstance.Texture_nextNote.Textureis set using the texture from the note queuePlease verify that this inversion is intentional. Specifically:
- Confirm that the node at
NoteQueueSprite/NextNoteis meant to display the current note’s texture.- Ensure that the node at
NoteQueueSprite/CurrentNoteis intended for displaying the next note’s texture.If this swap is by design, consider adding a comment in the scene or code file to clarify the rationale; otherwise, the assignments may need to be reverted to match the expected behavior.
scenes/Maps/scripts/Cartographer.cs (8)
8-15: Added player sprite representation and focus managementThe addition of the PlayerSprite property and focus management variables enables visual representation of the player's position on the map and improves navigation control.
16-20: Added focus change event handlingSubscribing to the GuiFocusChanged event enables the Cartographer to track and respond to focus changes in the UI, enhancing user interaction.
22-29: Focus restoration mechanismThis new _Process method ensures that focus remains on a valid button when the game is not paused, preventing focus loss during gameplay.
30-34: Focus tracking mechanismThe UpdateFocus method correctly updates the currently focused button, but only if it's one of the valid buttons in the map interface.
44-44: Improved map drawing with focus neighborsThe DrawMap method now orders buttons by X position and sets up focus relationships between them, creating a more intuitive left-to-right navigation flow.
Also applies to: 56-58
60-97: Enhanced room visualization with buttonsThe DrawMapSprite method has been significantly improved to:
- Use interactive buttons instead of static sprites
- Handle focus and disable states based on room accessibility
- Set appropriate icons based on room types
- Update player sprite position for the current room
This transformation creates a more interactive map interface and improves player feedback.
99-108: Circular focus navigationThe AddFocusNeighbors method creates circular navigation between map buttons, allowing players to cycle through available rooms using directional inputs. This is particularly useful for gamepad navigation.
110-125: Improved stage transition with animationsThe EnterStage method now:
- Disables all buttons during transition
- Creates a smooth tween animation for the player sprite
- Transitions to the new room after animation completes
This creates a more polished and visually appealing transition between map rooms.
scenes/NoteManager/note_manager.tscn (5)
1-1: Updated resource loading with new arrow assetsThe scene now uses new arrow textures and includes a dedicated outline texture, improving visual clarity of note indicators.
Also applies to: 4-4, 6-6
14-23: Added Left Arrow with outlineA new left arrow has been added with an outline sprite, completing the set of directional indicators. The rotation (3.14159 or π radians) orients it correctly to the left.
25-27: Consistent arrow styling with outlinesAll arrows (up, down, right) have been updated with:
- New consistent positioning
- Updated textures
- Child outline sprites for better visibility
This creates a unified visual style for all directional inputs and improves visibility of the arrows against different backgrounds.
Also applies to: 30-32, 35-37, 40-42, 45-46, 49-51
60-60: Updated divider positionsThe divider positions have been adjusted to align with the new arrow positions, maintaining proper spacing even though they're currently hidden.
Also applies to: 66-66, 72-72
56-56:Details
❓ Verification inconclusive
Hidden dividers
The dividers node is now hidden by default. Consider whether this intentionally removes visual lane separation or if the dividers should be made visible in certain gameplay contexts.
🏁 Script executed:
#!/bin/bash # Check if there are references to showing/hiding the dividers in the codebase rg "dividers.*visible" --type csLength of output: 34
Let's re-verify the context because the previous search was limited to C# files. We should check the actual scene file to inspect how the "dividers" node is referenced and whether its visibility state is intentionally set.
I'm generating a new shell script to search within the
scenes/NoteManager/note_manager.tscnfile for references to the "dividers" node:Once this result is available, we can determine if the hidden state (i.e. "visible = false") is an intended change or if further adjustments are needed in code or documentation.
🏁 Script executed:
#!/bin/bash # Check for references to the "dividers" node in the NoteManager scene file rg -n "dividers" scenes/NoteManager/note_manager.tscnLength of output: 282
Dividers Visibility: Design Confirmation Needed
At line 56 in
scenes/NoteManager/note_manager.tscn, thevisible = falsesetting hides thedividersnode by default. The scene still defines thedividersnode along with its child nodes (DivA, DivB, DivC). Could you please confirm whether this change is intentional and that visual lane separation isn’t required during gameplay? If the dividers should appear under certain conditions, consider revising the visibility logic or adding documentation to clarify the intended behavior.
| 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 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential null reference with StageProducer
The code accesses StageProducer.PlayerStats but doesn't verify if StageProducer itself is null, which could cause a runtime exception.
- invenMenu.Display(StageProducer.PlayerStats ?? new PlayerStats()); //For now work around for testing
+ var playerStats = StageProducer?.PlayerStats ?? new PlayerStats();
+ invenMenu.Display(playerStats); //For now work around for testing📝 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.
| invenMenu.Display(StageProducer.PlayerStats ?? new PlayerStats()); //For now work around for testing | |
| var playerStats = StageProducer?.PlayerStats ?? new PlayerStats(); | |
| invenMenu.Display(playerStats); //For now work around for testing |
| 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; | ||
| } | ||
| 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.
💡 Verification agent
🧩 Analysis chain
Add null check for StageProducer and consider menu cleanup
The input handling implementation is good, but there's a potential null reference issue.
🏁 Script executed:
#!/bin/bash
# Check if StageProducer is ever null-checked before accessing PlayerStats
rg -A 3 -B 3 "StageProducer\s*\.\s*PlayerStats" --glob "*.cs"Length of output: 2105
Action Required: Null Check and Menu Cleanup Improvements
- The current implementation protects against a null
PlayerStatsby usingStageProducer.PlayerStats ?? new PlayerStats(), but it does not account for the possibility ofStageProduceritself being null. Consider adding an explicit null check (or using the safe navigation operator, e.g.,StageProducer?.PlayerStats ?? new PlayerStats(), if supported) to avoid a potential null reference exception. - Additionally, review the menu management logic. Without cleaning up or removing existing menus before instantiating and adding new ones, there's a risk of accumulating duplicate menu instances on the scene.
| private void LoadControlScheme() | ||
| { | ||
| string scheme = ProjectSettings.HasSetting("game/input_scheme") | ||
| ? (string)ProjectSettings.GetSetting("game/input_scheme") | ||
| : "ARROWS"; | ||
| 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(); | ||
| eventKey.Keycode = (Key)Enum.Parse(typeof(Key), selectedScheme[arrow.Key]); | ||
| InputMap.ActionAddEvent(arrow.Key, eventKey); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and validation to control scheme loading.
The implementation for dynamically loading control schemes looks good. However, it's missing validation and error handling for two potential issues:
- If an invalid scheme name is provided in project settings
- If a key in the selected scheme isn't a valid Key enum value
Consider adding error handling and validation:
private void LoadControlScheme()
{
string scheme = ProjectSettings.HasSetting("game/input_scheme")
? (string)ProjectSettings.GetSetting("game/input_scheme")
: "ARROWS";
+ // Validate that the scheme exists
+ if (!ControlSchemes.Schemes.ContainsKey(scheme))
+ {
+ GD.PrintErr($"Control scheme '{scheme}' not found, defaulting to ARROWS");
+ scheme = "ARROWS";
+ }
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))
{
+ try
+ {
InputEventKey eventKey = new InputEventKey();
eventKey.Keycode = (Key)Enum.Parse(typeof(Key), selectedScheme[arrow.Key]);
InputMap.ActionAddEvent(arrow.Key, eventKey);
+ }
+ catch (ArgumentException)
+ {
+ GD.PrintErr($"Invalid key '{selectedScheme[arrow.Key]}' for action '{arrow.Key}'");
+ }
}
}
}📝 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 LoadControlScheme() | |
| { | |
| string scheme = ProjectSettings.HasSetting("game/input_scheme") | |
| ? (string)ProjectSettings.GetSetting("game/input_scheme") | |
| : "ARROWS"; | |
| 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(); | |
| eventKey.Keycode = (Key)Enum.Parse(typeof(Key), selectedScheme[arrow.Key]); | |
| InputMap.ActionAddEvent(arrow.Key, eventKey); | |
| } | |
| } | |
| } | |
| private void LoadControlScheme() | |
| { | |
| string scheme = ProjectSettings.HasSetting("game/input_scheme") | |
| ? (string)ProjectSettings.GetSetting("game/input_scheme") | |
| : "ARROWS"; | |
| // Validate that the scheme exists | |
| if (!ControlSchemes.Schemes.ContainsKey(scheme)) | |
| { | |
| GD.PrintErr($"Control scheme '{scheme}' not found, defaulting to ARROWS"); | |
| scheme = "ARROWS"; | |
| } | |
| 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)) | |
| { | |
| try | |
| { | |
| InputEventKey eventKey = new InputEventKey(); | |
| eventKey.Keycode = (Key)Enum.Parse(typeof(Key), selectedScheme[arrow.Key]); | |
| InputMap.ActionAddEvent(arrow.Key, eventKey); | |
| } | |
| catch (ArgumentException) | |
| { | |
| GD.PrintErr($"Invalid key '{selectedScheme[arrow.Key]}' for action '{arrow.Key}'"); | |
| } | |
| } | |
| } | |
| } |
| public override void _Ready() | ||
| { | ||
| MM = new MidiMaestro("Audio/midi/midiTest2.mid"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid hardcoding file paths.
The MIDI file path is hardcoded, which reduces flexibility and makes it difficult to change the MIDI file without modifying the code.
Consider using one of these approaches:
- Make the file path configurable via a property that can be set in the editor
- Load the path from a configuration file
- Pass the path as a parameter when initializing the Conductor
public override void _Ready()
{
- MM = new MidiMaestro("Audio/midi/midiTest2.mid");
+ // Option 1: Use an export variable
+ if (string.IsNullOrEmpty(MidiFilePath))
+ {
+ MidiFilePath = "Audio/midi/midiTest2.mid"; // Default path
+ }
+ MM = new MidiMaestro(MidiFilePath);
}
// Add this property
[Export]
public string MidiFilePath { 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 override void _Ready() | |
| { | |
| MM = new MidiMaestro("Audio/midi/midiTest2.mid"); | |
| } | |
| public override void _Ready() | |
| { | |
| // Option 1: Use an export variable | |
| if (string.IsNullOrEmpty(MidiFilePath)) | |
| { | |
| MidiFilePath = "Audio/midi/midiTest2.mid"; // Default path | |
| } | |
| MM = new MidiMaestro(MidiFilePath); | |
| } | |
| // Add this property | |
| [Export] | |
| public string MidiFilePath { get; set; } |
| foreach (midiNoteInfo mNote in MM.GetNotes(ArrowType.Up)) | ||
| { | ||
| AddNoteToLane( | ||
| ArrowType.Up, | ||
| (int)(mNote.GetStartTimeSeconds() / (60 / (double)TimeKeeper.Bpm)), | ||
| Scribe.NoteDictionary[0] | ||
| ); | ||
| } | ||
| foreach (midiNoteInfo mNote in MM.GetNotes(ArrowType.Down)) | ||
| { | ||
| AddNoteToLane( | ||
| ArrowType.Down, | ||
| (int)(mNote.GetStartTimeSeconds() / (60 / (double)TimeKeeper.Bpm)), | ||
| Scribe.NoteDictionary[0] | ||
| ); | ||
| } | ||
| foreach (midiNoteInfo mNote in MM.GetNotes(ArrowType.Right)) | ||
| { | ||
| AddNoteToLane( | ||
| ArrowType.Right, | ||
| (int)(mNote.GetStartTimeSeconds() / (60 / (double)TimeKeeper.Bpm)), | ||
| Scribe.NoteDictionary[0] | ||
| ); | ||
| } | ||
| foreach (midiNoteInfo mNote in MM.GetNotes(ArrowType.Left)) | ||
| { | ||
| AddNoteToLane( | ||
| ArrowType.Left, | ||
| (int)(mNote.GetStartTimeSeconds() / (60 / (double)TimeKeeper.Bpm)), | ||
| Scribe.NoteDictionary[0] | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring repeated code blocks.
The four foreach loops for different arrow types contain identical logic with only the arrow type changing. This violates the DRY (Don't Repeat Yourself) principle.
-foreach (midiNoteInfo mNote in MM.GetNotes(ArrowType.Up))
-{
- AddNoteToLane(
- ArrowType.Up,
- (int)(mNote.GetStartTimeSeconds() / (60 / (double)TimeKeeper.Bpm)),
- Scribe.NoteDictionary[0]
- );
-}
-foreach (midiNoteInfo mNote in MM.GetNotes(ArrowType.Down))
-{
- AddNoteToLane(
- ArrowType.Down,
- (int)(mNote.GetStartTimeSeconds() / (60 / (double)TimeKeeper.Bpm)),
- Scribe.NoteDictionary[0]
- );
-}
-foreach (midiNoteInfo mNote in MM.GetNotes(ArrowType.Right))
-{
- AddNoteToLane(
- ArrowType.Right,
- (int)(mNote.GetStartTimeSeconds() / (60 / (double)TimeKeeper.Bpm)),
- Scribe.NoteDictionary[0]
- );
-}
-foreach (midiNoteInfo mNote in MM.GetNotes(ArrowType.Left))
-{
- AddNoteToLane(
- ArrowType.Left,
- (int)(mNote.GetStartTimeSeconds() / (60 / (double)TimeKeeper.Bpm)),
- Scribe.NoteDictionary[0]
- );
-}
+// Process all arrow types using a loop
+foreach (ArrowType arrowType in Enum.GetValues(typeof(ArrowType)))
+{
+ foreach (midiNoteInfo mNote in MM.GetNotes(arrowType))
+ {
+ AddNoteToLane(
+ arrowType,
+ (int)(mNote.GetStartTimeSeconds() / (60 / (double)TimeKeeper.Bpm)),
+ Scribe.NoteDictionary[0]
+ );
+ }
+}📝 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.
| foreach (midiNoteInfo mNote in MM.GetNotes(ArrowType.Up)) | |
| { | |
| AddNoteToLane( | |
| ArrowType.Up, | |
| (int)(mNote.GetStartTimeSeconds() / (60 / (double)TimeKeeper.Bpm)), | |
| Scribe.NoteDictionary[0] | |
| ); | |
| } | |
| foreach (midiNoteInfo mNote in MM.GetNotes(ArrowType.Down)) | |
| { | |
| AddNoteToLane( | |
| ArrowType.Down, | |
| (int)(mNote.GetStartTimeSeconds() / (60 / (double)TimeKeeper.Bpm)), | |
| Scribe.NoteDictionary[0] | |
| ); | |
| } | |
| foreach (midiNoteInfo mNote in MM.GetNotes(ArrowType.Right)) | |
| { | |
| AddNoteToLane( | |
| ArrowType.Right, | |
| (int)(mNote.GetStartTimeSeconds() / (60 / (double)TimeKeeper.Bpm)), | |
| Scribe.NoteDictionary[0] | |
| ); | |
| } | |
| foreach (midiNoteInfo mNote in MM.GetNotes(ArrowType.Left)) | |
| { | |
| AddNoteToLane( | |
| ArrowType.Left, | |
| (int)(mNote.GetStartTimeSeconds() / (60 / (double)TimeKeeper.Bpm)), | |
| Scribe.NoteDictionary[0] | |
| ); | |
| } | |
| // Process all arrow types using a loop | |
| foreach (ArrowType arrowType in Enum.GetValues(typeof(ArrowType))) | |
| { | |
| foreach (midiNoteInfo mNote in MM.GetNotes(arrowType)) | |
| { | |
| AddNoteToLane( | |
| arrowType, | |
| (int)(mNote.GetStartTimeSeconds() / (60 / (double)TimeKeeper.Bpm)), | |
| Scribe.NoteDictionary[0] | |
| ); | |
| } | |
| } |
| private void ChangeKeySprites(string scheme) | ||
| { | ||
| var selectedScheme = ControlSchemes.SpriteMappings[scheme]; | ||
| GD.Print(scheme); | ||
| leftKey.Texture = GD.Load<Texture2D>(selectedScheme["left"]); | ||
| rightKey.Texture = GD.Load<Texture2D>(selectedScheme["right"]); | ||
| upKey.Texture = GD.Load<Texture2D>(selectedScheme["up"]); | ||
| downKey.Texture = GD.Load<Texture2D>(selectedScheme["down"]); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for texture loading.
The method doesn't handle cases where a texture might fail to load or when a scheme doesn't exist in the mappings dictionary. Consider adding error handling to make the code more robust.
private void ChangeKeySprites(string scheme)
{
- var selectedScheme = ControlSchemes.SpriteMappings[scheme];
- GD.Print(scheme);
- leftKey.Texture = GD.Load<Texture2D>(selectedScheme["left"]);
- rightKey.Texture = GD.Load<Texture2D>(selectedScheme["right"]);
- upKey.Texture = GD.Load<Texture2D>(selectedScheme["up"]);
- downKey.Texture = GD.Load<Texture2D>(selectedScheme["down"]);
+ if (!ControlSchemes.SpriteMappings.TryGetValue(scheme, out var selectedScheme))
+ {
+ GD.PrintErr($"Control scheme '{scheme}' not found");
+ return;
+ }
+
+ try
+ {
+ if (leftKey != null) leftKey.Texture = GD.Load<Texture2D>(selectedScheme["left"]);
+ if (rightKey != null) rightKey.Texture = GD.Load<Texture2D>(selectedScheme["right"]);
+ if (upKey != null) upKey.Texture = GD.Load<Texture2D>(selectedScheme["up"]);
+ if (downKey != null) downKey.Texture = GD.Load<Texture2D>(selectedScheme["down"]);
+ }
+ catch (Exception e)
+ {
+ GD.PrintErr($"Failed to load textures for scheme '{scheme}': {e.Message}");
+ }
}📝 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 ChangeKeySprites(string scheme) | |
| { | |
| var selectedScheme = ControlSchemes.SpriteMappings[scheme]; | |
| GD.Print(scheme); | |
| leftKey.Texture = GD.Load<Texture2D>(selectedScheme["left"]); | |
| rightKey.Texture = GD.Load<Texture2D>(selectedScheme["right"]); | |
| upKey.Texture = GD.Load<Texture2D>(selectedScheme["up"]); | |
| downKey.Texture = GD.Load<Texture2D>(selectedScheme["down"]); | |
| } | |
| private void ChangeKeySprites(string scheme) | |
| { | |
| if (!ControlSchemes.SpriteMappings.TryGetValue(scheme, out var selectedScheme)) | |
| { | |
| GD.PrintErr($"Control scheme '{scheme}' not found"); | |
| return; | |
| } | |
| try | |
| { | |
| if (leftKey != null) leftKey.Texture = GD.Load<Texture2D>(selectedScheme["left"]); | |
| if (rightKey != null) rightKey.Texture = GD.Load<Texture2D>(selectedScheme["right"]); | |
| if (upKey != null) upKey.Texture = GD.Load<Texture2D>(selectedScheme["up"]); | |
| if (downKey != null) downKey.Texture = GD.Load<Texture2D>(selectedScheme["down"]); | |
| } | |
| catch (Exception e) | |
| { | |
| GD.PrintErr($"Failed to load textures for scheme '{scheme}': {e.Message}"); | |
| } | |
| } |
| private void ChangeKeySprites(string scheme) | ||
| { | ||
| var selectedScheme = ControlSchemes.SpriteMappings[scheme]; | ||
| GD.Print(scheme); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove debug print statement.
The GD.Print statement should be removed before merging to production.
- GD.Print(scheme);📝 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.
| GD.Print(scheme); |
| private midiNoteInfo[] _leftNotes; | ||
| private midiNoteInfo[] _rightNotes; | ||
|
|
||
| private MidiFile strippedSong; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused field.
The field strippedSong is declared but never used in the code.
- private MidiFile strippedSong;📝 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 MidiFile strippedSong; |
🧰 Tools
🪛 GitHub Check: build
[warning] 19-19:
The field 'MidiMaestro.strippedSong' is never used
[warning] 19-19:
The field 'MidiMaestro.strippedSong' is never used
| public MidiMaestro(string filePath) | ||
| { | ||
| if (!FileAccess.FileExists(filePath)) | ||
| { | ||
| GD.PrintErr("ERROR: Unable to load level Midi file: " + filePath); | ||
| } | ||
|
|
||
| _midiFile = MidiFile.Read(filePath); | ||
|
|
||
| //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; | ||
| } | ||
| } | ||
|
|
||
| //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
MIDI file loading and track processing looks solid.
The constructor correctly handles file existence checks, loads the MIDI file, and processes tracks based on named categories. The approach to map track names to arrow directions is intuitive and maintainable.
However, there's a hardcoded BPM value:
- //TODO: Allow for changes in this data
- Bpm = 120,
+ Bpm = _midiFile.GetTempoMap().GetTempoAtTime(0).BeatsPerMinute,Consider extracting the actual BPM from the MIDI file's tempo map instead of hardcoding it.
📝 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 MidiMaestro(string filePath) | |
| { | |
| if (!FileAccess.FileExists(filePath)) | |
| { | |
| GD.PrintErr("ERROR: Unable to load level Midi file: " + filePath); | |
| } | |
| _midiFile = MidiFile.Read(filePath); | |
| //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; | |
| } | |
| } | |
| //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, | |
| }; | |
| } | |
| public MidiMaestro(string filePath) | |
| { | |
| if (!FileAccess.FileExists(filePath)) | |
| { | |
| GD.PrintErr("ERROR: Unable to load level Midi file: " + filePath); | |
| } | |
| _midiFile = MidiFile.Read(filePath); | |
| //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; | |
| } | |
| } | |
| //Populate the song data | |
| songData = new SongData | |
| { | |
| Bpm = _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, | |
| }; | |
| } |
| public Room[] GetRooms() | ||
| { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
GetRooms() directly exposes the underlying array.
Returning the internal array can allow unwanted modifications. Consider returning a copy or read-only collection.
|
Closing this for now. Will pull soon. |
Summary by CodeRabbit
New Features
Enhancements