Fix slider tail volume not saving #28619
Merged
Conversation
This file contains 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
Closes ppy#28587. As outlined in the issue thread, the tail volume wasn't saving because it wasn't actually attached to a hitobject properly, and as such the `LegacyBeatmapEncoder` logic, which is based on hitobjects, did not pick them up on save. To fix that, switch to using `NodeSamples` for objects that are `IHasRepeats`. That has one added complication in that having it work properly requires changes to the decode side too. That is because the intent is to allow the user to change the sample settings for each node (which are specified via `NodeSamples`), as well as "the rest of the object", which generally means ticks or auxiliary samples like `sliderslide` (which are specified by `Samples`). However, up until now, `Samples` always queried the control point which was _active at the end time of the slider_. This obviously can't work anymore when converting `NodeSamples` to legacy control points, because the last node's sample is _also_ at the end time of the slider. To bypass that, add extra sample points after each node (just out of reach of the 5ms leniency), which are supposed to control volume of ticks and/or slides. Upon testing, this *sort of* has the intended effect in stable, with the exception of `sliderslide`, which seems to either respect or _not_ respect the relevant volume spec dependent on... not sure what, and not sure I want to be debugging that. It might be frame alignment, or it might be the phase of the moon.
smoogipoo
reviewed
Jun 28, 2024
smoogipoo
reviewed
Jul 1, 2024
bdach
commented
Jul 1, 2024
Comment on lines
296
to
297
| if (spanDuration > LegacyBeatmapDecoder.CONTROL_POINT_LENIENCY + 1 && hitObject.Samples.Count > 0) | ||
| yield return createSampleControlPointFor(nodeTime + LegacyBeatmapDecoder.CONTROL_POINT_LENIENCY + 1, hitObject.Samples); |
There was a problem hiding this comment.
Realised that this is probably slightly bugged, this control point shouldn't be output for the last node (which is presumed to be the end of the object). Will fix whenever able.
smoogipoo
approved these changes
Jul 3, 2024
|
I think this makes sense to me. |
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.
RFC. Closes #28587.
As outlined in the issue thread, the tail volume wasn't saving because it wasn't actually attached to a hitobject properly, and as such the
LegacyBeatmapEncoderlogic, which is based on hitobjects, did not pick them up on save.To fix that, switch to using
NodeSamplesfor objects that areIHasRepeats. That has one added complication in that having it work properly requires changes to the decode side too. That is because the intent is to allow the user to change the sample settings for each node (which are specified viaNodeSamples), as well as "the rest of the object", which generally means ticks or auxiliary samples likesliderslide(which are specified bySamples).However, up until now,
Samplesalways queried the control point which was active at the end time of the slider. This obviously can't work anymore when convertingNodeSamplesto legacy control points, because the last node's sample is also at the end time of the slider. To bypass that, add extra sample points after each node (just out of reach of the 5ms leniency), which are supposed to control volume of ticks and/or slides, and read those instead on decode intoSampleswhen parsing anIHasRepeats. See following crude visual explanation:Upon testing, this sort of has the intended effect in stable, with the exception of
sliderslide, which seems to either respect or not respect the relevant volume spec dependent on... not sure what, and not sure I want to be debugging that. It might be frame alignment, or it might be the phase of the moon.cc @OliBomby (not sure if you want to voice your opinion on this)