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

Avoid drawing unnecessary things when we're about to quit #2653

Closed
wants to merge 2 commits into from

Conversation

tobbi
Copy link
Member

@tobbi tobbi commented Oct 2, 2023

Also, don't update integrations for every tick, that's way too often and wastes CPU cycles.

@Vankata453
Copy link
Member

Is there a case when ScreenManager isn't running, other than when the game is being quit?

@tobbi
Copy link
Member Author

tobbi commented Oct 2, 2023

Is there a case when ScreenManager isn't running, other than when the game is being quit?

Not that I know of.

@Vankata453
Copy link
Member

Additionally, while the game quit screen fade is being performed, wouldn't not further drawing the UI elements make them disappear immediately before the fade starts, causing for a visual inconsistency? I will test this soon to check, this is just an observation from looking at the code.

@Vankata453
Copy link
Member

Testing shows that now quitting the game with transitions enabled makes the UI disappear before the fade out happens. Considering this didn't happen before the change, and keeping in mind ScreenManager is only not running (shouldn't draw UI elements, process sound or events) when quitting the game, I'm not sure whether the changes (other than the integration update ones) provide a significant benefit.

@mrkubax10 mrkubax10 requested a review from Vankata453 October 3, 2023 19:37
@mrkubax10 mrkubax10 added involves:performance category:code status:needs-review Work needs to be reviewed by other people labels Oct 3, 2023
@Vankata453
Copy link
Member

What is the optimization achieved in this PR? Is it significant enough to warrant not drawing UI elements on screen fade-out?

@tobbi
Copy link
Member Author

tobbi commented Oct 3, 2023

It's drawing them now

@Vankata453
Copy link
Member

It's drawing them now

It's only drawing the menu on fade-out, still hides FPS counter, player position, etc... There should be some benefit from not drawing those UI items to warrant changing their behaviour, and I'm not sure what it is.

@tobbi
Copy link
Member Author

tobbi commented Oct 3, 2023

Fine, you want the pr closed, I'll close the pr.

@tobbi tobbi closed this Oct 3, 2023
@Vankata453
Copy link
Member

Vankata453 commented Oct 3, 2023

Sorry, I only wanted to ask a question regarding the PR, so I can justify a review. For example, the integration change seems beneficial. For this part I genuinely didn't know what benefit it serves, so I'm asking to hear why you decided to implement it, possibly I could be missing something.

@tobbi
Copy link
Member Author

tobbi commented Oct 3, 2023

Reason I implemented this was because shutdown always felt slow to me

@Vankata453
Copy link
Member

Did it feel slow when the menu was hidden?

@mrkubax10 mrkubax10 removed the status:needs-review Work needs to be reviewed by other people label Oct 7, 2023
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.

3 participants