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 WaveformGraph overhead when DrawPosition is changed #6009

Merged
merged 2 commits into from Oct 2, 2023

Conversation

peppy
Copy link
Sponsor Member

@peppy peppy commented Sep 29, 2023

The previous attempt to fix this (#5197) helped, but there were still some scenarios where invalidation would cause regeneration.

Let's just do our own invalidation for now.

One thing I'm not sure about is whether checking DrawSize here is enough (should I be using ScreenSpaceDrawQuad.Size instead?).

Closes ppy/osu#24951

Before After
osu! 2023-09-29 at 10 03 06 osu! 2023-09-29 at 10 02 11

The previous attempt to fix this
(ppy#5197) helped, but there
were still some scenarios where invalidation would cause
regeneration.

Let's just do our own invalidation for now.

One thing I'm not sure about is whether checking `DrawSize` here is
enough (should I be using `ScreenSpaceDrawQuad.Size` instead?).
smoogipoo
smoogipoo previously approved these changes Oct 2, 2023
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.

The screenshots don't really make much sense as an observer to the issue, but I think it's fine.

Comment on lines 167 to 168
// We should regenerate when `Scale` changed, but not `Position`.
// Unfortunately both of these are grouped together in `MiscGeometry`.
// Can't use invalidation for this as RequiredParentSizeToFit is closes, but also triggers on DrawPosition changes.
if (lastGeneratedDrawSize != null && DrawSize != lastGeneratedDrawSize)
queueRegeneration();
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment previously mentioned scale, but that's not included in DrawSize. Perhaps it makes more sense this way, though? That is - when we think of scale it's usually in the context of stretching data and not generating new data for the stretched bounds?

Copy link
Collaborator

Choose a reason for hiding this comment

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

RequiredParentSizeToFit = MiscGeometry | DrawSize. MiscGeometry invalidates on Scale, so the comment made sense. I don't necessarily disagree with ignoring it though as you say.

@smoogipoo smoogipoo requested a review from bdach October 2, 2023 01:37
@peppy
Copy link
Sponsor Member Author

peppy commented Oct 2, 2023

The screenshots don't really make much sense as an observer to the issue, but I think it's fine.

Yeah it's subtle. See the spikes on the update thread, which occur every beat in the editor.

@smoogipoo
Copy link
Contributor

smoogipoo commented Oct 2, 2023

As a side note, TestSceneWaveform is absolute crap... What even is that "zoom" slider?

Can there maybe be a TestSceneWaveformGraph that demonstrates some of these cases - moving it around, changing resolution, scaling, resizing, along with a counter for number of regens for each? There's already a OnWaveformRegenerated virtual method as part of WaveformGraph, so all I'm imagining is incrementing a counter similar to TestSceneCachedBufferedContainer from there.

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.

Let's go with this for now

@bdach
Copy link
Collaborator

bdach commented Oct 2, 2023

Can there maybe be a TestSceneWaveformGraph that demonstrates some of these cases - moving it around, changing resolution, scaling, resizing, along with a counter for number of regens for each? There's already a OnWaveformRegenerated virtual method as part of WaveformGraph, so all I'm imagining is incrementing a counter similar to TestSceneCachedBufferedContainer from there.

To speed things up I'm just gonna leave that for the next time we touch that I think. Or a separate PR.

@bdach bdach merged commit 60fa441 into ppy:master Oct 2, 2023
17 of 21 checks passed
@peppy peppy deleted the fix-waveform-regeneration-on-draw-position branch October 12, 2023 04:40
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.

Waveform display in editor with long songs causes lag spikes
3 participants