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

GLTF export: Use sparse accessors for morph targets #89356

Merged
merged 2 commits into from
Mar 14, 2024

Conversation

lyuma
Copy link
Contributor

@lyuma lyuma commented Mar 10, 2024

Implements sparse for vec3 vertex position, normal and tangent buffers within morph targets (blend shapes).

Reduces buffer sizes on some blendshape-heavy exported gltf files by 50%

The exported size of this file was reduced from 18MB down to 12MB:
https://hub.vroid.com/en/characters/673873032957715085/models/7956797656633477494

Fixes: godotengine/godot-proposals#9276


After making this change, I determined that Godot's morph normal and tangent exports were completely incorrect, and hence that we were using the incorrect reference buffer: because morph targets are relative to zero, it is appropriate to compare to a zero-initialized buffer (undefined bufferView).

As it is a separate change, I have included this bugfix as a separate commit.

As a testcase, I activated 4 blend shapes in at once a blender exported gltf to make the effect extremely obvious:

Current Godot 4.x gltf export: you will notice the mouth is more grey and some wrinkles around the mouth are less prominent:
godot_blendshape_normals_export_previous
Fue to blendshapes effectively including the sum of the original normals and the new normals, it produces roughly an average, so the normal deltas are about half what they should be

Bug-fixed gltf export:
godot_blendshape_normals_export_new

Original gltf from blender: should look identical to the bug-fixed version:
godot_blendshape_normals_blender_reference

@lyuma lyuma requested a review from a team as a code owner March 10, 2024 13:05
@lyuma lyuma force-pushed the vsk-gltf-sparse-accessors-4.3 branch from c6c0c80 to 3167e19 Compare March 10, 2024 13:08
@fire
Copy link
Member

fire commented Mar 10, 2024

Dependent on #89352

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will take https://sketchfab.com/3d-models/shibahu-2e9dffd948f747668609a5a477daad86 and test if the blend shape sizes are reduced.

@akien-mga akien-mga changed the title gltf export: Use sparse accessors for morph targets GLTF export: Use sparse accessors for morph targets Mar 10, 2024
@lyuma lyuma force-pushed the vsk-gltf-sparse-accessors-4.3 branch from 3167e19 to e32e48b Compare March 11, 2024 07:47
Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please resolve the commentary items and I think it's good to go.

modules/gltf/gltf_document.cpp Outdated Show resolved Hide resolved
modules/gltf/gltf_document.cpp Outdated Show resolved Hide resolved
modules/gltf/gltf_document.cpp Outdated Show resolved Hide resolved
@lyuma lyuma force-pushed the vsk-gltf-sparse-accessors-4.3 branch from e32e48b to db2c957 Compare March 12, 2024 09:39
@lyuma lyuma requested a review from fire March 12, 2024 09:53
@lyuma
Copy link
Contributor Author

lyuma commented Mar 12, 2024

Upon debugging why exported blend shapes were still far larger than expected despite the sparse buffers, I determined that the blend shape handling during export was entirely incorrect for normals and tangents, and the sparse code was handling vertex positions incorrectly. This bug affects existing versions of Godot, so it can be viewed as a separate bugfix. Therefore, I made it a separate commit.

I also adjusted the is_zero_approx factor for blend shape normals and tangents to account for normals and tangents requiring lower precision than vertex positions: 0.0001 should be sufficient.

Due to a lot of code change to handling of blend shape exports, I am requesting another review,

Commentary (broadly speaking, not really related to this PR): Whether these blend shape normals are even desirable or correct on most models is an entirely different discussion. Blend shape normals seem highly distrusted by most toon shaded model authors, with the official VRM exporter excluding them entirely. fbx files (as well as FBX2glTF converted files) seem to often have zero deltas on blend shape normals.

Godot really needs the ability to calculate correct blend shape normals in this case, at least on fbx files: I'm pretty sure there's a reason that Unity re-calculates blend shape normals by default.

In fact, it seems rare to find models with deliberately authored blend shape normals, so it seems odd to build the rendering pipeline in such a way that requires not only blend shape normals to be defined, but also blend shape tangents, and for all vertices. This requirement is new as of Godot 4.2, and I consider it a regression especially for toon shaded characters which often do not use normals maps and should not require tangent vectors at all.

I hope that Godot's rendering team considers an overhaul of storage of blend shape normals and tangents to be more memory efficient, which would probably also be more GPU efficient in terms of how the compute shader might be able to iterate over the sparse indices instead of all vertices on the mesh.

@lyuma
Copy link
Contributor Author

lyuma commented Mar 14, 2024

I was asked for a better explanation of the change I made to (normalized blend shape mode) normal and tangent export.

The current behavior (4.0-4.2) of Godot's gltf export is incorrect: it converts normalized vertex blend shapes correctly to relative by subtracting, but it does not perform the subtraction for normals and tangents. I needed to fix this in order for the sparse accessors to work, since they would have been otherwise referencing the wrong buffer

For example, let's take a mesh with one vertex and one blend shape. Say a blend shape moves a vertex left by 0.5, from (3.0,0,0) to (2.5,0,0), and the normal changes from (0,0,1) to (0.05, 0, 0.99)

Godot's base mesh will have: vertices: (3,0,0); normals: (0,0,1)
Godot's blend shape will have: vertices: (2.5,0,0); normals: (0.05, 0, 0.99)

However, glTF stores in relative, so glTF export will have the same base vertices, but glTF blend shape should have: vertex (-0.5,0,0); normal: (0.05, 0, -0.01)

The bug is Godot's current gltf exporter is outputting blend shape vertex: (-0.5,0,0); normal: (0.05, 0, 0.99)

the effect of this is not quite as bad as it appears, and this is what surprised me when trying to understand how we didn't notice the bug. What happens is the normal is applied as relative during import. this will calculate the following:
normal = (0,0,1) + (0.05, 0, 0.99) = (0.05,0,1.99) ... This result will be normalized, roughly: (0.025,0,0.995) or something

In effect, due to the bug I am fixing here, exported glTF normals were almost exactly half as pronounced as they should be, and that can be difficult to notice, when it only applies to blend shapes, and only blend shapes that affect the normal, and only when using standard shading with enough contrast in the lighting.

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a rewrite of the explanation if it's easier to understand.

We have identified a bug in Godot's GLTF exporter. The issue is with the output of blend shape vertex and normal, as well as the subsequent calculations during import. Here's an illustration of the problem:

# Output
blend shape vertex: (-0.5,0,0)
normal: (0.05, 0, 0.99)

# During import
normal = (0,0,1) + (0.05, 0, 0.99) = (0.05,0,1.99)

# After normalization
result = (0.025,0,0.995)

The result of this process is that the normal appears less pronounced than it should be. This effect is subtle and can be difficult to detect, as it only applies to certain blend shapes under specific lighting conditions. We need to address this bug to ensure accurate representation of normals in all scenarios.

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Mar 14, 2024
@akien-mga akien-mga merged commit 37b08a3 into godotengine:master Mar 14, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@fire fire deleted the vsk-gltf-sparse-accessors-4.3 branch March 14, 2024 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

Fully implement sparse accessors in gltf
4 participants