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 updateClouds method from updateFrame #13939

Merged
merged 4 commits into from Nov 8, 2023

Conversation

JosiahWI
Copy link
Contributor

This is a code cleanup. Other than adding this-> to member variables in the extracted method, no changes were made to the extracted code.

To do

Ready for Review.

How to test

I compiled a Debug build and confirmed in devtest that the clouds move and have the same appearance they did before.

@grorp grorp added Code quality @ Engine Core What happens inside the very engine labels Oct 28, 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
@grorp grorp added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Nov 4, 2023
@JosiahWI
Copy link
Contributor Author

JosiahWI commented Nov 4, 2023

An automated formatter would help with these things. :)

JosiahWI and others added 2 commits November 4, 2023 09:58
Co-authored-by: Gregor Parzefall <82708541+grorp@users.noreply.github.com>
@JosiahWI JosiahWI requested a review from grorp November 5, 2023 00:25
@Zughy Zughy removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Nov 5, 2023
Copy link
Member

@srifqi srifqi left a comment

Choose a reason for hiding this comment

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

When changing these lines/functions, it might be better to also improve the comments (in one go).

diff --git a/src/client/game.cpp b/src/client/game.cpp
index d6a5a5bb6..cc8891547 100644
--- a/src/client/game.cpp
+++ b/src/client/game.cpp
@@ -4198,7 +4198,7 @@ void Game::updateClouds(float dtime)
 	if (this->sky->getCloudsVisible()) {
 		this->clouds->setVisible(true);
 		this->clouds->step(dtime);
-		// this->camera->getPosition is not enough for 3rd person views
+		// this->camera->getPosition is not enough for third-person camera.
 		v3f camera_node_position = this->camera->getCameraNode()->getPosition();
 		v3s16 camera_offset      = this->camera->getOffset();
 		camera_node_position.X   = camera_node_position.X + camera_offset.X * BS;
@@ -4206,14 +4206,13 @@ void Game::updateClouds(float dtime)
 		camera_node_position.Z   = camera_node_position.Z + camera_offset.Z * BS;
 		this->clouds->update(camera_node_position, this->sky->getCloudColor());
 		if (this->clouds->isCameraInsideCloud() && this->m_cache_enable_fog) {
-			// if inside clouds, and fog enabled, use that as sky
-			// color(s)
+			// If camera is inside cloud and fog is enabled, use cloud's colors as sky colors.
 			video::SColor clouds_dark = this->clouds->getColor().getInterpolated(
 					video::SColor(255, 0, 0, 0), 0.9);
 			this->sky->overrideColors(clouds_dark, this->clouds->getColor());
 			this->sky->setInClouds(true);
 			this->runData.fog_range = std::fmin(this->runData.fog_range * 0.5f, 32.0f * BS);
-			// do not draw clouds after all
+			// Clouds are not drawn in this case.
 			this->clouds->setVisible(false);
 		}
 	} else {

I found no changes/regressions from this PR.

@JosiahWI JosiahWI requested a review from srifqi November 6, 2023 12:43
@srifqi srifqi merged commit 5690274 into minetest:master Nov 8, 2023
13 checks passed
@JosiahWI JosiahWI deleted the refactor/extract-update-clouds branch November 8, 2023 03:13
cx384 pushed a commit to cx384/minetest that referenced this pull request Dec 9, 2023

Co-authored-by: Gregor Parzefall <82708541+grorp@users.noreply.github.com>
kawogi pushed a commit to kawogi/minetest that referenced this pull request Dec 19, 2023

Co-authored-by: Gregor Parzefall <82708541+grorp@users.noreply.github.com>
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