Allow fog color to be overriden (properly) #14296

sfan5 · 2024-01-21T19:34:00Z

problem: currently you can either opt for the regular skybox or decide the fog color, you can't do both.

closes #13421, see also #7284

To do

This PR is Ready for Review.

Note: I changed some indentation so it's best to review with whitespace disabled.

How to test

core.register_on_joinplayer(function(objref)
	core.after(1, function()
		objref:set_sky({
			fog = {
				fog_color = "red"
			}
		})
	end)
end)

fog and the far edges of clouds(!) should turn red

sfan5 · 2024-01-21T19:34:30Z

src/client/game.cpp

@@ -981,7 +974,6 @@ class Game {
 	bool *kill;
 	std::string *error_message;
 	bool *reconnect_requested;
-	scene::ISceneNode *skybox;


this variable was unused

sfan5 · 2024-01-21T19:34:40Z

src/skyparams.h

@@ -46,7 +46,7 @@ struct SkyboxParams
 	float body_orbit_tilt { INVALID_SKYBOX_TILT };
 	s16 fog_distance { -1 };
 	float fog_start { -1.0f };
-	float volumetric_light_strength { 0.0f };


as was this one

grorp

I can confirm that this fixes #13421, i.e. that the new fog_color field overrides the underground fog color of type = "regular" skyboxes.

doc/lua_api.md

grorp · 2024-01-21T21:11:46Z

src/client/sky.h

 		ITextureSource *tsrc);
 	const video::SColorf &getCurrentStarColor() const { return m_star_color; }
+
+	// Note: the sky class doesn't even use these values. It just stores them.
+	// These should be moved to a better place in the future.


Where should they be moved?

perhaps directly into Client or Game, not entirely sure

wouldn't that be worse? both of these classes are already quite big

Hey... We had a bunch of debates about where to put the server fog distance and fog start settings.

the sky class doesn't even use these values

That commend does not make sense to me. Maybe no code in Sky is using this, but it is currently a Sky feature.
"fog" could be a feature of the sky, or a feature of "lighting" or its own. We decided that the sky API is the right place, so on the client it ends up in Sky. That still makes sense to me.

Let's be careful where we put things and before we make any rash moves.

I'd prefer if we remove that comment until we have a firm decision about where is the right place.

Alright.

I'll use this opportunity to air another grievance however:
We have SkyboxParams, we receive it, push it through a client event and it arrives in the game code.
Now instead of giving it to the sky class where the actual business happens the game code pulls apart the structure and calls a bunch of methods on the sky class to effect the necessary changes.
This results in me having to add a setFoobar/getFoobar pair to Sky for every new property even though the sky class literally has a SkyboxParams member internally. how does this make sense??

I agree with that :)

We can file another PR to cleanup the API.
And while we're at it make sure we are happy with where I put volumetric_lighting. (It's part of the lighting API, but could be part of cloud, sky, or fog :) )

grorp · 2024-01-21T21:19:28Z

If I disable fog via F3, there is still fog on clouds (but less than with fog enabled?). I expect the fog on clouds to disappear as well if I disable fog.

player:set_sky({
	fog = {
		fog_color = "#61b5f5",
	},
})

screenshot with fog enabled

screenshot with fog disabled

sfan5 · 2024-01-22T09:55:56Z

If I disable fog via F3, there is still fog on clouds (but less than with fog enabled?). I expect the fog on clouds to disappear as well if I disable fog.

Not my bug, but I've included a fix.

grorp

LGTM apart from that

src/client/clouds.cpp

lhofhansl · 2024-01-22T17:49:54Z

src/client/sky.h

