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

[Merged by Bors] - Fix loading non-TriangleList meshes without normals in gltf loader #4376

Closed
wants to merge 3 commits into from

Conversation

tim-blackbird
Copy link
Contributor

@tim-blackbird tim-blackbird commented Mar 31, 2022

Objective

Make it so that loading in a mesh without normals that is not a TriangleList succeeds.

Solution

Flat normals can only be calculated on a mesh made of triangles.
Check whether the mesh is a TriangleList before trying to compute missing normals.

Additional changes

The panic condition in duplicate_vertices did not make sense to me. I moved it to compute_flat_normals where the algorithm would produce incorrect results if the mesh is not a TriangleList.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Mar 31, 2022
@alice-i-cecile alice-i-cecile added A-Assets Load files from disk to use for things like images, models, and sounds C-Bug An unexpected or incorrect behavior and removed S-Needs-Triage This issue needs to be labelled labels Mar 31, 2022
@cart
Copy link
Member

cart commented Mar 31, 2022

Makes sense to me!

@cart
Copy link
Member

cart commented Mar 31, 2022

bors r+

@cart
Copy link
Member

cart commented Mar 31, 2022

bors r-

@bors
Copy link
Contributor

bors bot commented Mar 31, 2022

Canceled.

@cart cart changed the title Fix gltf loader Fix loading non-TriangleList meshes without normals in gltf loader Mar 31, 2022
@cart
Copy link
Member

cart commented Mar 31, 2022

bors r+

@tim-blackbird
Copy link
Contributor Author

Thanks Cart. I did forget to give this a proper title.

bors bot pushed a commit that referenced this pull request Mar 31, 2022
…4376)

# Objective
Make it so that loading in a mesh without normals that is not a `TriangleList` succeeds.

## Solution
Flat normals can only be calculated on a mesh made of triangles.
Check whether the mesh is a `TriangleList` before trying to compute missing normals.

## Additional changes
The panic condition in `duplicate_vertices` did not make sense to me. I moved it to `compute_flat_normals` where the algorithm would produce incorrect results if the mesh is not a `TriangleList`.

Co-authored-by: devil-ira <[email protected]>
@bors bors bot changed the title Fix loading non-TriangleList meshes without normals in gltf loader [Merged by Bors] - Fix loading non-TriangleList meshes without normals in gltf loader Mar 31, 2022
@bors bors bot closed this Mar 31, 2022
aevyrie pushed a commit to aevyrie/bevy that referenced this pull request Jun 7, 2022
…evyengine#4376)

# Objective
Make it so that loading in a mesh without normals that is not a `TriangleList` succeeds.

## Solution
Flat normals can only be calculated on a mesh made of triangles.
Check whether the mesh is a `TriangleList` before trying to compute missing normals.

## Additional changes
The panic condition in `duplicate_vertices` did not make sense to me. I moved it to `compute_flat_normals` where the algorithm would produce incorrect results if the mesh is not a `TriangleList`.

Co-authored-by: devil-ira <[email protected]>
@tim-blackbird tim-blackbird deleted the fix_gltf_loader branch November 8, 2022 21:52
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
…evyengine#4376)

# Objective
Make it so that loading in a mesh without normals that is not a `TriangleList` succeeds.

## Solution
Flat normals can only be calculated on a mesh made of triangles.
Check whether the mesh is a `TriangleList` before trying to compute missing normals.

## Additional changes
The panic condition in `duplicate_vertices` did not make sense to me. I moved it to `compute_flat_normals` where the algorithm would produce incorrect results if the mesh is not a `TriangleList`.

Co-authored-by: devil-ira <[email protected]>
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 C-Bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants