Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compute map length starting from the first note #130

Closed
wants to merge 3 commits into from

Conversation

IceDynamix
Copy link
Contributor

Both common BPM and map length computation assume the map starts at 0ms. This is changed so both compute starting from the first hit object instead. This indirectly fixes the NPS shown in-game as well.

@YaLTeR
Copy link
Member

YaLTeR commented Aug 1, 2022

The CommonBPM computation is done the exact same way as in osu! because if it's wrong then the SVs break (they are based on CommonBPM). I'm pretty sure the current way matches osu!? If not please attach a map that is broken and add a test.

@IceDynamix IceDynamix changed the title Fix common bpm and map length for maps with a lot of empty sections Fix common bpm and map length for maps with a lot of empty intro time Aug 1, 2022
@IceDynamix
Copy link
Contributor Author

IceDynamix commented Aug 1, 2022

One use case that initiated fixing common BPM was an album map with individual difficulties for each song that had empty intro time because they still use the same mp3, so the common BPM was displayed incorrectly in-game. I feel like this is a sensible change that should not affect anything made in Quaver because of the BPM invariant scroll setting, so only maps created in osu! are affected.

One way to fix this is to compute the common BPM the old way in the osu! class and adjust the SVs values during Qua construction.

@IceDynamix
Copy link
Contributor Author

Another way would be to keep the old computation and add a new one used only for display purposes, which the game calls from. It's the easiest solution, but feels like a roundabout way of addressing this issue though.

@Swan
Copy link
Member

Swan commented Aug 1, 2022

If the whole purpose of this is to fix an incorrect display, then we should be worrying about how to improve the UI instead. The game should instead display the different BPMs in the map as well as the most common one imo.

@IceDynamix
Copy link
Contributor Author

The common BPM still needs to be computed somehow, and an incorrect computation is something UI can't fix.

Displaying it like osu! does would look something like 120-220 (180). Etterna does this with a single number that changes value, transitioning from min to max to min. Another idea would be to provide a tooltip over the difficulty plot in gameplay preview, that shows the current BPM at the location of the mouse cursor in the plot. This tooltip could also show other things at that location, like timestamp, difficulty, NPS or LN%.

@IceDynamix
Copy link
Contributor Author

I'll revert the common BPM commit and create a new issue instead

@IceDynamix IceDynamix changed the title Fix common bpm and map length for maps with a lot of empty intro time Compute map length starting from the first note Aug 1, 2022
@Swan Swan closed this Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants