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 ints, 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.

@exuin said: Overall 2/5

I'm not sure that I've ever encountered a code review with a rating attached, and I'm honestly not sure at what level the joke is swooshing over my head :joy: . Language is weird -- "review" as "a critical evaluation" (an artifact itself) vs "to give a critical evaluation of" (the process, with no actual artifact of the review necessarily emerging).

A few comments on the comments:

So, better code would look like this:

func _process(delta):
    position.y += speed * delta

More broadly though, why isn't motion using the physics system? I can see some potential for additional issues when needing to juggle how frame time, physics time, and audio time all line up, and I haven't done much with the physics system myself, but naively it seems that managing continuous movement is one of the things it it designed for. Moving that out of per-frame script evaluation and into the core engine seems like a potentially big win.

A better approach to this would to be to rethink how notes are stored in the game.

I agree, but I think it would be possible to take this idea even further than you propose, and use Godot editor facilities to allow visual editing of note patterns, even without adding anything new. For example, it looks like this game always uses a fixed y position for where notes spawn. So one could create a scene where the y axis is time, and includes an editor-only (or hiden/removed at game launch) visual representation of the audio track running vertically along one side. Then one could play this scene by either first transforming it into an array of data in some form as you propose, or by even literally loading the scene and giving the entire thing motion!