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 performance degradation while trying to place object with a new combo in the editor. #31446

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

EVAST9919
Copy link
Contributor

Closes #31428.
HitObjectPlacementBlueprint.UpdateTimeAndPosition() calls IHasComboInformation.UpdateComboInformation() in which bindables can be reassigned multiple times with the end result being the same, so triggering bound actions can be avoided.

master:
https://streamable.com/oa6ktq
pr:
https://streamable.com/bpm3ux

@bdach
Copy link
Collaborator

bdach commented Jan 7, 2025

Next time please provide actual profiling results. Frame graph is not profiling.

As far as I can tell this is at most halving the number of assignments to the various bindables, which alone wouldn't fix the issue. What is probably fixing the issue is that reducing this code to a single assignment at the end of the method is reducing change callbacks firing because otherwise the bindable thrashes between intermediate values and the final complete values. If only final complete values are assigned to the bindables, the "is the bindable value already equal to this" checks framework-side early-return and thus change callbacks aren't fired.

Is this a correct interpretation of this diff? Seems fine if so, but that is very not obvious to me, and all of this information should be spelled out in the OP.

Copy link
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

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

probably ok pending confirmation of above

@EVAST9919
Copy link
Contributor Author

Next time please provide actual profiling results. Frame graph is not profiling.

This pr is only possible due to profiling (results of which could've been included in the OP, yes)
Recorded while doing the same actions on the video and shows a big difference on my end.

master pr
master Снимок экрана 2025-01-07 201447

reducing this code to a single assignment at the end of the method is reducing change callbacks firing because otherwise the bindable thrashes between intermediate values and the final complete values. If only final complete values are assigned to the bindables, the "is the bindable value already equal to this" checks framework-side early-return and thus change callbacks aren't fired.

"bindables can be reassigned multiple times with the end result being the same, so triggering bound actions can be avoided" in the OP means the same thing imo.

@peppy peppy requested review from peppy and removed request for peppy January 8, 2025 05:06
@bdach
Copy link
Collaborator

bdach commented Jan 8, 2025

This pr is only possible due to profiling (results of which could've been included in the OP, yes)

Should have been. All performance optimisations should come with profiling results out of the gate. Especially if you already had the profiling results and was working off of them.

"bindables can be reassigned multiple times with the end result being the same, so triggering bound actions can be avoided" in the OP means the same thing imo.

I dunno I didn't read it that way.

I don't say this sort of thing to be a hardass and give you a difficult time, I say it because the lack of profiling results and a detailed explanation of the optimisation costs me review time. It's half an hour spent yesterday trying to understand what this does that I'm not getting back, and that is my only concern.

Comment on lines +166 to +171
// For the purpose of combo colours, spinners never start a new combo even if they are flagged as doing so.
if (this is not BananaShower)
{
// For the purpose of combo colours, spinners never start a new combo even if they are flagged as doing so.
return;
// At decode time, the first hitobject in the beatmap and the first hitobject after a banana shower are both enforced to be a new combo,
// but this isn't directly enforced by the editor so the extra checks against the last hitobject are duplicated here.
if (NewCombo || lastObj == null || lastObj is BananaShower)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@peppy mentioned in passing on stream that he'd consider coalescing these two conditionals into a single one because the nesting makes it hard to read otherwise. I'm neither here or there on that, I could live with this version, but at least maybe it's a good thing to at least propose it.

@peppy
Copy link
Member

peppy commented Jan 9, 2025

Just a tip when including profiling screenshots: make sure the original method is visible in the new screenshot so the relative percentage usage can be compared to other methods in the same parent method.

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.

Moving cursor in editor with slider tool + new combo selected causes huge performance drops
3 participants