Fix WaveformGraph regenerating on position changes
#5197
Conversation
| result = true; | ||
| // We should regenerate when `Scale` changed, but not `Position`. | ||
| // Unfortunately both of these are grouped together in `MiscGeometry`. | ||
| if (requiredPointCount != resampledPointCount) |
There was a problem hiding this comment.
Hmm, this (well, the Scale/DrawWidth referencing) is technically an invalid operation to be done in Invalidate(), because it could behave badly with relative/autosizing. Could this check be moved to the schedule instead, or is that prohibitively difficult to do?
There was a problem hiding this comment.
I had things structured differently to avoid this then double-took for a reason I can't remember. Please check the new additions. Also please confirm AddOnce is correctly used here (seems to be, but I can never be 100%).
There was a problem hiding this comment.
I'm not sure what was changed here, doesn't look like anything was? Missing a commit?
There was a problem hiding this comment.
No, I just re-refactored to the point it didn't solve the issue you pointed out. Will revisit.
|
Test failures need attention here. |
|
Applied two more fixes, the first is a race condition that I couldn't leave hanging (was present in the previous code too), the second prevented the waveform from being regenerated when there's a feedback loop causing multiple invalidations in successive frames. |
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?).
This is a bare minimum fix for waveforms being regenerated completely when their
Positionis changed.There's plenty more work which needs to be done to improve performance during resampling / generation, but this alone allows me to move forward with my more intensive usage of this class for now.