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

Ambient light and server control for it #14343

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

Andrey2470T
Copy link
Contributor

@Andrey2470T Andrey2470T commented Feb 3, 2024

This PR allows to add ambient light for environment per player that exists independently from the natural and artificial light sources. It is controlled by the new property ambient_light = <ColorSpec> of player:set_lighting. It makes possible to adjust brightness and color of poorly eliminated places e.g. caves, nether biome and any other closed spaces.

ambient_light_test.mp4
Some old screenshots

{luminance = 8, color = "green"}
Снимок экрана от 2024-02-03 22-41-22

{luminance = 5, color = "yellow"}
Снимок экрана от 2024-02-03 22-42-25

{luminance = 3, color = "red"}
Снимок экрана от 2024-02-03 22-51-32

This PR is Ready for Review.

How to test

  1. Enable shaders in the settings.
  2. Select devtest game and start the world.
  3. Enter /set_lighting command that will show the menu with scrollbars adjusting some lighting parameters.
  4. Adjust the red, green, blue channels of the ambient light.

Copy link

@nauta-turbidus nauta-turbidus left a comment

Choose a reason for hiding this comment

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

I feel like the names of the forceUpdateMapblockMeshes() function and the m_update_mapblock_meshes member variable flag aren't descriptive enough. In the same time, they're quite long already and I don't have any better ideas.

Aside of that, the code generally looks fine and seems to uphold existing conventions.

Haven't tested yet.

Another thing I would like to know, while I am not really concerned about it, how expensive is the forced mapblock meshes update?

src/client/clientenvironment.cpp Outdated Show resolved Hide resolved
@Zughy Zughy added Feature ✨ PRs that add or enhance a feature Roadmap The change matches an item on the current roadmap. labels Feb 3, 2024
@Andrey2470T
Copy link
Contributor Author

I feel like the names of the forceUpdateMapblockMeshes() function and the m_update_mapblock_meshes member variable flag aren't descriptive enough. In the same time, they're quite long already and I don't have any better ideas.

I think they sound fine. What this method does is enable that flag notifying in such way updateDrawList() about the need of adding the mesh update tasks (addUpdateMeshTask()) for the most actual m_drawlist. The same happens but only the one mapblock mesh is updated when e.g. a player made cracks on nodes.

Another thing I would like to know, while I am not really concerned about it, how expensive is the forced mapblock meshes update?

That causes a slight delays within a few seconds since as I said above it updates meshes of all currently loaded mapblocks.

@HybridDog
Copy link
Contributor

Adding a fullbright mode where everything is visible without torches is a thing which should be done right away since 5.4.2013 or since even before that date. I think a configurable ambient light makes a fullbright mode possible.

https://dev.minetest.net/TODO#Other
#6509
#6844

@Andrey2470T Andrey2470T force-pushed the ambient_light branch 2 times, most recently from 6daa777 to fe0a1a3 Compare February 24, 2024 15:15
Copy link
Contributor

@appgurueu appgurueu left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Also thanks to the people who took a closer look at this before I did.
The way ambient light is applied seems to be correct.

It does not seem to be gamma-correct, but I have to agree with HybridDog that this is out of the scope for this PR to fix (simply because Minetest is gamma-incorrect pretty much everywhere, it seems?).

Rudimentary testing (RGB): Works.

src/client/game.cpp Outdated Show resolved Hide resolved
src/client/mapblock_mesh.cpp Outdated Show resolved Hide resolved
src/client/mapblock_mesh.cpp Outdated Show resolved Hide resolved
doc/lua_api.md Outdated Show resolved Hide resolved
doc/lua_api.md Outdated Show resolved Hide resolved
src/network/clientpackethandler.cpp Outdated Show resolved Hide resolved
src/server.cpp Outdated Show resolved Hide resolved
src/client/clientmap.cpp Outdated Show resolved Hide resolved
src/client/mapblock_mesh.cpp Outdated Show resolved Hide resolved
client/shaders/nodes_shader/opengl_fragment.glsl Outdated Show resolved Hide resolved
@appgurueu appgurueu added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Mar 3, 2024
@Zughy Zughy removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Mar 4, 2024
@appgurueu
Copy link
Contributor

appgurueu commented Mar 4, 2024

This does not seem to look the same with shaders enabled / disabled:

Enabled:

black areas are black

Disabled:

black areas are blue as expected

I don't know what we can do about that / if we can do something about that.

We could make this a shader-only feature (like other lighting parameters). Then we could also stop caring about the performance of this if shaders aren't available.

Minetest might eventually want to require shaders (but that would require knowing what the extent of breakages would be).


I'm also starting to doubt whether min(x, 1) is the correct thing to do... This effectively means that "light" is capped at "full intensity" on all channels (as opposed to capping the "reflected" light). This enables fullbright (by setting ambient light to full white) and "reduces tint", which is why I think it's good.

We could add a second color after multiplying with texture color for said "tint".

@HybridDog should the light color be clamped after or before multiplying with the texture color, or something else entirely?

@HybridDog
Copy link
Contributor

We could make this a shader-only feature (like other lighting parameters). Then we could also stop caring about the performance of this if shaders aren't available.

I don't know exactly how the mapblock meshes, i.e. the vertex buffers, are generated; I assume they are not generated every frame and a change in ambient lighting would need a regeneration of all mapblock meshes for the change to take effect if the ambient light cannot be configured with a shader uniform. Perhaps Irrlicht has an option to set the ambient light in the fixed function pipeline, which would be faster than regenerating mapblock meshes.

@HybridDog should the light color be clamped after or before multiplying with the texture color, or something else entirely?

It should not be clamped if the shader outputs colours as float values and post processing is enabled since the second stage shaders already clamps the colour. If it outputs 8-bit colours, which can happen if floating point textures are unsupported by the GPU I think, or if post processing is disabled, it should clamp the colour at the end before storing it in gl_FragData[0].

Clamping RGB values directly gives bad results, as explained at https://bottosson.github.io/posts/gamutclipping/.
I think clipping colours in the OKLab colour space or a similar colour space could give good results but may be difficult to implement. hdr-toys implements some gamut mapping algorithms.
A simpler approach, which may also give better results than a simple RGB clamping (I haven't tested it thoroughly), is clamping the saturation in the YCbCr colour space and then the RGB values:

// Clamp the saturation in YCbCr before a clamp in RGB for a better colour
// preservation
vec3 clamp_saturation(vec3 color)
{
	vec3 yuv = rgb_to_ycbcr(color.rgb);
	yuv.yz = clamp(yuv.yz, vec2(-0.5), vec2(0.5));
	return ycbcr_to_rgb(yuv);
}

color.rgb = clamp_saturation(color.rgb);
color.rgb = clamp(color.rgb, vec3(0.), vec3(1.));

@appgurueu
Copy link
Contributor

appgurueu commented Mar 6, 2024

Thanks for the reply.

I don't know exactly how the mapblock meshes, i.e. the vertex buffers, are generated; I assume they are not generated every frame and a change in ambient lighting would need a regeneration of all mapblock meshes for the change to take effect if the ambient light cannot be configured with a shader uniform.

Yes. (It does not need a full regeneration of the meshes, though: Just updating the vertex colors should be enough.)

Perhaps Irrlicht has an option to set the ambient light in the fixed function pipeline, which would be faster than regenerating mapblock meshes.

It seems I gave up on looking for this too quickly: There is indeed a setAmbientLight function. Just calling this seems to do nothing, though; we might also have to enable lighting, or to install a dummy light source with only ambient color.

I'm also not sure whether this does what we want, though.
The problem is that we set vertex colors to emulate light in the fixed function pipeline. These vertex colors would be multiplied with the ambient light if I'm not mistaken, when we would really want them to be added to the light, then multiplied with the texture color. If something is pitch black, it will stay pitch black, for example.

It should not be clamped if the shader outputs colours as float values and post processing is enabled since the second stage shaders already clamps the colour.

That seems reasonable. However, I'm confused by what our shader is currently doing:

  • It's clamping the RGB (which you shouldn't do)?
  • It's clamping before applying tonemapping? Does this not defeat the purpose of tonemapping?
  • It's applying linear RGB -> sRGB conversion, but were the colors we're working with ever converted from sRGB -> linear RGB in the first place? (Should we convert ambient light from sRGB to linear RGB?)

