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

Extract Game::drawScene from Game::updateFrame #13967

Merged
merged 5 commits into from Dec 17, 2023

Conversation

JosiahWI
Copy link
Contributor

@JosiahWI JosiahWI commented Nov 8, 2023

There were big banner comments setting apart this section of code, so it was not hard to see it should be its own method. Besides the extraction, there are only two code changes:

  • this-> was added to member accesses in the extracted method.
  • The player object was retrieved again at the top of the extracted method so it wouldn't have to be passed in.

To do

Ready for Review.

How to test

There should not any logical change. I confirmed in-game that things like the wield tool, crosshairs, and HUD are still there.

There were big banner comments setting apart this section of code, so
it was not hard to see it should be its own method. Besides the extraction,
there are only two code changes:
  - this-> was added to member accesses in the extracted method.
  - The player object was retrieved again at the top of the extracted
    method so it wouldn't have to be passed in.
@Zughy Zughy added Code quality @ Engine Core What happens inside the very engine labels Nov 8, 2023
src/client/game.cpp Outdated Show resolved Hide resolved
src/client/game.cpp Outdated Show resolved Hide resolved
src/client/game.cpp Outdated Show resolved Hide resolved
src/client/game.cpp Outdated Show resolved Hide resolved
@grorp grorp added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Dec 15, 2023
JosiahWI and others added 4 commits December 15, 2023 08:17
Co-authored-by: grorp <82708541+grorp@users.noreply.github.com>
Co-authored-by: grorp <82708541+grorp@users.noreply.github.com>
Co-authored-by: grorp <82708541+grorp@users.noreply.github.com>
This keeps the timing related code out of drawScene.
Copy link
Member

@grorp grorp left a comment

Choose a reason for hiding this comment

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

LGTM
Tested briefly, found nothing broken.

Whether adding this is a good idea or not can be decided by whoever gives this PR the second review.

@grorp grorp added One approval ✅ ◻️ and removed Action / change needed Code still needs changes (PR) / more information requested (Issues) labels Dec 15, 2023
@sfan5 sfan5 merged commit 7162b53 into minetest:master Dec 17, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants