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

Significantly reduce number of IsMaskedAway calculations #1504

Merged
merged 2 commits into from Apr 10, 2018

Conversation

3 participants
@smoogipoo
Contributor

smoogipoo commented Apr 9, 2018

Reduces anywhere from 1.5-2.5K IsMaskedAway calculations, depending on the screen. Results in about 35-45 update fps boost (270/280 - 310/320) in gameplay (mania), about 30fps in SongSelect.

This would've previously been computed during drawnode creation only if we were present anyway (basically just before the IsMaskedAway check in addFromComposite).

@peppy peppy requested a review from Tom94 Apr 9, 2018

@peppy peppy added the performance label Apr 9, 2018

@peppy peppy added this to the April 2018 milestone Apr 9, 2018

@Tom94

This comment has been minimized.

Collaborator

Tom94 commented Apr 9, 2018

I have a feeling this may cause problems. UpdateSubTree is optimized based on IsMaskedAway, so non-present drawables may not get the updates they require to become present again if IsMaskedAway isn't updated correctly. Not 100% sure, though. May be an invalid point if UpdateSubTree is also optimized heavily via IsPresent.

@smoogipoo

This comment has been minimized.

Contributor

smoogipoo commented Apr 9, 2018

UpdateSubTree is indeed heavily optimised through IsPresent, and arguably much less optimised through IsMaskedAway.

@smoogipoo

This comment has been minimized.

Contributor

smoogipoo commented Apr 10, 2018

If you're still unsure about this, I'll also bring up again that this was essentially previous behaviour - IsMaskedAway previously only got computed in addFromComposite, following the IsPresent check in there.

@Tom94

Tom94 approved these changes Apr 10, 2018

@Tom94 Tom94 merged commit 7d33f46 into ppy:master Apr 10, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment