-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Implement Physical Light Units in Vulkan Renderers #63751
Conversation
Any change in visual or is only a matter of unit definition? |
Default visuals are the same. But now lights can be much brighter without breaking rendering and lights can optionally specify color temperature to modify their color (in addition to the regular color property) |
Im trying to test this PR. So far I see everything shining white. I will keep testing it later. BTW, Intensity is only in lux. Is other unit going to be implemented? Can the temperature be a slider with a temperature color hint? |
You will need to add a CameraAttributes resource to your WorldEnvironment using it you can set your exposure settings. By default a brightness of 100000 lux will make your image over-exposed. The default settings for CameraAttributesPhysical should work fine.
I have no plans to add another unit. For directional lights Lux is the only unit that makes sense. Omnilights and Spotlights are in Lumens, I have no plans to add another unit, but we could add candela's if people really need it. Other light sources (Sky, emission etc.) are in nits already. For these light sources, only nits really makes sense
Sure. I was going to make a proposal for that after this PR is merged. I am not good with editor/ui stuff so I will leave that for someone else. What would be nice is just having the color spectrum with a slider that moves across it |
Testing it out. A few things to mention: Lightmaps dont work, even with the camera attribute set on it. |
@clayjohn I would recommend you to post some screenshots for people to appreciate your work on more detail. |
In my testing Lightmaps work fine, have you set CameraAttributes on both the Lightmap (before baking) and on the WorldEnvironment? Maybe there is some edge case I have not tested. If you have a simple MRP you could share that would be super helpful! I haven't changed the auto exposure effect and I am unlikely to dig too deep into it in this PR as the size is already getting out of hand. |
MRP |
7ba8809
to
fe43fc9
Compare
7a12f56
to
0fdc660
Compare
@jcostello I am running your MRP now. It looks like you used a CameraAttributesPractical with default exposure, but your DirectionalLight3D has an intensity of 100000 lux (also default). This causes the LightmapGI to be way over exposed. Swapping out the CameraAttributesPractical for a CameraAttributesPhysical (with default settings) makes your MRP look like this: I will clarify this more in the documentation. But when using Physical Light Units you have to be very careful that you manage the dynamic range well with exposure settings. This is a big part of why we have avoided implementing them until now. When baking lighting, users are going to have to manually set CameraAttributes settings to avoid overexposing and overflowing colors, or underexposing and creating banding. |
@clayjohn Thanks. I tried this and worked 💪 |
35ef4b5
to
556e4ab
Compare
I tested this PR. There's some good stuff, and some very broken stuff. It was getting too much to describe in text, so I made a video review: Summary points:
|
@tinmanjuggernaut Thank you for your comments, most of them I can address as I finish the PR! |
556e4ab
to
a4326c2
Compare
@tinmanjuggernaut Thank you for your comments. I have addressed most of them in the most recent push and I will now watch the video before seeing if I can address the rest. It should feel a lot more stable now as the defaults should all be synchronized nicely.
Fixed
I'm not sure what you mean by this. The sun is now 100,000 Lux instead of 1 arbitrary unit. reducing the suns strength to 1 lux would remove the benefit of using physical light units altogether. I think the core issue you are noticing is there was a poor alignment of default settings leading to the sun being way too strong. I have adjusted the defaults, including adding a CameraAttributesPhysical to the editor default WorldEnvironment which makes the 100,000 lux sun look good by default
Fixed
In addition to the above, I also changed the background intensity to 30,000 nits so it should look good with defaults now
Done
I haven't changed Auto exposure in this PR, but it certainly needs a little work now.
Done |
@clayjohn when you work on exposure, please check that the exposure |
With just the default omni/spot lights to light up stuff, and the default physical camera attributes, the scene is nearly pitch black right now.. Also Emissive materials need some work too. |
Seems to be an issue with master |
@tinmanjuggernaut What settings are you using for auto-exposure? You may have to expand the luminance range. We set caps on the luminance range in auto exposure so that you can still create bright and dark areas in your game Matius Goldberg explains it really well i the original PR. The default settings work will for default exposure ranges, if you push exposure to the extreme you will need to adjust the auto exposure to accept extreme values as well.
Updated, for some reason I lost this change right before pushing. Should be fixed now.
Reduced range down to 0.5
I wasn't able to reproduce this on my machine (intel integrated). I wonder if you are getting NaNs when your overexposed by a large enough factor. |
2dacc6f
to
9c890ef
Compare
NVidia RTX 3070. Exposure already allows many stops of blow out before turning black. Perhaps we can determine what exposure value causes white to turn to black and clamp it. Something like 10 stops over exposed should be sufficient, as anything over say 5 is going to be all white anyway.
I just left the defaults. When I and all other photographers and cinematographers enable auto exposure, the expectation is that the camera will adjust exposure of the current image to 50% middle gray. What "50% middle gray" means depends on the metering method, which could take a straight average of all pixels, or it could weight the center more heavily, or use a histogram, or many other methods. Any settings for auto exposure are to override it, meaning auto expose to 50% gray, then add or minus stops. I would expect AE settings to have only:
What I experienced is:
At the moment it is unreliable, inconsistent, not intuitive, and not how real analog or digital cameras work. |
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 far as I can judge code wise this all looks pretty solid to me. I must admit not being fully up to speed with the in's and outs of physical units but from what I understand of the general idea, I really like the approach of having separate settings for practical and physical units.
Nitpicking for the most part but my only two things I'd like to have further discussion on:
- Do the physical units result in higher intensity colors being stored in our 3D color buffer and thus would we run into trouble with the limited precision on the mobile renderer?
- Should we move
CameraAttributes
within the render server into its own storage object, having it inRenderSceneRender
seems counter productive in light of all the other work we've been doing on this front recently (off course keeping in mind the olderCameraEffects
was already inRenderSceneRender
, so this was really already outstanding).
@clayjohn Models in the import preview modal look all white (probably missing the camera attributes there) |
9c890ef
to
aa6043b
Compare
I have expanded the default range to handle extreme values better. It may be better to expose the range in EV-stops instead of luminance as EV scales better to extreme values. I'll try it out and see how it goes
Good catch! Fixed.
Not necessarily, light values are always pre-exposed so that the value stored in the buffer is approximately the same as it was before (for a correctly exposed scene). This goes for the 3D color buffer, for IBL, for Lightmaps, for VoxelGI, for SDFGI etc. |
9f1e4d6
to
e680544
Compare
This allows light sources to be specified in physical light units in addition to the regular energy multiplier. In order to avoid loss of precision at high values, brightness values are premultiplied by an exposure normalization value. In support of Physical Light Units this PR also renames CameraEffects to CameraAttributes.
e680544
to
385ee5c
Compare
I just figured this one out and it is now fixed! Rebased to fix merge conflicts, this should now be ready to go! |
This looks super solid to me, incredible work!! |
Let's merge! Thanks Clay for the awesome work, and thanks to everyone involved in testing and reporting bugs. Once merged, let's open dedicated issues for further bugs. |
This closes: godotengine/godot-proposals#4257
This allows light sources to be specified in physical light units in addition to the regular energy multiplier. In order to avoid loss of precision at high values, brightness values are premultiplied by an exposure normalization value.
In support of Physical Light Units this PR also renames CameraEffects to CameraAttributes.
This PR fully implements Physical Light Units and I believe it is mostly working as intended. I just need to polish the code and search for untested edge cases. I will update this description as I polish the feature.
Still TODO:
1. clean up, polish, and rebase2. Physical Lens properties3. Implement exposure settings in GLES3 and Vulkan Mobile4. Documentation