If it outputs 8-bit colours, which can happen if floating point textures are unsupported by the GPU I think, or if post processing is disabled, it should clamp the colour at the end before storing it in gl_FragData[0].

Ick. Fair enough.

Clamping RGB values directly gives bad results, as explained at https://bottosson.github.io/posts/gamutclipping/. I think clipping colours in the OKLab colour space or a similar colour space could give good results but may be difficult to implement. hdr-toys implements some gamut mapping algorithms. A simpler approach, which may also give better results than a simple RGB clamping (I haven't tested it thoroughly), is clamping the saturation in the YCbCr colour space and then the RGB values: [...]

Thanks for the suggestion, I'll give this a try.

Side note: If we don't do RGB clamping, then this feature can not be used to implement fullbright, but that's probably how it should be; ambient light simply does not work like that. The implementation of fullbright would be very similar, though.

@nauta-turbidus thoughts?

@HybridDog
Copy link
Contributor

I'm also not sure whether this does what we want, though.
The problem is that we set vertex colors to emulate light in the fixed function pipeline. These vertex colors would be multiplied with the ambient light if I'm not mistaken, when we would really want them to be added to the light, then multiplied with the texture color. If something is pitch black, it will stay pitch black, for example.

It sounds wrong to me, too. The vertex colour should be added to and not multiplied by the ambient colour.

