raphaelgoulart / ya_inputdisplay

Yet another input display for Clone Hero / YARG
16 stars 3 forks source link

Fix Singleton incorrectly removing inputs from inputs_gone #12

Closed SkyJumper409x closed 3 months ago

SkyJumper409x commented 3 months ago

I noticed this issue as index out of bounds errors that sometimes appeared in the console while running a release. I found a fix for this just looking at the code, so, because this error is only really noticeable if the application is run via the console, I didn't open an issue for it and just fixed it right away.

The issue was in the _process(delta) method of Singleton.

From my understanding, this is what the code should do: All indexes of elements to remove from inputs_gone are added to inputs_to_remove In the first for loop (lines 65- 69) and then, in the second for loop, inputs_gone.remove_at(x) is called for all indexes in inputs_to_remove (lines 70 & 71).

The problem was that the indexes were removed in ascending order, which moves all other indexes. Simply changing line 69 to inputs_to_remove.insert(0, i) makes the indexes be removed in descending order, which ensures that the indexes don't move around.

This was pretty tough to test, as it requires two inputs to be removed in the same processing frame. I tested by running both the old and the new _process(delta) method of Singleton.gd, and having each of them log if they got an error. I could only get the error on the old version, the new method didn't output any errors even if the old version did. I performed an input on every fret to have a good chance of at least 2 inputs being so close that they happen on the same processing frame.

(tldr i fixed a bug thats hard to notice normally but "bugged" me)

raphaelgoulart commented 3 months ago

Looks good, appreciate the explanation too! Will just do some basic testing once I'm home (at work rn) then merge

SkyJumper409x commented 3 months ago

okay :3 thkuuu