-	float getVolumetricLightStrength() const { return m_sky_params.volumetric_light_strength; }
+	void setFogColor(video::SColor v) { m_sky_params.fog_color = v; }
+	video::SColor getFogColor() const {
+		if (m_sky_params.fog_color.getAlpha() > 0)


What if alpha is less than 1? Is/should bgColor be partially visible then?

I assume you mean less than 255, because this isn't a float.
~~I don't know what happens if you set the fog to be semitransparent, does that make sense?~~ Edit: nothing happens, the alpha channel is not used.

If you can imagine an usecase for overriding the fog color in a way that it's mixed with the normal fog color then I'll add this.

Yeah sorry, 255.
I can't think of a usecase. I was just wondering because we do check for it being 0.

lhofhansl · 2024-01-22T17:51:01Z

Looks good. Please see my comment on the comment :) and the question on alpha.

src/client/game.cpp

lhofhansl · 2024-01-23T19:20:17Z

Underground there is this weird behavior:

At viewing range 50

At viewing range 40

Note that black "blob" that pops up from nowhere.

sfan5 · 2024-01-23T19:43:08Z

I think that's just a regular culling bug.
This is not a problem above ground because fog color = sky color so anything close to being culled has vanished into the background anyway. This falls apart if you use a different colors because our fog is not present in-world but only a shader effect on rendered terrain. (For a good demo fly far upwards with the red fog.)
🤷‍♂️

sfan5 added @ Script API @ Client / Audiovisuals Feature ✨ PRs that add or enhance a feature labels Jan 21, 2024

sfan5 commented Jan 21, 2024

View reviewed changes

sfan5 changed the title ~~Allow fog color to be overriden properly~~ Allow fog color to be overriden (properly) Jan 21, 2024

grorp reviewed Jan 21, 2024

View reviewed changes

Zughy added the Supported by core dev Not on the roadmap, yet some core dev decided to take care of this PR label Jan 21, 2024

sfan5 force-pushed the fogcolor branch from b3e7638 to 9037e4a Compare January 22, 2024 09:55

grorp approved these changes Jan 22, 2024

View reviewed changes

src/client/clouds.cpp Outdated Show resolved Hide resolved

grorp added the One approval ✅ ◻️ label Jan 22, 2024

lhofhansl reviewed Jan 22, 2024

View reviewed changes

Allow fog color to be overriden properly

56a284c

sfan5 force-pushed the fogcolor branch from 9037e4a to 56a284c Compare January 22, 2024 18:23

lhofhansl reviewed Jan 23, 2024

View reviewed changes

src/client/game.cpp Show resolved Hide resolved

lhofhansl added >= Two approvals ✅ ✅ and removed One approval ✅ ◻️ labels Jan 23, 2024

sfan5 merged commit 9e3a115 into minetest:master Jan 23, 2024
13 checks passed

sfan5 deleted the fogcolor branch January 23, 2024 21:34

theraven262 mentioned this pull request Mar 17, 2024

Sky goes black underground #14468

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow fog color to be overriden (properly) #14296

Allow fog color to be overriden (properly) #14296

sfan5 commented Jan 21, 2024 •

edited

sfan5 Jan 21, 2024

sfan5 Jan 21, 2024

grorp left a comment

grorp Jan 21, 2024

sfan5 Jan 22, 2024

grorp Jan 22, 2024

lhofhansl Jan 22, 2024

sfan5 Jan 22, 2024

lhofhansl Jan 23, 2024

grorp commented Jan 21, 2024

sfan5 commented Jan 22, 2024

grorp left a comment

lhofhansl Jan 22, 2024

sfan5 Jan 22, 2024 •

edited

lhofhansl Jan 23, 2024

lhofhansl commented Jan 22, 2024

lhofhansl commented Jan 23, 2024

sfan5 commented Jan 23, 2024

Allow fog color to be overriden (properly) #14296

Allow fog color to be overriden (properly) #14296

Conversation

sfan5 commented Jan 21, 2024 • edited

To do

How to test

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grorp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grorp commented Jan 21, 2024

sfan5 commented Jan 22, 2024

grorp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfan5 Jan 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhofhansl commented Jan 22, 2024

lhofhansl commented Jan 23, 2024

sfan5 commented Jan 23, 2024

sfan5 commented Jan 21, 2024 •

edited

sfan5 Jan 22, 2024 •

edited