[The second stage shader is] clamping the RGB (which you shouldn't do)?

As far as I know, Minetest currently uses only colours with values in $[0,1]$ (i.e. SDR), so the clamping may be redundant. Perhaps it was added there.

It's clamping before applying tonemapping? Does this not defeat the purpose of tonemapping?

"tonemapping" in Minetest is just a visual effect and does not map HDR values to SDR since the input is already SDR. Before this commit the clamping happened after the tonemapping; I don't know why the order is changed now.

It's applying linear RGB -> sRGB conversion, but were the colors we're working with ever converted from sRGB -> linear RGB in the first place? (Should we convert ambient light from sRGB to linear RGB?)

It's converted from sRGB (approximated by 2.2 gamma) to linear RGB and later back to sRGB, so only bloom and the exposure adjustment are done in linear RGB.

@Andrey2470T Andrey2470T force-pushed the ambient_light branch 2 times, most recently from 1aea0c9 to 5d889cc Compare March 8, 2024 14:04
@Andrey2470T
Copy link
Contributor Author

Andrey2470T commented Mar 8, 2024

I've rebased the branch and deleted the ambient light calculation for the vertex buffers of the mapblock meshes by such way allowing to calculate it only in the shaders. The reason of that is animate() behaves undefinitely. It can update the ambient light, but can not do it on some mapblocks. I struggled to find out it today, but unfortunately I failed. Besides, I think it should be a "shader feature", so needing users to enable shaders to run it.

As to clamping the output colors, I can confirm what it is even not just recommended, it is even necessary. Because without that, it provides a such result:
Снимок экрана от 2024-03-08 14-50-15

With that:
Снимок экрана от 2024-03-08 14-51-00

@nauta-turbidus
Copy link

Work well for nodes and objects, quite intuitive API (aside of alpha, but more on that later). It however doesn't work for the particles. As you can see, despite a fullbright (255, 255, 255) ambient light, particles are rendered as black when no other source of light (like the sun) is present.
Screenshot_20240325_001928
Screenshot_20240325_001938

Now to the alpha... I guess throwing an error is fine, and leaving it as a field that can be expected to be there and yet does nothing would be even worse, but I still find it funny that modder has to provide a specific value, as any other errors out. Again though, I have no better idea than that, and this is quite foolproof I think.

@appgurueu
Copy link
Contributor

appgurueu commented Mar 31, 2024

Now to the alpha... I guess throwing an error is fine, and leaving it as a field that can be expected to be there and yet does nothing would be even worse, but I still find it funny that modder has to provide a specific value, as any other errors out. Again though, I have no better idea than that, and this is quite foolproof I think.

You are right that this is a bit of a hack. It stems from the fact that our "colorspecs" and Irrlicht's SColor support alpha, but we don't need or want alpha here. Hence we enforce that the alpha be "unused" / that colors be opaque.

The proper solution would be to have "RGB-only" colorspecs / SColor, of which our current colorspecs and SColor would be extensions, but I think that is out of scope for this PR.

It should be noted that in practice, the modder does not "need to provide a specific value" - opaque colors are the default for colorspecs. If you pass {r = r, g = g, b = b}, a will default to 255, as it should, and you will be fine. If you pass "green" or "#00FF00", you will also get an opaque color. If you pass "green#42", expecting the #42 (alpha) to do something special, you will get an error, as you should. (The only case where there isn't (and can't be) a default, and which hence is slightly ugly, is when you use colorspec integer form: 0x00FF00 will throw an error, since alpha is 0. You would have to use 0xFF00FF00.)


As for the particles, I think one option would be to add a particle shader. (This might be a good idea for performance-related reasons anyways.) I might make a PR for it.


@Andrey2470T excepting the particles, the only thing I'm unsure about concerning this PR is when to do the clamping. I think I have to agree with HybridDog that if we want to have somewhat physically realistic light, we need to do the clamping after applying the ambient light, rather than clamping the light color. The upside is that strongly colored ambient light remains strongly colored, even in full brightness. (Whereas when clamping the light, you get the somewhat "weird" effect of colors only "shining through" the darker a place is.)
The downsides are that clamping after applying the light to the texture (1) makes using this for proper fullbright impossible (2) modders may prefer the "physically incorrect" behavior.

I hence propose what I think may be an interesting "middle ground":

  • Ambient light is physically accurate. It is not clamped before being applied. The final colors are already clamped.
  • We add another "clamped light" color, which is added to the light color, the result of which is clamped before being applied. The ambient light is added after this.
  • (Of course this would need to be documented properly.)

Thoughts?

@nauta-turbidus
Copy link

nauta-turbidus commented Mar 31, 2024

@appgurueu In my opinion, ambient light is inherently "physically inaccurate". There's no such thing in real world.

The main thing that we should want about ambient light is that it

  1. is flexible and
  2. doesn't break other (actual) light sources

This PR feels like it fulfills both these, but I'm not 100% sure about (2), and I don't really understand how would your proposed middle ground work, how would it look, and how it would be better than what we have now in this PR.

@appgurueu
Copy link
Contributor

appgurueu commented Mar 31, 2024

@appgurueu In my opinion, ambient light is inherently "physically inaccurate". There's no such thing in real world.

Indeed ambient light is intended as an approximation of "scattered" light. I think it also makes sense to choose the "better" approximation among two approximations.

Still, it is a well-defined term in computer graphics, and we probably don't want to diverge from the established light models when we use the same terms.

I don't really understand how would your proposed middle ground work, how would it look, and how it would be better than what we have now in this PR.

My proposed middle ground is to extend the API to support two colors, say ambient_light and clamped_light.

The color of a fragment would then (very roughly) be calculated as (min(light + clamped_light, 1) + ambient_light) * texture_color. That is, clamped light is added to normal light, then clamped; ambient light is just added and not clamped.

The final color is clamped (much later) anyways.

As "special cases", my proposal supports:

  • Fullbright as clamped light = (1, 1, 1), ambient light = (0, 0, 0).
  • Arbitrary "clamped light" (what this PR currently implements) by setting clamped light = (0, 0, 0).
  • Arbitrary ambient light, by setting clamped light = (0, 0, 0).

In general, my proposal would support a mix of clamped light (which "complements" existing light sources so to speak) and ambient light, which is unconditionally added on top.

For an example of what the difference between "ambient" and "clamped" light may look like, see Andrey's comment here. In particular, see how the "tint" of the ambient light (in this case full red) goes away in broad daylight:

Screenshot from 2024-03-31 03-08-56

This may or may not be what modders want. My proposal would give them more flexibility, at the risk of being more confusing and slightly more complex.

@nauta-turbidus
Copy link

@appgurueu OK, I'm almost convinced.

What would be the result of setting clamped_light = {1,1,1} and ambient_light = {1,1,1}?

@appgurueu
Copy link
Contributor

What would be the result of setting clamped_light = {1,1,1} and ambient_light = {1,1,1}?

(min(light + clamped_light, 1) + ambient_light) * texture_color = (1 + ambient_light) * texture_color = 2 * texture_color - it would double the RGB components for the texture color. (If we want to give some advice in the docs: It's probably usually not desirable to have ambient light + clamped light > 1 for any component.)

@nauta-turbidus
Copy link

What would be the result of setting clamped_light = {1,1,1} and ambient_light = {1,1,1}?

(min(light + clamped_light, 1) + ambient_light) * texture_color = (1 + ambient_light) * texture_color = 2 * texture_color - it would double the RGB components for the texture color. (If we want to give some advice in the docs: It's probably usually not desirable to have ambient light + clamped light > 1 for any component.)

Since clamped_light is added to light from other sources, it is very easy to exceed 1 on the sum by adding the ambient_light and "overbrighten" the texture_color anyway...

Copy link
Contributor

@appgurueu appgurueu left a comment

Choose a reason for hiding this comment

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

Works, adds a useful feature (ambient light with clamping). This has my approval as-is.


Some minor things that I'd still like to point out:

  • Perhaps call this something different, to avoid confusion when "unclamped" ambient light is expected. (I don't have an idea for a good name, though.)
  • The comments in the new files say only Copyright (C) 2024 Andrey2470T, AndreyT <andreyt2203@gmail.com>, which is slightly misleading, since it doesn't mention the original authors of the code (which are still in the Git history though). (That said, I think many of our authorship comments are outdated.)

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.

  • When shaders are disabled, particles still get ambient light while nodes and entities don't. This should be consistent. I agree with the decision not do extra work to support enable_shaders = false.

  • Some nodes become weirdly desatured with ambient light. Example with Mineclonia, /time 23:00, //lua minetest.get_connected_players()[1]:set_lighting({ambient_light = "#ffffff"}):

    screenshot

    All of the affected nodes are hardware-colored (https://github.com/minetest/minetest/blob/5.8.0/doc/lua_api.md#hardware-coloring). Entities, particles, etc. are not affected.

  • I'd like to note that this PR is effectively a reimplementation of Night vision #11499 with a different API (the other PR had split brightness and color) and a different PR title.

@@ -473,6 +474,11 @@ class GameGlobalShaderConstantSetter : public IShaderConstantSetter
get_sunlight_color(&sunlight, daynight_ratio);
m_day_light.set(sunlight, services);

auto lighting = m_client->getEnv().getLocalPlayer()->getLighting();
Copy link
Member

Choose a reason for hiding this comment

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

Unresolved?
My IDE agrees with HybrigDog.

@@ -0,0 +1,61 @@
/*
Minetest
Copyright (C) 2024 Andrey2470T, AndreyT <andreyt2203@gmail.com>
Copy link
Member

Choose a reason for hiding this comment

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

please fix the copyright header

@@ -0,0 +1,98 @@
/*
Minetest
Copyright (C) 2024 Andrey2470T, AndreyT <andreyt2203@gmail.com>
Copy link
Member

Choose a reason for hiding this comment

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

please fix the copyright header

return video::SColor(r, b, b, b);
}

void get_sunlight_color(video::SColorf *sunlight, u32 daynight_ratio){
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
void get_sunlight_color(video::SColorf *sunlight, u32 daynight_ratio){
void get_sunlight_color(video::SColorf *sunlight, u32 daynight_ratio)
{

more code style: the ambientLight parameter in various functions should be ambient_light

@grorp grorp added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label May 8, 2024
@grorp grorp self-assigned this May 8, 2024
@HybridDog
Copy link
Contributor

HybridDog commented May 9, 2024

[Hardware-colored] nodes become weirdly desatured with ambient light.

This is probably caused by the clamping, similar to the green color in #14343 (comment).

@appgurueu
Copy link
Contributor

Shouldn't hardware coloring first be applied to the texture color? Ambient light should be clamped and applied only after that. So rather than (light + ambient_light + hardware_color) * texture_color it should be clamp(light + ambient_light) * hardware_color * texture_color.

@HybridDog
Copy link
Contributor

Yes, I haven't noticed this before. I think this can be fixed by adding the ambientLight earlier in the vertex shader:

	color.rgb = color.rgb * (color.a * dayLight.rgb +
		nightRatio * artificialLight.rgb + ambientLight) * 2.0;

If I understand it correctly, it incidentally also means that it is impossible to add ambient light in the fragment shader because varColor is the light multiplied with the texture color.

@Andrey2470T
Copy link
Contributor Author

Yes, I haven't noticed this before. I think this can be fixed by adding the ambientLight earlier in the vertex shader:

	color.rgb = color.rgb * (color.a * dayLight.rgb +
		nightRatio * artificialLight.rgb + ambientLight) * 2.0;

If I understand it correctly, it incidentally also means that it is impossible to add ambient light in the fragment shader because varColor is the light multiplied with the texture color.

That solves the problem with the desaturation, however this is wrong since it is already not an ambient light literally. In such case it actually changes the tint of the mix of the sunlight and artificial light leading to that the places don't get colorized by the ambient light where the artificial lights are absent or the sunlight doesn't hit directly and e.g. the caves stay fully dark:
Снимок экрана от 2024-05-11 01-40-16

@nauta-turbidus
Copy link

Fixed the desaturation problem here:

2df2372

@HybridDog
Copy link
Contributor

You're right. I have overlooked that color.rgb contains shading information, too.

So, if I understand it correctly, it's impossible to calculate clamp(light + ambient_light) * hardware_color * texture_color in the shader without changing the vertex data shader input because we don't have access to light and hardware_color but only inVertexColor, which is hardware_color * light. I don't know a simple solution to this problem.

@HybridDog
Copy link
Contributor

HybridDog commented May 12, 2024

Fixed the desaturation problem here:

2df2372

I think this may not work if some mod uses hardware node coloring to brighten or darken nodes instead of coloring them.

@nauta-turbidus
Copy link

Fixed the desaturation problem here:
2df2372

I think this may not work if some mod uses hardware node coloring to brighten or darken nodes instead of coloring them.

Does hardware coloring darken nodes?

@appgurueu
Copy link
Contributor

Does hardware coloring darken nodes?

Yes, it may

@nauta-turbidus
Copy link

Does hardware coloring darken nodes?

Yes, it may

If you point me to a specific instance, I could look into it too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Action / change needed Code still needs changes (PR) / more information requested (Issues) @ Client rendering Feature ✨ PRs that add or enhance a feature One approval ✅ ◻️ Roadmap The change matches an item on the current roadmap. @ Script API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet