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

Implement Physical Light Units in Vulkan Renderers #63751

Merged
merged 1 commit into from
Sep 1, 2022

Conversation

clayjohn
Copy link
Member

@clayjohn clayjohn commented Jul 31, 2022

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 rebase
2. Physical Lens properties
3. Implement exposure settings in GLES3 and Vulkan Mobile
4. Documentation

@jcostello
Copy link
Contributor

Any change in visual or is only a matter of unit definition?

@clayjohn
Copy link
Member Author

clayjohn commented Aug 1, 2022

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)

@jcostello
Copy link
Contributor

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?

@clayjohn
Copy link
Member Author

clayjohn commented Aug 1, 2022

Im trying to test this PR. So far I see everything shining white. I will keep testing it later.

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.

BTW, Intensity is only in lux. Is other unit going to be implemented?

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

Can the temperature be a slider with a temperature color hint?

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

@jcostello
Copy link
Contributor

Testing it out. A few things to mention:

Lightmaps dont work, even with the camera attribute set on it.
The speed attribute in the auto exposure setting influences the exposure itself.

@reduz
Copy link
Member

reduz commented Aug 1, 2022

@clayjohn I would recommend you to post some screenshots for people to appreciate your work on more detail.

@clayjohn
Copy link
Member Author

clayjohn commented Aug 1, 2022

Testing it out. A few things to mention:

Lightmaps dont work, even with the camera attribute set on it. The speed attribute in the auto exposure setting influences the exposure itself.

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.

@jcostello
Copy link
Contributor

Im submitting MRP. I added a CameraAttributes to the environment and the lightmap before baking.

Lightmap disabled

image

Lightmap enabled

image

@jcostello
Copy link
Contributor

MRP

Lightmap.zip

@clayjohn
Copy link
Member Author

Some nice screenshots of SDFGI playing nicely with Physically Based Light units.
Screenshot from 2022-08-10 12-28-10
Screenshot from 2022-08-10 12-27-38

@clayjohn clayjohn force-pushed the physical_light_units branch 2 times, most recently from 7a12f56 to 0fdc660 Compare August 12, 2022 17:51
@clayjohn
Copy link
Member Author

@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:

Screenshot from 2022-08-12 11-58-13

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.

@jcostello
Copy link
Contributor

jcostello commented Aug 12, 2022

@clayjohn Thanks. I tried this and worked 💪

@clayjohn clayjohn force-pushed the physical_light_units branch 3 times, most recently from 35ef4b5 to 556e4ab Compare August 17, 2022 23:16
@clayjohn
Copy link
Member Author

Now physically-based DoF is working.

Here is an example with various settings and a telephoto lens:

Aperture: 16, Focal length: 200, Focus distance: 1m
Screenshot from 2022-08-17 17-18-53

Aperture: 16, Focal length: 200, Focus distance: 5m
Screenshot from 2022-08-17 17-19-04

Aperture: 16, Focal length: 200, Focus distance: 9m
Screenshot from 2022-08-17 17-19-11

Aperture: 4, Focal length: 200, Focus distance: 5m
Screenshot from 2022-08-17 17-19-30
Note: Shutter speed is also changed to compensate for the increase in aperture size

