I agreed to review someone's code for free in exchange for being able to post about it publicly. So here we go.
Naming and Organization
3/5
While the names for the files and nodes don't follow the official GDScript style guidelines, they still appear to be somewhat consistent. I'm not sure what style convention is being used, but at least it's consistent. Most nodes are named with snake_case and the file names appear to be some kind of... Capital_Snake_Case? Well, the files are neatly organized in folders at least.
Code
1/5
This is a rhythm game where the notes move down the screen. As so, this is part of the code for the "Note" node.
func _process(delta):
self.position.y = self.position.y + speed * delta
This code can definitely be optimized. Using self to access a property should only be done when you have another variable with the same name as the property - and even then you should just name the variable something else to avoid confusion. Also, the += operator can be used to add a value to the property's pre-existing value.
So, better code would look like this:
func _process(delta):
position.y += speed * delta
Now, let's look at the main script.
func spawn(pos):
var note_instance = note.instance()
if pos == 1:
note_instance.set_position(Vector2(362, y_position_spawn))
self.add_child(note_instance)
elif pos == 2:
note_instance.set_position(Vector2(462, y_position_spawn))
self.add_child(note_instance)
elif pos == 3:
note_instance.set_position(Vector2(562, y_position_spawn))
self.add_child(note_instance)
elif pos == 4:
note_instance.set_position(Vector2(662, y_position_spawn))
self.add_child(note_instance)
Again, using self
here is completely unnecessary. It would also be better to store the numbers here at the top of the script so that they aren't magic numbers. I'm not sure why this wasn't done since y_position_spawn
is at the top of the script. :s Also, a match statement could be used here, but it's not necessary. add_child
is used in all cases of the if
statement, so it can be moved after the blocks.
Also, the code uses the accessor set_position()
to set the position, which isn't necessary since the position property can just be set directly with note_instance.position =
.
func note(playback, timing, pos):
var delay = 600 / 550
if playback >= timing - delay and playback <= timing + 0.05 - delay:
spawn(pos)
Again, delay
can be placed at the top. Also, since 600 and 550 are both int
s, this division will use integer division, which will result in 1 and not 1.09 repeating like expected. Make one or more of them a float, like 600.0 / 500.0
in order to use float division instead. Also, 0.05 is a magic number as well. Move it to the top.
To be honest, I've never made a rhythm game with Godot before, but from reading this documentation article, it's not a good idea to just hard-code a delay in like that. The person who requested the code review should read that article as well if they haven't already.
func _process(_delta):
var playback = $Music.get_playback_position()
note(playback, 2.03, 1)
note(playback, 4.06, 2)
note(playback, 6.07, 3)
note(playback, 8.10, 4)
note(playback, 8.60, 4)
note(playback, 9.20, 4)
note(playback, 9.7, 4)
note(playback, 10.1, 4)
note(playback, 10.5, 4)
note(playback, 10.9, 4)
note(playback, 11.3, 4)
note(playback, 11.7, 4)
note(playback, 12.1, 4)
Ouch. This can definitely be optimized. This code is checking whether to spawn each note every frame! That's unnecessary.
A better approach to this would to be to rethink how notes are stored in the game. Note that again, I've never made a rhythm game in Godot before. One way would to be to create a 2D Array with time values and notes and use an enum to store the position the note is spawned in. For example:
enum {LEFT, UP, DOWN, RIGHT}
var notes = [
[0.1, LEFT],
[0.1, RIGHT],
[0.2, UP],
[0.3, DOWN],
]
This is a lot more readable and easier to edit. All you need is a counter variable to indicate how far along you are in the array so you don't have to check the entire thing every frame.
Overall
2/5
It's pretty clear that the requester is new to Godot and coding. But it's not all bad - they know how to instance scenes, which isn't the easiest thing to do! I think with some effort they can improve their game and make it something worth playing.