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

AmbientLight docs doesn't specify units #11933

Open
fintelia opened this issue Feb 18, 2024 · 8 comments · May be fixed by #13297
Open

AmbientLight docs doesn't specify units #11933

fintelia opened this issue Feb 18, 2024 · 8 comments · May be fixed by #13297
Labels
A-Rendering Drawing game state to the screen C-Docs An addition or correction to our documentation D-Trivial Nice and easy! A great choice to get started with Bevy S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it!

Comments

@fintelia
Copy link
Contributor

How can Bevy's documentation be improved?

The documentation for AmbientLights brightness field says:

A direct scale factor multiplied with color before being passed to the shader.

However, it would be helpful to know the units of the product of multiplying color and the scale factor. I suspect the unit is either lux or nits, but I haven't investigated

@fintelia fintelia added C-Docs An addition or correction to our documentation S-Needs-Triage This issue needs to be labelled labels Feb 18, 2024
@james7132 james7132 added A-Rendering Drawing game state to the screen and removed S-Needs-Triage This issue needs to be labelled labels Feb 18, 2024
@JMS55 JMS55 added this to the 0.13.1 milestone Feb 18, 2024
@BD103
Copy link
Member

BD103 commented Mar 13, 2024

brightness is a multiplier, so it itself does not have any units. It was originally added in #1605, where it calculates:

(ambient_light_resource.color * ambient_light_resource.brightness)

Which is equivalent to a Vec4 * f32. On main, it currently looks like:

ambient_color: Vec4::from_slice(&LinearRgba::from(ambient_light.color).to_f32_array())
* ambient_light.brightness,

@fintelia
Copy link
Contributor Author

You could say the same thing about the skybox's brightness field. The documentation I'm looking for is the analog of this line:

Scale factor applied to the skybox image. After applying this multiplier to the image samples, the resulting values should be in units of cd/m^2.

@BD103
Copy link
Member

BD103 commented Mar 14, 2024

That's a very good point. So would AmbientLight::brightness also be in cd/m^2, or would it be some other unit?

@mockersf mockersf modified the milestones: 0.13.1, 0.13.2 Mar 18, 2024
@superdump
Copy link
Contributor

Looking at the code I think @fintelia ’s last comment is the correct answer.

@BD103
Copy link
Member

BD103 commented Mar 23, 2024

Marking this as a good first issue. AmbientLight's brightness field should document that it is in cd/m^2, similar to Skybox::brightness, if I'm understanding @superdump's comment correctly.

@BD103 BD103 added D-Trivial Nice and easy! A great choice to get started with Bevy S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! labels Mar 23, 2024
@janhohenheim
Copy link
Contributor

Maybe it would also be good to specify that this is not equivalent to Filament's IBL, even though the units are the same and they are both used for global illumination.

@superdump
Copy link
Contributor

@janhohenheim my understanding is that if one sets bevy’s ambient light brightness to 1 then it would be the same. Or you can consider the combination of color * brightness to be the same as Filament’s IBL.

@janhohenheim
Copy link
Contributor

janhohenheim commented Mar 23, 2024

@superdump I'm very skeptical because per google/filament#1667, Blender's background shader is the same as IBL, but my testing in #12280 reveals that it does not correspond at all to an AmbientLight when using a strength of 1.
Of course, I might be wrong in how I export stuff from Blender, so take this with a grain of salt.

@alice-i-cecile alice-i-cecile modified the milestones: 0.13.2, 0.14 Apr 1, 2024
@BD103 BD103 linked a pull request May 9, 2024 that will close this issue
@BD103 BD103 removed this from the 0.14 milestone May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Docs An addition or correction to our documentation D-Trivial Nice and easy! A great choice to get started with Bevy S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants