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

Fox.glb has incorrectly scaled normals #8099

Closed
NotAFile opened this issue Mar 15, 2023 · 11 comments
Closed

Fox.glb has incorrectly scaled normals #8099

NotAFile opened this issue Mar 15, 2023 · 11 comments
Labels
A-Assets Load files from disk to use for things like images, models, and sounds A-glTF Related to the glTF 3D scene/model format A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior C-Examples An addition or correction to our examples D-Trivial Nice and easy! A great choice to get started with Bevy

Comments

@NotAFile
Copy link
Contributor

Bevy version

35e107c

What went wrong

The Fox.glb model has incorrect scaling of the vertex normals.

e.g. for the first vertex, the normals are:

0.176476 -0.507666 -0.242626

correctly scaled would be:

0.299268 -0.860901 -0.411446

This has led to issues such as #6528. That has been fixed, but it would be good to fix this at the source anyway.

@NotAFile NotAFile added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Mar 15, 2023
@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Examples An addition or correction to our examples A-Assets Load files from disk to use for things like images, models, and sounds D-Trivial Nice and easy! A great choice to get started with Bevy and removed S-Needs-Triage This issue needs to be labelled labels Mar 15, 2023
@alecgarza96
Copy link

New to Bevy, would love to help out. Still learning the codebase

Does this change involve edits to the 3D model itself? I looked under examples/animated_fox.rs and didn't find any code performing scaling.

I've been searching under crates/bevy_render as well for where this error could be originating

@james7132
Copy link
Member

Does this change involve edits to the 3D model itself? I looked under examples/animated_fox.rs and didn't find any code performing scaling.

Looks like it. There's no normalization code we do on any meshes by default. IMO we should any or all of the following:

  • Update the 3D model itself to include normalized normals
  • Add a validation step when loading meshes to ensure normals are normalized.
  • Add a asset preprocessing step to do this when that pipeline is ready (contingent on the asset rework @cart's working on)

@alecgarza96
Copy link

Does this change involve edits to the 3D model itself? I looked under examples/animated_fox.rs and didn't find any code performing scaling.

Looks like it. There's no normalization code we do on any meshes by default. IMO we should any or all of the following:

  • Update the 3D model itself to include normalized normals
  • Add a validation step when loading meshes to ensure normals are normalized.
  • Add a asset preprocessing step to do this when that pipeline is ready (contingent on the asset rework @cart's working on)

Looking into adding the validation step for when meshes are normalized

I found the compute_flat_normals method which utilizes the face_normal

would it be possible to reuse this functionality for the validation check, or do we want to calculate the standard normals instead of flat?

Also, at what point should the validation check occur? My initial thoughts are in either ::new() or inert_attribute()

@JohnTheCoolingFan
Copy link
Contributor

While you are figuring out validation, I've been trying to fix the model itself. I've loaded it into blender, recalculated normals (removed duplicate vertices, recalculated, disconnected faces back, export). Visually nothing changed (judging by blender's normal visualization), I tried it on commit specified in #6528 and it's still black. I've uploaded my updated model to a fork: https://github.com/JohnTheCoolingFan/bevy/blob/dfdda55edb9e1ac6f6d497259faed32dd300068b/assets/models/animated/Fox.glb

@NotAFile
Copy link
Contributor Author

For reference, the way I personally did the normalization was via MeshLab, which has a "normalize normals" option. I unfortunately could not get to output a drop in replacement .glb file though.

@mockersf
Copy link
Member

mockersf commented Apr 13, 2023

I would prefer to not change the model at all. It should come straight from https://github.com/KhronosGroup/glTF-Sample-Models/tree/master/2.0/Fox without any modification. If it's wrong and need updating, could you submit your fix on the gltf sample repo?

@nicopap
Copy link
Contributor

nicopap commented Apr 13, 2023

I believe one of the aim of glTF is avoiding the kind of pitfall where everything needs to be sanitized and recomputed, it's "GPU ready". The glTF standard unambiguously says the normal attributes are normalized so if the model doesn't have normalized normals, it's probably in infraction of the standard.

@mockersf
Copy link
Member

If the wrong normals are confirmed on the upstream fox, fixing them would be very nice. If not, we can update ours with the original 👍

@NotAFile
Copy link
Contributor Author

There's been a commit about "fixing validation errors" upstream that's newer than the in-tree copy, perhaps pulling that in would actually already be enough.

@alice-i-cecile alice-i-cecile added the A-glTF Related to the glTF 3D scene/model format label Sep 26, 2024
@rparrett
Copy link
Contributor

Am I crazy or does Fox.glb not contain normals? I looked at both the current version of Fox.glb in our repo, and the latest version in glTF-Sample-Models.

Image

@rparrett
Copy link
Contributor

It seems that this was fixed at some point. Maybe there was an issue with computing normals?

On the latest main, I added the following to animated_mesh to peek at the first normal:

fn show_first_normal(query: Query<(Entity, &Mesh3d)>, assets: Res<Assets<Mesh>>) {
    for (entity, mesh3d) in &query {
        let mesh_asset = assets.get(&mesh3d.0).unwrap();
        if let Some(VertexAttributeValues::Float32x3(normals)) =
            mesh_asset.attribute(Mesh::ATTRIBUTE_NORMAL)
        {
            info!("{} first normal: {:?}", entity, normals[0]);
        }
    }
}

And got:

2025-02-11T14:35:04.298302Z  INFO animated_mesh: 38v1 first normal: [0.29926783, -0.8609013, -0.41144586]

@alice-i-cecile alice-i-cecile closed this as not planned Won't fix, can't repro, duplicate, stale Feb 11, 2025
github-merge-queue bot pushed a commit that referenced this issue Feb 11, 2025
# Objective

Fix gltf validation errors in `Fox.glb`.

Inspired by #8099, but that issue doesn't appear to describe a real bug
to fix, as far as I can tell.

## Solution

Use the latest version of the Fox from
[glTF-Sample-Assets](https://github.com/KhronosGroup/glTF-Sample-Assets/blob/main/Models/Fox/glTF-Binary/Fox.glb).

## Testing

Dropped both versions in https://github.khronos.org/glTF-Validator/

`cargo run --example animated_mesh` seems to still look fine.

Before:

```
The asset contains errors.
"numErrors": 126,
"numWarnings": 4184,
```

After:

```
The asset is valid.
"numErrors": 0,
"numWarnings": 0,
```

## Discussion

The 3d testbed was panicking with
```
thread 'main' panicked at examples/testbed/3d.rs:288:60:
called `Result::unwrap()` on an `Err` value: QueryDoesNotMatch(35v1 with components Transform, GlobalTransform, Visibility, InheritedVisibility, ViewVisibility, ChildOf, Children, Name)
```
Which is bizarre. I think this might be related to #17720, or maybe the
structure of the gltf changed.

I fixed it by using updating the testbed to use a more robust method of
finding the correct entity as is done in `animated_mesh`.
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-glTF Related to the glTF 3D scene/model format A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior C-Examples An addition or correction to our examples D-Trivial Nice and easy! A great choice to get started with Bevy
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants