-
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
Ambient light and server control for it #14343
base: master
Are you sure you want to change the base?
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... |
8ab244d
to
b08ee98
Compare
What's your plan for this PR? The bug with hardware-colored nodes is still unsolved. I don't think it makes sense to ship this feature in a known buggy state. It's not like hardware-colored nodes are very rare either, e.g. the Mineclones have lots of them. |
Fwiw, I fixed the coloring and partially shading, my branch is linked above. The remaining bugs in my branch should be doable too. I could adopt it, theoretically... |
I'm currently working on the texture atlasing for mapblocks meshes. I showed some results of it in Discord a while ago and now it is necessary to solve the known artifacts with UV of some drawtypes nodes like nodebox, mesh, fencelike and some other ones. Also, except it I l was looking into deeply the reasons of the big count of the clientmap drawcalls and some algorithms improving the current frustum culling (the first prototype of this new rendering system I demonstrated in Discord lately too). However, this appeared to be far inefficient and after I got back to the atlas again. When I PR this feature (I hope very soon), I will return to this PR also. As to the problem concerning to the hardware-colored nodes, according to this comment: https://discord.com/channels/369122544273588224/369137254641303560/1272964923500134524 it still can be solved by embedding the new vertex format. The format may be distributed over the old GL drivers due to replacing the glVertex/Normal/TexCoordPointer with glVertexAttribPointer call (it already exists starting with OpenGL 2). And I think it shouldn't conflict with the fixed-function pipeline in these drivers. EDIT: Actually I hurried with my previous statement (I forgot the fixed function pipeline doesn't have shaders, so the new attribute won't just be sent to them, only the standard ones will. But if we remove the |
…blend() if shaders are off
…other improvements
…g light and hardware colors
…ng emissive color to GenericCAO
…ted adding emissive color to GenericCAO" This reverts commit b08ee98.
a73e37a
to
807cbb8
Compare
…te saving light and hardware colors" This reverts commit d3b63ba.
807cbb8
to
9eef510
Compare
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"}
![Снимок экрана от 2024-02-03 22-41-22](https://private-user-images.githubusercontent.com/25750346/302059504-c9a84ce7-03ee-4c14-aacc-17294681b944.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjQ4OTk2OTgsIm5iZiI6MTcyNDg5OTM5OCwicGF0aCI6Ii8yNTc1MDM0Ni8zMDIwNTk1MDQtYzlhODRjZTctMDNlZS00YzE0LWFhY2MtMTcyOTQ2ODFiOTQ0LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA4MjklMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwODI5VDAyNDMxOFomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTFkYmIzMWMyMzc0ZmM3NDU5ZDY5NGUzZTczYTc0ZjU0ZGI2YjU4NTUxMjA0NDhjN2NmMTI1ZDNiZDgwN2EyZGYmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.R7gx0rb99qwOmh4nbyBqNsaZvkj_B6cdf6e0PV8r3nM)
{luminance = 5, color = "yellow"}
![Снимок экрана от 2024-02-03 22-42-25](https://private-user-images.githubusercontent.com/25750346/302059536-6df6517a-81f4-4ba7-a3ff-90ad91019afc.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjQ4OTk2OTgsIm5iZiI6MTcyNDg5OTM5OCwicGF0aCI6Ii8yNTc1MDM0Ni8zMDIwNTk1MzYtNmRmNjUxN2EtODFmNC00YmE3LWEzZmYtOTBhZDkxMDE5YWZjLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA4MjklMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwODI5VDAyNDMxOFomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWFhMjk0NjE3YzkyMTk3YzllYmFjMjAwMzg1ZDczZDFmZGYxM2ViZDBkYjllYTA4N2Q0YTQ2ZmRlNWQzZGQ1NzkmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.64cY7yDzmajkKKOlI35yVaPNO_wOXD731WLZaSCmrdI)
{luminance = 3, color = "red"}
![Снимок экрана от 2024-02-03 22-51-32](https://private-user-images.githubusercontent.com/25750346/302059555-7940f597-ddcd-4b4c-8257-def1bdd4549f.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjQ4OTk2OTgsIm5iZiI6MTcyNDg5OTM5OCwicGF0aCI6Ii8yNTc1MDM0Ni8zMDIwNTk1NTUtNzk0MGY1OTctZGRjZC00YjRjLTgyNTctZGVmMWJkZDQ1NDlmLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA4MjklMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwODI5VDAyNDMxOFomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWY0ZDY3YThmYjIwOWM0YjY3Y2QxNjhhOGYwNDdjYjVhZDY4ZTEzMDgyZjMwZTUyNDdlM2M1YzU3M2RiZmJhNDcmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.eiBMOVdZi8GkQMI0B1drjKhYxXJMVaXditalsL-sr6U)
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.