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

Fix stack leniency not applying immediately after change #28441

Merged

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Jun 10, 2024

RFC. Closes #20675.

I don't love this diff and I'm a bit scared of this change, but the last time this was attempted to be fixed the end result was #25467, so in comparison this looks like it may have a chance of maybe passing review.

Will add some self-review comments in a sec because this is a bit counterintuitive and I wasn't sure how much of this was deserving comments (provided this diff survives the initial eye test even). Can also try and add tests for this if this is deemed a viable route forward.

@@ -105,7 +105,7 @@ public EditorBeatmap(IBeatmap playableBeatmap, ISkin beatmapSkin = null, Beatmap
BeatmapSkin.BeatmapSkinChanged += SaveState;
}

beatmapProcessor = playableBeatmap.BeatmapInfo.Ruleset.CreateInstance().CreateBeatmapProcessor(PlayableBeatmap);
beatmapProcessor = playableBeatmap.BeatmapInfo.Ruleset.CreateInstance().CreateBeatmapProcessor(this);
Copy link
Collaborator Author

@bdach bdach Jun 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the primary fix and it works by addressing the thing mentioned in #20675 (comment), namely reading from the correct BeatmapInfo instance.

I audited both beatmap processors of ours that actually do stuff (osu! and catch), and all of the properties they touch aside from BeatmapInfo are proxied in EditorBeatmap forward to PlayableBeatmap, so from that angle this seems safe-ish.

@@ -49,6 +49,7 @@ protected override void LoadComplete()
private void updateBeatmap()
{
Beatmap.BeatmapInfo.StackLeniency = stackLeniency.Current.Value;
Beatmap.UpdateAllHitObjects();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bypasses the following guard:

if (batchPendingUpdates.Count == 0 && batchPendingDeletes.Count == 0 && batchPendingInserts.Count == 0)
return;

Compare:

private void updateValues()
{
// for now, update these on commit rather than making BeatmapMetadata bindables.
// after switching database engines we can reconsider if switching to bindables is a good direction.
Beatmap.Difficulty.CircleSize = CircleSizeSlider.Current.Value;
Beatmap.Difficulty.DrainRate = HealthDrainSlider.Current.Value;
Beatmap.Difficulty.ApproachRate = ApproachRateSlider.Current.Value;
Beatmap.Difficulty.OverallDifficulty = OverallDifficultySlider.Current.Value;
Beatmap.Difficulty.SliderMultiplier = BaseVelocitySlider.Current.Value;
Beatmap.Difficulty.SliderTickRate = TickRateSlider.Current.Value;
Beatmap.UpdateAllHitObjects();
Beatmap.SaveState();
}

@@ -41,47 +42,47 @@ public override void PostProcess()
{
base.PostProcess();

var osuBeatmap = (Beatmap<OsuHitObject>)Beatmap;
Copy link
Collaborator Author

@bdach bdach Jun 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hard cast needs to be gone for the EditorBeatmap change to have a chance of working, because EditorBeatmap is not a Beatmap<OsuHitObject>. The catch processor strangely does not do this sort of casting and does nice proper soft casts instead.

As an "optimisation" (reducing iterator and list allocations) I added the soft-cast of Beatmap.HitObjects to List<OsuHitObject>.

Copy link
Contributor

@smoogipoo smoogipoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine

@smoogipoo smoogipoo merged commit 39b219b into ppy:master Jun 12, 2024
11 of 17 checks passed
@bdach bdach deleted the fix-editor-stacking-not-applying-immediately branch June 12, 2024 06:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stack leniency doesn't affect objects immediately in Editor
2 participants