[Merged by Bors] - calculate flat normals for mesh if missing #1808
Conversation
alice-i-cecile
added
A-Assets
|
A future optimization might be to have a flat shading |
|
@jakobhellermann I think it makes sense to copy the discord message with example numbers, just to give some context. |
When forcing the flight helmet to calculate normals (and thus to duplicate the vertices), the combined vertex buffer length for all meshes grows from The actual examples of models in the glTF Sample Models are all very small: ExpandOld length: 10 Old length: 4 Old length: 4 Old length: 4 Old length: 4 Old length: 4 Old length: 4 Old length: 4 Old length: 4 Old length: 4 Old length: 3 |
|
Another alternative proposed by Links: |
You did mention one other interesting thing: Although the vertex buffer length grows significantly, the index buffer is eliminated. Still not a good trade off, given there's likely more properties per vertex, but it helps. |
also panic in compute_flat_normals if indices are set. That way there won't be no unexpected vertex buffer size increases.
jakobhellermann
force-pushed
the
gltf-generate-normals
branch
from
a83750b
to
5ca2020
Compare
| /// | ||
| /// Panics if [`Indices`] are set. | ||
| /// Consider calling [Mesh::duplicate_vertices] or export your mesh with normal attributes. | ||
| pub fn compute_flat_normals(&mut self) { |
There was a problem hiding this comment.
Normals should also be normalized/unit length.
There was a problem hiding this comment.
Right, normals should be actually normal 😄
crates/bevy_gltf/src/loader.rs
Outdated
| if vertex_count_before != vertex_count_after { | ||
| bevy_log::warn!("Missing vertex normals in indexed geometry, computing them as flat. Vertex count increased from {} to {}", vertex_count_before, vertex_count_after); | ||
| } |
There was a problem hiding this comment.
we should log something even if there wasn't a vertex count change
crates/bevy_gltf/src/loader.rs
Outdated
| let vertex_count_after = mesh.count_vertices(); | ||
|
|
||
| if vertex_count_before != vertex_count_after { | ||
| bevy_log::warn!("Missing vertex normals in indexed geometry, computing them as flat. Vertex count increased from {} to {}", vertex_count_before, vertex_count_after); |
There was a problem hiding this comment.
Calculating normals is a "feature" users might want to use. Warnings must be actionable, but users might not have the ability or inclination to add normals to a model (ex: they bought it on an asset store).
Additionally, some meshes / shaders, might not need normals, and calculating them could be "wasted effort". This is also true for #1803.
I think ideally we detect these cases only when they are relevant (ex: a mesh is missing normals and it is used by a shader that needs normals) and we warn or fail. Then users could somehow opt-in to "mesh transformations" like generating zero-ed out buffers or computing normals. The error/warning could provide a list of ways to resolve the error (ex: fix the mesh manually or opt in to a mesh transformation).
Opting in to these transforms would ideally be a part of the "asset configuration", but bevy_asset currently doesn't support asset config. Distill does (via .meta files), so long term I think thats our best bet.
Its hard to say what we should do in the short term. I'm curious to hear other peoples thoughts. Some ideas:
- Just warn like we do here and accept that there will be un-actionable warnings for some valid use cases (and open an issue calling this out so we dont forget).
- Use something like add a method on
AssetServerto create an asset from an existing one #1665 to allow users to apply the transformation manually to suppress the warning. - Add "asset config" to the current bevy_asset.
|
Okay, this all(#1808, #1803) falls down to the PR I made last year to force attributes to be defined. #870 I made this because of the reasons mentiond in the PR, but seeing all the trouble it causes... 😬 I guess I can try out to re-add the fallback buffer. But this time, missing attributes are defined with the already existing data. This would mean you get garbage vertex data (and I'm not sure how multi platform friendly this is), but at least bevy doesn't crash. Other alternative would be to raise a warning and don't draw the mesh at all/display with error material, Unity style. |
|
it's not exactly the same that providing a fallback buffer. In this case we follow the gltf spec, that is to compute flat normals if missing. In most case it's better to fix/cleanup the data at its source because you can do so with an idea of its meaning/goal rather than in the buffer when it's "just data" still a fallback buffer could make sense to avoid crashing, if that's the target we want for Bevy |
|
Hmm yeah if the gltf spec wants us to automatically calculate flat normals if missing, then I guess we should do that. I don't recall seeing that in the "uber gltf visual png spec doc". Do you have a link to the source of that info? In the general "missing mesh attribute" case, I think I'd prefer a more explicit "mesh fixup" workflow where developers are notified that their mesh is missing something and they then take some action to apply a "fix" (ex: opt in to zeroed out buffers or add it to the mesh source). This "general case" might be a good candidate for an RFC so we can outline the use cases, desired apis, and prior art. |
@jakobhellermann put the link in the pr description: https://github.com/KhronosGroup/glTF/blob/master/specification/2.0/README.md#meshes: The part about normal is not directly after but it reads:
|
Whoops. Sorry about that 😅 |
|
Given that normal calculation is part of the spec, I think we should probably just change the warning to a debug log and be done with it :) |
It's a |
crates/bevy_gltf/src/loader.rs
Outdated
| if vertex_count_before != vertex_count_after { | ||
| bevy_log::debug!("Missing vertex normals in indexed geometry, computing them as flat. Vertex count increased from {} to {}", vertex_count_before, vertex_count_after); | ||
| } | ||
| if vertex_count_before != vertex_count_after { |
There was a problem hiding this comment.
looks like we have dupe checks here
|
bors r+ |
If the gltf loader encounters a mesh without normal attributes, it will duplicate the vertex attributes and compute flat normals, as defined by https://github.com/KhronosGroup/glTF/blob/master/specification/2.0/README.md#meshes: > **Implementation Note**: When normals are not specified, client implementations should calculate flat normals.  Helps with #1802 Co-authored-by: Carter Anderson <mcanders1@gmail.com>
|
Pull request successfully merged into main. Build succeeded: |
bors
bot
changed the title
If the gltf loader encounters a mesh without normal attributes, it will duplicate the vertex attributes and compute flat normals, as defined by https://github.com/KhronosGroup/glTF/blob/master/specification/2.0/README.md#meshes: > **Implementation Note**: When normals are not specified, client implementations should calculate flat normals.  Helps with bevyengine#1802 Co-authored-by: Carter Anderson <mcanders1@gmail.com>
Will apply the flat interpolation qualifier for vertex_normal, this uses the provoking vertex for the whole primitive in the fragment shader, so mostly useful for custom meshes. I made this as an alternative for bevyengine#1808 when for example making procedually generated terrain.
Will apply the flat interpolation qualifier for vertex_normal, this uses the provoking vertex for the whole primitive in the fragment shader, so mostly useful for custom meshes. I made this as an alternative for bevyengine#1808 when for example making procedually generated terrain.
Use the partial derivative (dFdx, dFdy) for computing facet normals in the fragment shader. I made this as an alternative for bevyengine#1808 when for example making procedually generated terrain.
Use the partial derivative (dFdx, dFdy) for computing facet normals in the fragment shader. I made this as an alternative for bevyengine#1808 when for example making procedually generated terrain.
Use the partial derivative (dFdx, dFdy) for computing facet normals in the fragment shader. I made this as an alternative for bevyengine#1808 when for example making procedually generated terrain.
Use the partial derivative (dFdx, dFdy) for computing facet normals in the fragment shader. I made this as an alternative for bevyengine#1808 when for example making procedually generated terrain.
Use the partial derivative (dFdx, dFdy) for computing facet normals in the fragment shader. I made this as an alternative for bevyengine#1808 when for example making procedually generated terrain.
Use the partial derivative (dFdx, dFdy) for computing facet normals in the fragment shader. I made this as an alternative for bevyengine#1808 when for example making procedually generated terrain.
If the gltf loader encounters a mesh without normal attributes, it will duplicate the vertex attributes and compute flat normals, as defined by https://github.com/KhronosGroup/glTF/blob/master/specification/2.0/README.md#meshes:
Helps with #1802