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

Run stacking while performing movement in osu! composer #28792

Merged
merged 5 commits into from
Aug 5, 2024

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Jul 9, 2024

RFC. Closes #28635.

I was hoping that this was going to be simpler than it is, but it is not. See explanation in inline comments.

I'm not even considering performance implications or testing right now until I see whether the baseline fix survives review.

9cc0e01 is not required (it was a failed attempt to fix the biggest issue with this, which is the jittering mentioned in the inline comments), but felt nice so I included it.

bdach added 2 commits July 9, 2024 13:52
Previously it would be required to drag to the starting position of the
stack which feels weird.
@peppy
Copy link
Member

peppy commented Jul 10, 2024

It's a bit ugly, but as always it's very localised and should work until it doesn't. I'd say you can continue in the direction (and test performance).

bdach added a commit to bdach/osu that referenced this pull request Jul 10, 2024
… times is detected

Tangentially found when profiling ppy#28792.

For reproduction, import https://osu.ppy.sh/beatmapsets/972#osu/9007,
move any object on the playfield, and observe a half-second freeze
when ending the drag.
bdach added a commit to bdach/osu that referenced this pull request Jul 10, 2024
… times is detected

Tangentially found when profiling ppy#28792.

For reproduction, import https://osu.ppy.sh/beatmapsets/972#osu/9007,
move any object on the playfield, and observe a half-second freeze
when ending the drag.
@bdach
Copy link
Collaborator Author

bdach commented Jul 10, 2024

Performance is passable with #28801, but not great.

master:
image
image

this PR:
image
image

HandleMovement() itself is slightly slower but the increased UpdateState() calls do pile up for considerable overhead. Not sure what the move here is.

I guess one thing that would be suboptimal but fast would be to just fully unstack the object on drag start and re-stack it on drag end? That said, it would be nice to have this post-processing run somehow, because there are other issues like #24006 that cannot be worked around so easily.

@smoogipoo
Copy link
Contributor

Maybe you can expose a way for the osu! ruleset to run post-movement actions, and have it use OsuBeatmapProcessor.applyStacking() directly? Obviously it requires exposing further, but the implementation was adjusted a few years ago with that intention in mind (#4005).

@bdach
Copy link
Collaborator Author

bdach commented Jul 11, 2024

Post-movement is too late, re-stacking already happens on post-movement (mouse up). Something has to happen at the start, minimum, if not every frame during.

I have hopes that maybe if I just isolate the per-frame part to run stacking exclusively rather than do all of that ApplyDefaults() work performance will be more acceptable, but am yet to try it.

@smoogipoo
Copy link
Contributor

That's what I meant, but it was obtuse. Basically some sort of callback here whether a virtual method or otherwise, that the osu implementation can use to invoke that stacking method.

@pull-request-size pull-request-size bot added size/M and removed size/S labels Jul 11, 2024
@bdach
Copy link
Collaborator Author

bdach commented Jul 11, 2024

I beelined for the simplest possible variant of that in 37a296b, and it seems to be... acceptable? Not sure about code quality, but yeah.

method master this PR before 37a296b this PR after 37a296b
UpdateState() 1720697656 1720697689 1720697751
HandleMovement() 1720697629 1720697667 1720697704

Mind you, I did try to do a not-full-beatmap pass of the stacking update, but getting the correct indices is not free or even trivial to do. You would think that it would be enough to get the indices of the first and last object in the selection, but that's not enough - you would really need indices of all objects that have alive blueprints that can be snapped to, and fishing that out at the level of OsuSelectionHandler is... not trivial. So maybe this is good enough for how simple it is?

@peppy peppy requested a review from smoogipoo August 5, 2024 06:44
@smoogipoo smoogipoo merged commit fb8f80f into ppy:master Aug 5, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Moving stacked hitobject ends up in the wrong coordinates
3 participants