Use crevice to write LightsUniform #1931
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mockersf
reviewed
Apr 15, 2021
crates/bevy_math/Cargo.toml
Outdated
Comment on lines
16
to
17
| glam = { version = "0.14.0", features = ["serde"], path = "../../../glam-rs" } | ||
| crevice = { version = "0.6", optional = true, path = "../../../crevice" } |
There was a problem hiding this comment.
could you put your forks of those on GitHub and point to them?
|
I'm very interested in this to keep backends other than the official one (mainly WebGL2) working. For example, the WebGL2 shader currently fails after #1778 got merged, even after applying the changes, due to a wrongly sized buffer |
Crap, that sucks. I need to make testing webgl part of my workflow for any future changes to shaders. |
mockersf
added
C-Code-Quality
bors bot
pushed a commit
that referenced
this pull request
Apr 19, 2021
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
cart
added
C-Dependencies
cart
added
the
S-Pre-Relicense
mockersf
removed
the
S-Pre-Relicense
ostwilkens
pushed a commit
to ostwilkens/bevy
that referenced
this pull request
Jul 27, 2021
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
|
Closing this now that |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a draft of what usage of crevice for writing Std140 layout looks like, see #1779.
Note that
WriteStd140is fairly low overhead, it converts glam types intoStd140primitives before serializing).AsStd140, which can be derived in many cases, potentially has more overhead, since it converts the entire struct into a version with padding andStd140primitives.Dynamic sizing of
WriteStd140is also not without cost, since it serializes the type toio::Sink()and useswriter.len()to calculate the size.This draft currently depends on an upcoming PR on crevice that adds the
StaticStd140Sizetrait, so that the maximum size ofLightsUniformcan be calculated.Additionally there's draft PR to glam to implement crevice traits. This is probably the most problematic part of crevice right now. Because of the orphan rule it's impossible to implement crevice traits without newtype wrappers on foreign types. There might be an upcoming solution, Lucien has been very welcoming in discussing improvements.