-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Conversation
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 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?
I think they sound fine. What this method does is enable that flag notifying in such way
That causes a slight delays within a few seconds since as I said above it updates meshes of all currently loaded mapblocks. |
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. |
6daa777
to
fe0a1a3
Compare
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.
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.
This does not seem to look the same with shaders enabled / disabled: Enabled: Disabled: 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 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? |
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.
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 Clamping RGB values directly gives bad results, as explained at https://bottosson.github.io/posts/gamutclipping/. // 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.)); |
Thanks for the reply.
Yes. (It does not need a full regeneration of the meshes, though: Just updating the vertex colors should be enough.)
It seems I gave up on looking for this too quickly: There is indeed a I'm also not sure whether this does what we want, though.
That seems reasonable. However, I'm confused by what our shader is currently doing:
Ick. Fair enough.
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? |
It sounds wrong to me, too. The vertex colour should be added to and not multiplied by the ambient colour.
As far as I know, Minetest currently uses only colours with values in
"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 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. |
1aea0c9
to
5d889cc
Compare
You are right that this is a bit of a hack. It stems from the fact that our "colorspecs" and Irrlicht's The proper solution would be to have "RGB-only" colorspecs / 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 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.) I hence propose what I think may be an interesting "middle ground":
Thoughts? |
@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
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. |
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.
My proposed middle ground is to extend the API to support two colors, say The color of a fragment would then (very roughly) be calculated as The final color is clamped (much later) anyways. As "special cases", my proposal supports:
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: 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. |
@appgurueu OK, I'm almost convinced. What would be the result of setting |
|
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... |
Did a quick test. This PR currently breaks node lighting. https://github.com/minetest/minetest/pull/14343/files#r1795490828 screenshot on the master branch (e3aa79c) screenshot with this PR (plain MTG) (20a1abb, as of now the latest commit) |
With this: Andrey2470T#3 it doesn't. |
Andrey tried too hard to make the unit tests not fail. |
Those unit tests are really broken... |
It seems some of them are still failing for some reason. |
Still looks noticeably worse than on master: Another scene. Also noticeable difference, but looks less bad: |
Apply review, fix stuff
@grorp "noticeably" ? I don't see absolutely any difference between your showed screenshots. On the first screenshot I can see, but Herowl has fixed now. |
per sfan5 request, uploaded the two sand images unsubscribing |
yeah I see the differences in the screenshots but I wonder what is causing it |
it looks like it sort of shrinks the torches lighting a little too |
I decreased the light precision to fit all the data in the few bytes we have, to allow ambient light to work with hardware coloring (grass color etc.)., see grass desaturation screenshots way above. To not cause issues like that, we'll have to make more space for all the data, and it's not trivial. |
yeah okay |
@Andrey2470T updates? I'm not closing it only because it's in the next milestone |
@Zughy After the PR had failed to come in the master branch for 5.10.0 release to be honest I did lose any motivation of continuing of the developing it and also a hope of it would get merged finally. I will close it not to hang out here open. |
Discussed in today's meeting, starting from here: https://irc.minetest.net/minetest-dev/2024-11-24#i_6222271 |
... aw man i was hoping for something like this to come into luanti |
@Wbjitscool me too. Not having this feature is very annoying. |
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>
ofplayer:set_lighting
. It makes possible to adjust brightness and color of poorly eliminated places e.g. caves, nether biome and any other closed spaces.Youtube Demo Video
https://m.youtube.com/watch?v=Lfk4XTqnZEU
Some old screenshots
ambient_light_test.mp4
{luminance = 8, color = "green"}
{luminance = 5, color = "yellow"}
{luminance = 3, color = "red"}
This PR is Ready for Review.
How to test
devtest
game and start the world./set_lighting
command that will show the menu with scrollbars adjusting some lighting parameters.