Remove rounding of slider velocity (when applied as scroll speed) #26616
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
This affects both osu!taiko and osu!mania. Closes ppy#25862.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
Note that I'm not PRing the |
|
Diffcalc runs failed again because the sheet generator is dependent on the old structure of the I'm not fixing this on github before smoogi comes back, so I guess I'll have to run any and all sheets locally. |
|
Manual taiko spreadsheet: https://docs.google.com/spreadsheets/d/1QXLt1PDzt2uIEpYi9GVLekaNgjkqWBFU2hReRD44CvI/edit#gid=825868992 Looks promising, as long as I didn't screw up the score table structure updates... (smoogipoo/diffcalc-sheet-generator#1) |
|
@bdach I've pushed a WIP fix for the slider bar. See what you think of it. It works fine, except the textbox shows too much precision due to inaccuracies coming from osu/osu.Game/Graphics/UserInterfaceV2/SliderWithTextBoxInput.cs Lines 113 to 114 in 0bacf69 |
peppy
added
the
next release
|
I continue to think that messing with the control is the wrong approach. I would propose this instead of the last commit: diff --git a/osu.Game/Screens/Edit/Timing/EffectSection.cs b/osu.Game/Screens/Edit/Timing/EffectSection.cs
index 7e484433f7..9d4ef5c62a 100644
--- a/osu.Game/Screens/Edit/Timing/EffectSection.cs
+++ b/osu.Game/Screens/Edit/Timing/EffectSection.cs
@@ -52,17 +52,44 @@ void saveChanges()
protected override void OnControlPointChanged(ValueChangedEvent<EffectControlPoint?> point)
{
- if (point.NewValue != null)
+ if (point.OldValue is EffectControlPoint oldEffectPoint)
+ scrollSpeedSlider.Current.ValueChanged -= updateControlPointFromSlider;
+
+ if (point.NewValue is EffectControlPoint newEffectPoint)
{
isRebinding = true;
- kiai.Current = point.NewValue.KiaiModeBindable;
- scrollSpeedSlider.Current = point.NewValue.ScrollSpeedBindable;
+ kiai.Current = newEffectPoint.KiaiModeBindable;
+ scrollSpeedSlider.Current = new BindableDouble
+ {
+ MinValue = 0.01,
+ MaxValue = 10,
+ Precision = 0.01,
+ Value = newEffectPoint.ScrollSpeedBindable.Value
+ };
+ scrollSpeedSlider.Current.ValueChanged += updateControlPointFromSlider;
isRebinding = false;
}
}
+ private void updateControlPointFromSlider(ValueChangedEvent<double> scrollSpeed)
+ {
+ if (ControlPoint.Value is not EffectControlPoint effectPoint || isRebinding)
+ return;
+
+ effectPoint.ScrollSpeedBindable.Value = scrollSpeed.NewValue;
+ }
+
protected override EffectControlPoint CreatePoint()
{
var reference = Beatmap.ControlPointInfo.EffectPointAt(SelectedGroup.Value.Time);
It appears to work...? And yes it probably doesn't support undo well but nothing really does right now. We can probably leave a todo for that (and even then it shouldn't be difficult to make work, just do some careful propagation in the converse direction). |
|
That looks far better than what I had if it works 👍 . |
|
@bdach wanna apply your patch? I can't seem to make it apply 😓 . |
|
Have applied with slight alterations. Turns out the simple unidirectional solution is even enough to support undo for now (albeit, only due to some special circumstances, as per attached inline comment). |
bdach
approved these changes
Jan 29, 2024
bdach
added a commit
to bdach/osu
that referenced
this pull request
May 28, 2024
Compare: ppy#26616 This came up elsewhere, namely in ppy#28277 (comment). As it turns out, at least one beatmap among those whose scores had unexpected changes in total score, namely https://osu.ppy.sh/beatmapsets/971028#fruits/2062131, was using slider velocity multipliers that were not a multiple of 0.01 (the specific value used was 0.225x). This meant that due to the rounding applied to `SliderVelocityMultiplierBindable` via `Precision`, the raw value was being incorrectly rounded, resulting in incorrect conversion. The "direct" change that revealed this is most likely ppy/osu-framework#6249, by the virtue of shuffling the `BindableNumber` rounding code around and accidentally changing midpoint rounding semantics in the process. But it was not at fault here, as rounding was just as wrong before that change as after in this specific context.
1 task
TextAdventurer12
pushed a commit
to TextAdventurer12/osu
that referenced
this pull request
Jul 6, 2024
Compare: ppy#26616 This came up elsewhere, namely in ppy#28277 (comment). As it turns out, at least one beatmap among those whose scores had unexpected changes in total score, namely https://osu.ppy.sh/beatmapsets/971028#fruits/2062131, was using slider velocity multipliers that were not a multiple of 0.01 (the specific value used was 0.225x). This meant that due to the rounding applied to `SliderVelocityMultiplierBindable` via `Precision`, the raw value was being incorrectly rounded, resulting in incorrect conversion. The "direct" change that revealed this is most likely ppy/osu-framework#6249, by the virtue of shuffling the `BindableNumber` rounding code around and accidentally changing midpoint rounding semantics in the process. But it was not at fault here, as rounding was just as wrong before that change as after in this specific context.
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.
Was of course only done for the benefit of editor UI controls.
This affects both osu!taiko and osu!mania.
Closes #25862.
TODO: