-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Allow fog color to be overriden (properly) #14296
Conversation
@@ -981,7 +974,6 @@ class Game { | |||
bool *kill; | |||
std::string *error_message; | |||
bool *reconnect_requested; | |||
scene::ISceneNode *skybox; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this variable was unused
@@ -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 }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as was this one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can confirm that this fixes #13421, i.e. that the new fog_color
field overrides the underground fog color of type = "regular"
skyboxes.
src/client/sky.h
Outdated
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where should they be moved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps directly into Client
or Game
, not entirely sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't that be worse? both of these classes are already quite big
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :) )
Not my bug, but I've included a fix. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM apart from that
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if alpha is less than 1? Is/should bgColor be partially visible then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah sorry, 255.
I can't think of a usecase. I was just wondering because we do check for it being 0.
Looks good. Please see my comment on the comment :) and the question on alpha. |
I think that's just a regular culling bug. |
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
fog and the far edges of clouds(!) should turn red