[Merged by Bors] - Rename Light => PointLight and remove unused properties #1778
Conversation
alice-i-cecile
added
C-Code-Quality
crates/bevy_pbr/src/light.rs
Outdated
| depth: 0.1..50.0, | ||
| fov: f32::to_radians(60.0), | ||
| intensity: 200.0, | ||
| intensity: 100.0, |
There was a problem hiding this comment.
I see you are changing the default intensity value from 200.0 to 100.0. Why?
Also, do examples that don't change the default still render OK?
There was a problem hiding this comment.
Hmm, not on purpose. I'll revert that.
| pub color: Color, | ||
| pub fov: f32, | ||
| pub depth: Range<f32>, |
There was a problem hiding this comment.
I presume the original reasoning for why these extra fields were included, was to reuse the same Light type for both Point lights and (future) Spot lights?
I think I like this change. As a user, I think having multiple distinct clearly-named types for different kinds of lights is a good idea.
As for the implementation, I am not very familiar with how differently the different kinds of lights have to be handled for shading. I know Directional/Ambient are special, and those better be their own independent types anyway, but for Point/Spot I am not so sure.
If it's not going to complicate passing of light data and handling in the shader, then this change seems good, also from an implementation perspective. I wonder what the arguments for unifying them into a single type are?
There was a problem hiding this comment.
From experience I have a strong preference to not put in unused code like this 'because we'll use it soon'. It's simple enough to put it in the next PR and it's only confusing at this point.
For example, while working on shadowmaps, the above fields needed changing anyway...
There was a problem hiding this comment.
I think separate types for each light type makes sense, even if point vs spot have some technical overlap. I expect most of the changes in this pr to hold up even when we add spotlights.
crates/bevy_pbr/src/light.rs
Outdated
| pos: [x, y, z, 1.0 / (light.range * light.range)], // pos.w is the attenuation. | ||
| PointLightUniform { | ||
| pos: global_transform.translation.into(), | ||
| inverse_range_squared: 1.0 / (light.range * light.range), |
There was a problem hiding this comment.
I like this. Having a separate field (with the same binary layout!) rather than overloading the 4th value, is much cleaner.
There was a problem hiding this comment.
Theoretically we waste 3 bytes per light.
There was a problem hiding this comment.
Yeah this is definitely clearer which I think is worth optimizing for, especially in the early days of bevy's renderer. Im down for this change for now. We can revisit later if needed.
Rename inverseRadiusSquared More renaming Restore original default intensity for PointLight Fix layout
|
bors r+ |
After an inquiry on Reddit about support for Directional Lights and the unused properties on Light, I wanted to clean it up, to hopefully make it ever so slightly more clear for anyone wanting to add additional light types. Co-authored-by: Carter Anderson <mcanders1@gmail.com>
|
Pull request successfully merged into main. Build succeeded: |
bors
bot
changed the title
Introduced in #1778, not fixed by #1931 The size of `Lights` buffer currently is : ```rust 16 // (color, `[f32; 4]`) + 16 // (number of lights, `f32` encoded as a `[f32; 4]`) + 10 // (maximum number of lights) * ( 16 // (light position, `[f32; 4]` + 16 // (color, `[16; 4]`) + 4 // (inverse_range_squared, `f32`) ) -> 392 ``` This makes the pbr shader crash when running with Xcode debugger or with the WebGL2 backend. They both expect a buffer sized 512. This can also be seen on desktop by adding a second light to a scene with a color, it's position and color will be wrong. adding a second light to example `load_gltf`: ```rust commands .spawn_bundle(PointLightBundle { transform: Transform::from_xyz(-3.0, 5.0, -3.0), point_light: PointLight { color: Color::BLUE, ..Default::default() }, ..Default::default() }) .insert(Rotates); ``` before fix: <img width="1392" alt="Screenshot 2021-04-16 at 19 14 59" src="https://user-images.githubusercontent.com/8672791/115060744-866fb080-9ee8-11eb-8915-f87cc872ad48.png"> after fix: <img width="1392" alt="Screenshot 2021-04-16 at 19 16 44" src="https://user-images.githubusercontent.com/8672791/115060759-8cfe2800-9ee8-11eb-92c2-d79f39c7b36b.png"> This PR changes `inverse_range_squared` to be a `[f32; 4]` instead of a `f32` to have the expected alignement
Introduced in bevyengine#1778, not fixed by bevyengine#1931 The size of `Lights` buffer currently is : ```rust 16 // (color, `[f32; 4]`) + 16 // (number of lights, `f32` encoded as a `[f32; 4]`) + 10 // (maximum number of lights) * ( 16 // (light position, `[f32; 4]` + 16 // (color, `[16; 4]`) + 4 // (inverse_range_squared, `f32`) ) -> 392 ``` This makes the pbr shader crash when running with Xcode debugger or with the WebGL2 backend. They both expect a buffer sized 512. This can also be seen on desktop by adding a second light to a scene with a color, it's position and color will be wrong. adding a second light to example `load_gltf`: ```rust commands .spawn_bundle(PointLightBundle { transform: Transform::from_xyz(-3.0, 5.0, -3.0), point_light: PointLight { color: Color::BLUE, ..Default::default() }, ..Default::default() }) .insert(Rotates); ``` before fix: <img width="1392" alt="Screenshot 2021-04-16 at 19 14 59" src="https://user-images.githubusercontent.com/8672791/115060744-866fb080-9ee8-11eb-8915-f87cc872ad48.png"> after fix: <img width="1392" alt="Screenshot 2021-04-16 at 19 16 44" src="https://user-images.githubusercontent.com/8672791/115060759-8cfe2800-9ee8-11eb-92c2-d79f39c7b36b.png"> This PR changes `inverse_range_squared` to be a `[f32; 4]` instead of a `f32` to have the expected alignement
After an inquiry on Reddit about support for Directional Lights and the unused properties on Light, I wanted to clean it up, to hopefully make it ever so slightly more clear for anyone wanting to add additional light types.