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

fix normals computation for gltf #13396

Merged

Conversation

mockersf
Copy link
Member

Objective

thread 'IO Task Pool (2)' panicked at crates/bevy_render/src/mesh/mesh/mod.rs:581:9:
`compute_flat_normals` can't work on indexed geometry. Consider calling either `Mesh::compute_smooth_normals` or `Mesh::duplicate_vertices` followed by `Mesh::compute_flat_normals`.
  • test with example custom_gltf_vertex_attribute or gltf_skinned_mesh

Solution

  • Call the wrapper function for normals that will either call compute_flat_normals or compute_smooth_normals as appropriate

Testing

  • Ran the two examples mentioned above

@mockersf mockersf added A-Rendering Drawing game state to the screen A-Assets Load files from disk to use for things like images, models, and sounds labels May 16, 2024
@mockersf mockersf added this to the 0.14 milestone May 16, 2024
@alice-i-cecile
Copy link
Member

@adithramachandran can I get your review here?

@alice-i-cecile alice-i-cecile added D-Trivial Nice and easy! A great choice to get started with Bevy S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it C-Bug An unexpected or incorrect behavior labels May 16, 2024
@IceSentry
Copy link
Contributor

IceSentry commented May 16, 2024

Technically the current behaviour is the 0.13 behaviour. Are those examples new since 0.13?

I'm still not a huge fan of normals not being computed the same based on the vertex layout without any warnings to the user, but I guess that's not a big issue.

@rparrett
Copy link
Contributor

Are those examples new since 0.13?

gltf_skinned_mesh has been around since Bevy 0.7, custom_gltf_vertex_attribute since Bevy 0.11.

@IceSentry
Copy link
Contributor

Oh, I know what's going on. In 0.13 it was just always calling duplicate_meshes and it was removed in the PR that computed smooth normals inside compute_flat_normals.

I feel like it would be better to use that instead of now not always computing flat normals.

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels May 17, 2024
@mockersf mockersf added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels May 18, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 18, 2024
Merged via the queue into bevyengine:main with commit a55e0e3 May 18, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Assets Load files from disk to use for things like images, models, and sounds A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior D-Trivial Nice and easy! A great choice to get started with Bevy S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants