BPM event refactor #579
Merged
BPM event refactor #579
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* songBpmTime is now nullable. If time conversion fails, it is set to null. Casting it to float when it's null will raise an exception. This is intentional; it means there is a serious bug and we should know about it. * SongBpmTime cannot be set anymore. This was only used in one place, and you should set JsonTime instead. * BPM time conversion logic is moved to from BPMChangeGridContainer to BaseDifficulty. BPM events must be bootstrapped after loading for this to function. All object songBpmTimes should be recomputed after doing this. * SetTimes in BaseObject has been simplified since songBpmTime was getting recalculated anyway.
* When saving BPM regions, properly round calculated sample indexes, and ensure continuity by setting start sample index to previous end sample index. * When converting BPM regions into events, round the BPM value of resulting events if the unrounding was (likely) caused by numerical error. Together these should (mostly) eliminate accumulating errors on load/save cycles, which is important for v4 maps, since they do not store the actual BPM events.
* FindLastBpmEvent should have been exclusive, not inclusive. This is because BPM events use this function to update their own songBpmTime, and if it's inclusive they will recalculate based on themselves. * BPM events need to be refreshed before any other objects, since the result of their refresh is used in the refresh of others. * ATSC time update on BPM tweak should occur after objects have been refreshed.
turns out inclusive search WAS useful for some things
Caeden117
approved these changes
Mar 26, 2025
Assets/__Scripts/MapEditor/Grid/Collections/BeatmapObjectContainerCollection.cs
Outdated
Show resolved
Hide resolved
XAce1337manX
requested changes
Mar 28, 2025
creating an object with this constructor would have left songBpmTime as null
|
tests should be passing now |
XAce1337manX
approved these changes
Mar 30, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
BPM event refactor:
This fixes wrong saving of BPM info files (BPMInfo.dat/AudioData.dat) when saving diffs from the song edit screen. It should also make it impossible for such a bug to go unnoticed in the future.
I haven't found any new bugs stemming from this refactor, but more testing is of course welcome.
mitigate accumulating numerical errors on BPM region load/save:
Together these should (mostly) eliminate accumulating errors on load/save cycles, which is important for v4 maps, since they do not store the actual BPM events.