Skip to content

[BUGFIX] Chart Editor - Fix duplicating selection boxes (fix fastIndexOf implementation)#5073

Merged
AbnormalPoof merged 1 commit intoFunkinCrew:developfrom
NotHyper-474:bugfix/duplicate-displayed-notes
Jun 17, 2025
Merged

[BUGFIX] Chart Editor - Fix duplicating selection boxes (fix fastIndexOf implementation)#5073
AbnormalPoof merged 1 commit intoFunkinCrew:developfrom
NotHyper-474:bugfix/duplicate-displayed-notes

Conversation

@NotHyper-474
Copy link
Contributor

@NotHyper-474 NotHyper-474 commented May 12, 2025

Linked Issues

Closes #5064

Description

The chart editor uses an array, displayedNoteData to dictate whether a rendered note sprite should be created, kept or killed, which would be populated with duplicate notes or be missing notes, which was caused by fastIndexOf not working very well with notes of same time. This would cause multiple selection boxes and apparently multiple note sprites being unnecessarily created, seemingly causing a space leak.

To fix this I had to modify SongNoteDataArrayTools.fastIndexOf, it was either this or replacing the functions with contains. This changes the time complexity from O(log n) to a worst case of O(log n) + O(k + l) an extra O(k) and O(l) where k is the number of notes sharing the same time towards the left - and l towards the right - of midIndex.

The issue with the original implementation is that if notes are never equal but have the same time, highIndex will keep decreasing until the search stops and never finds the note, even if it actually exists in the array. So I've made it so it does extra checking towards both "directions" of the array while there's notes of equal time.
I've profiled the function via Tracy and saw no noticeable difference in performance, in fact it might even save some since rendered notes are now properly killed only when they need to.

Screenshots/Videos

8mb.video-hKB-PZkjhpz4.mp4

@github-actions github-actions bot added status: pending triage Awaiting review. pr: haxe PR modifies game code. size: medium A medium pull request with 100 or fewer changes. labels May 12, 2025
@AbnormalPoof AbnormalPoof added type: minor bug Involves a minor bug or issue. topic: chart editor Related to the operation of the Chart Editor. labels May 12, 2025
@Lasercar
Copy link
Contributor

Fixes #4277

@NotHyper-474
Copy link
Contributor Author

Fixes #4277

I cannot reproduce that one even on a 0.5.3 build so maybe not.

@NotHyper-474 NotHyper-474 force-pushed the bugfix/duplicate-displayed-notes branch from 65d2ae9 to 481ad86 Compare May 12, 2025 20:38
@EliteMasterEric EliteMasterEric added size: medium A medium pull request with 100 or fewer changes. and removed size: medium A medium pull request with 100 or fewer changes. labels May 14, 2025
@NotHyper-474 NotHyper-474 changed the title [BUGFIX] Chart Editor - Fix duplicating selection boxes [BUGFIX] Chart Editor - Fix duplicating selection boxes (fix fastIndexOf implementation) May 15, 2025
@NotHyper-474 NotHyper-474 force-pushed the bugfix/duplicate-displayed-notes branch from 481ad86 to 52bf3ae Compare May 15, 2025 20:46
@Lasercar
Copy link
Contributor

Fixes #4277

I cannot reproduce that one even on a 0.5.3 build so maybe not.

oh...

@Starexify
Copy link
Contributor

Starexify commented May 26, 2025

Aight I think it fixes all of my #5064 issues it seems, thanks 👍

@NotHyper-474 NotHyper-474 changed the base branch from develop to main May 28, 2025 13:53
@NotHyper-474 NotHyper-474 changed the base branch from main to develop May 28, 2025 13:54
@NotHyper-474 NotHyper-474 force-pushed the bugfix/duplicate-displayed-notes branch from 52bf3ae to 08f6a9c Compare May 30, 2025 12:03
@EliteMasterEric
Copy link
Member

O(log n) + O(k + l)

I'm pretty sure that's not how time complexity works, but it seems like it'll work fine regardless.

@NotHyper-474
Copy link
Contributor Author

I'm pretty sure that's not how time complexity works, but it seems like it'll work fine regardless.

You're most certainly right lol. But the point I wanted to get across is that it comes with a cost, even though it's probably negligible unless we're considering a chart completely filled with notes of equal time (or so I believe would be a worst case scenario)

@Hundrec Hundrec added status: reviewing internally Under consideration and testing. and removed status: pending triage Awaiting review. labels Jun 6, 2025
@EliteMasterEric
Copy link
Member

Note to self: reproduction of this issue requires moving the note to a row which has another note in a different column, it doesn't happen when there's a lone note.

@Hundrec Hundrec added status: accepted PR was approved for contribution. If it's not already merged, it may be merged on a private branch. and removed status: reviewing internally Under consideration and testing. labels Jun 9, 2025
@Hundrec Hundrec added this to the 0.7.0 milestone Jun 9, 2025
@AbnormalPoof AbnormalPoof merged commit 7947eca into FunkinCrew:develop Jun 17, 2025
6 checks passed
@NotHyper-474 NotHyper-474 deleted the bugfix/duplicate-displayed-notes branch June 17, 2025 00:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr: haxe PR modifies game code. size: medium A medium pull request with 100 or fewer changes. status: accepted PR was approved for contribution. If it's not already merged, it may be merged on a private branch. topic: chart editor Related to the operation of the Chart Editor. type: minor bug Involves a minor bug or issue.

Development

Successfully merging this pull request may close these issues.

Bug Report: [Crash Editor] Having Hold Note Strumlines in Chart Editor leaking RAM & selection box issue

6 participants