Some of the settings are a little adhoc as I used the existing DoF shader instead of writing a stochastic one from scratch. I think the results are pretty convincing (we may just need to tweak the total blur amount if this isn't realistic).

@TokisanGames
Copy link
Contributor

TokisanGames commented Aug 18, 2022

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:
https://youtu.be/CZm3WdFNUps (31min)

Summary points:

  • Default environment sun extremely large

  • Glow is strange

  • DirectionalLight is effectively 100,000x too strong, which blows out all materials

  • Blown out materials turn black instead of staying white

  • Blown out materials hide gizmo controls

  • Material previews are black (for basic white material)

  • Default camera settings create a black environment. The numbers are fine defaults, the effects are inaccurate.

  • Blown out objects create a shape larger than the actual object

  • Focal length doesn't change field of view, and vica versa

  • FOV is only partially locked out under Phys Camera. Either unlock it or completely lock it.

  • Exposure settings should all be in the exposure group

  • Recommend reordering settings: Focus Distance, Focal Length, Aperture, Shutter speed, Sensitivity, Multiplier.

  • Auto exposure only sometimes works, doesn't override exposure settings

  • Physical Lenses have different blur amounts for back focal plane and front focal plane depending on circumstances. Both the practical and physical cameras should have the option. The latter seems to have it already.

  • The Physical Camera does seem to be fairly accurate, supporting separate front/back blur amounts. I need to do more testing once I can get set up in my game with more complex objects. However I have only one minor issue with it below. I think it's good enough for this PR. I'll do more testing once I can get my game working with it in GD4.

  • The Physical Camera amount of blur is a bit weak. I was estimating the amount of blur on an object at 2m away is what 3m away should look like. So perhaps a 66.7% increase will achieve it. Your photos above are not representative of common photography. 200mm lens is a huge, telephoto lens. It's an odd choice to test with. An aperture of f/16 generally means everything is in focus. So I understand why you had to push the focal length all the way to 200 to get blur on f/16. 50mm lenses are considered normal, or the equivalent of how human vision works. Better testing numbers would be in the ranges of a normal 50mm f/1 to f/4, or a portrait 90mm f/1 to f/8. Using the DOF simulator to compare blur amounts as I did in the video is a reasonable method.

  • Here's how I change fov and focal length in real time:

func set_focal_length(value: float) -> void:
		focal_length = value
		fov = focal2fov(focal_length)
		focus_at(focus_point)


var _set_wait: bool = false
func set_fov(value: float) -> void:
		fov = value
		focal_length = fov2focal(fov)
		print("set_fov %f %f" % [ value, focal_length ])
		if is_inside_tree() and not _set_wait:
			_set_wait = true
			await get_tree().create_timer(1.0).timeout	# Allow user to update for 1 sec before processing sub tasks
			_set_wait = false
			focus_at(focus_point)

static func focal2fov(focal: float) -> float:
	return atan(12.0/focal) * 114.5916	# Simplified atan(24/2/focal_length) * 180/PI * 2
	
static func fov2focal(p_fov: float) -> float:
	return 12.0/tan(p_fov/114.5916)
	

@clayjohn
Copy link
Member Author

@tinmanjuggernaut Thank you for your comments, most of them I can address as I finish the PR!

@clayjohn
Copy link
Member Author

clayjohn commented Aug 20, 2022

@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.

  • Default environment sun extremely large

Fixed

  • DirectionalLight is effectively 100,000x too strong, which blows out all materials

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

  • Material previews are black (for basic white material)

Fixed

  • Default camera settings create a black environment. The numbers are fine defaults, the effects are inaccurate.

In addition to the above, I also changed the background intensity to 30,000 nits so it should look good with defaults now

  • Focal length doesn't change field of view, and vica versa
  • FOV is only partially locked out under Phys Camera. Either unlock it or completely lock it.
  • Exposure settings should all be in the exposure group
  • Recommend reordering settings: Focus Distance, Focal Length, Aperture, Shutter speed, Sensitivity, Multiplier.

Done

  • Auto exposure only sometimes works, doesn't override exposure settings

I haven't changed Auto exposure in this PR, but it certainly needs a little work now.

  • The Physical Camera amount of blur is a bit weak. I was estimating the amount of blur on an object at 2m away is what 3m away should look like. So perhaps a 66.7% increase will achieve it.

Done

@jcostello
Copy link
Contributor

@clayjohn when you work on exposure, please check that the exposure speed affect the exposure itself. I think this is an issue in this branch

@mrjustaguy
Copy link
Contributor

mrjustaguy commented Aug 22, 2022

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.

@jcostello
Copy link
Contributor

@jcostello Can you try your test scene in master and see if you have the same issues? I can't reproduce your issue 1 with this PR or in master.

Seems to be an issue with master

@clayjohn
Copy link
Member Author

clayjohn commented Aug 28, 2022

I'm not sure what auto exposure issues you fixed, because it still looks broken. I made the image very overexposed, all white, turned on auto exposure and it darkened the image but left it still very over exposed. Then I mildly under exposed (f/5, shutter 6000, iso 100), it brightens but its still under exposed. Then defaults (f/16, sh100, iso 100) turned on auto exposure and it did properly expose the middle of the image, but it left part of the image very blown out. I understand you have another PR for overhauling this.

@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.

I didn't see a default of 35mm. But whatever you guys decide is fine, I said my piece.

Updated, for some reason I lost this change right before pushing. Should be fixed now.

You could lower the aperture range to say 0.5, in case someone wants to get just a bit of extra blur, as such lenses exist f/0.95, f/0.7, and the lowest for spherical elements is f/0.5, or f/0.001? for aspheric lenses. In practice, I've only seen f/0.95 lenses and didn't know about the others until I looked it up.

Reduced range down to 0.5

Materials are broken on the ends of the ranges, eg. Everything that is overexposed should stay white.

f/4, shutter 0.1, iso 100 = white sky, white materials
f/3.5, shutter 0.1, iso 100 => white sky, materials are black !!
f/1, shutter 0.1-1.271 => white sky, materials are black !!
f/1, shutter 1.272-8000 => all white to color
f/1, shutter 0.1, iso 0.1-7.8 => all white
f/1, shutter 0.1, iso 7.9-32000 => white sky, black materials !!

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.

@clayjohn clayjohn force-pushed the physical_light_units branch 3 times, most recently from 2dacc6f to 9c890ef Compare August 29, 2022 01:02
@TokisanGames
Copy link
Contributor

TokisanGames commented Aug 30, 2022

f/1, shutter 0.1-1.271 => white sky, materials are black !!
f/1, shutter 1.272-8000 => all white to color
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.

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.

What settings are you using for auto-exposure?

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:

  • AE Metering mode (full image, spot, histogram, etc)
  • Increase/decrease exposure (after AE) by up to 5-10 stops
  • AE application speed

What I experienced is:

  • Way overexpose the image, and AE darkened it slightly, but still way overexposed.
  • Underexpose the image, AE slightly brightened it, but left it underexposed.
  • At other times over or underexposed, AE properly exposed it.

At the moment it is unreliable, inconsistent, not intuitive, and not how real analog or digital cameras work.

Copy link
Contributor

@BastiaanOlij BastiaanOlij left a 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 in RenderSceneRender seems counter productive in light of all the other work we've been doing on this front recently (off course keeping in mind the older CameraEffects was already in RenderSceneRender, so this was really already outstanding).

@jcostello
Copy link
Contributor

@clayjohn Models in the import preview modal look all white (probably missing the camera attributes there)

@clayjohn
Copy link
Member Author

What I experienced is:

  • Way overexpose the image, and AE darkened it slightly, but still way overexposed.
  • Underexpose the image, AE slightly brightened it, but left it underexposed.
  • At other times over or underexposed, AE properly exposed it.

At the moment it is unreliable, inconsistent, not intuitive, and not how real analog or digital cameras work.

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

@clayjohn Models in the import preview modal look all white (probably missing the camera attributes there)

Good catch! Fixed.

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?

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.

@clayjohn clayjohn force-pushed the physical_light_units branch 4 times, most recently from 9f1e4d6 to e680544 Compare August 31, 2022 18:53
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.
@clayjohn
Copy link
Member Author

Materials are broken on the ends of the ranges, eg. Everything that is overexposed should stay white.

f/4, shutter 0.1, iso 100 = white sky, white materials
f/3.5, shutter 0.1, iso 100 => white sky, materials are black !!
f/1, shutter 0.1-1.271 => white sky, materials are black !!
f/1, shutter 1.272-8000 => all white to color
f/1, shutter 0.1, iso 0.1-7.8 => all white
f/1, shutter 0.1, iso 7.9-32000 => white sky, black materials !!

I just figured this one out and it is now fixed!

Rebased to fix merge conflicts, this should now be ready to go!

@reduz
Copy link
Member

reduz commented Sep 1, 2022

This looks super solid to me, incredible work!!

@akien-mga
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add support for physical light units
8 participants