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

Use BLEND_SHAPE_MODE_RELATIVE for GLTF blend shape #60814

Closed

Conversation

WindyDarian
Copy link
Contributor

@WindyDarian WindyDarian commented May 6, 2022

Effectively undo #24647 (and #20377 it was fixing is no longer an issue since vertex weight in skeleton is now separate) and fixes #55812 where the total weight might not be normalized as expected under BLEND_SHAPE_MODE_NORMALIZED.

Note the behavior for blending multiple blend shapes will change from "average" to "additive".

Tested with the models from both #20377 and #55812 and both seems to behave correctly

Note this doesn't fix normal behaving weird when using BLEND_SHAPE_MODE_NORMALIZED (which my guess is final total weight not normalized to 1 before sending to renderer?), but instead just use BLEND_SHAPE_MODE_RELATIVE that now behaves correctly

@WindyDarian WindyDarian requested a review from a team as a code owner May 6, 2022 03:33
@WindyDarian WindyDarian marked this pull request as draft May 6, 2022 04:48
@WindyDarian
Copy link
Contributor Author

(Converted to draft because this doesn't work well with blend_surface_tool->generate_tangents(), which expects full data instead of offset)

@WindyDarian
Copy link
Contributor Author

WindyDarian commented May 6, 2022

Pushed a new change stores as blend shape as full value before generate_tangents() and subtracts base values after tangent space generation. Still work for #20377 and #55812 test cases and also made sure #19346 's test case doesnt regress

Effectively undo godotengine#24647 (and godotengine#20377 it was fixing is no longer a issue since vertex weight in skeleton is now separate) and fixes godotengine#55812 where the issue might total weight not normalized as expected under BLEND_SHAPE_MODE_NORMALIZED.

Note the behavior for blending multiple blend shapes will change from "average" to "additive".
@Chaosus Chaosus added this to the 4.0 milestone May 6, 2022
@Chaosus Chaosus changed the title Use BLEND_SHAPE_MODE_RELATIVE for gltf blend shape, fixes #55812 Use BLEND_SHAPE_MODE_RELATIVE for GLTF blend shape May 6, 2022
@WindyDarian WindyDarian marked this pull request as ready for review May 6, 2022 05:39
@fire
Copy link
Member

fire commented May 6, 2022

Pending review.

@akien-mga
Copy link
Member

Effectively undo #24647 (and #20377 it was fixing is no longer an issue since vertex weight in skeleton is now separate) and fixes #55812 where the total weight might not be normalized as expected under BLEND_SHAPE_MODE_NORMALIZED.

I see that #24647 added some code which is still present in the current importer, and this new PR is adding more code to do conversions later on. I'm not familiar with that code but just wanted to check that this is intended and not doing conversions twice back and forth.

@WindyDarian
Copy link
Contributor Author

WindyDarian commented May 10, 2022

I see that #24647 added some code which is still present in the current importer, and this new PR is adding more code to do conversions later on. I'm not familiar with that code but just wanted to check that this is intended and not doing conversions twice back and forth.

Yes, it is intended. My previous change actually just undid #24647 (254314a), so it made the blend shape store delta in the first place again. However, I found that would regress #19346 , and after a bit debugging i found the mikktspace generation code wants full positions and normals to be able to generate tangents correctly, otherwise it would get stuck by the mesh in #19346. So I made it store full vertices first, then after generating tangent space, substract original values to get offset again with new generated tangent space. This works for the mesh in #19346. Also had a comment in code about this.

@reduz
Copy link
Member

reduz commented May 18, 2022

I am not sure what is going on here, but I see no reason to store these as relative, since tangent and bitangent would not be stored properly.

@WindyDarian
Copy link
Contributor Author

WindyDarian commented May 18, 2022

Hi @reduz , can you elaborate on how tangent and bitangent is wrong?

This change will store the tangent as offset after generating tangent space (and i looked and found that flip component (w) isn't used for blend shape blending in shaders). And bitangent is recalculated from normal and tangent in shader after blending. (Though i know this is not mathematically precise unless we regenerate tangent space after blending, it is good enough in my opinion, and what happened before with storing whole vertices in blend shapes and use RELATIVE was not accurate either after blending)

The whole reason I am changing it back to BLEND_SHAPE_MODE_RELATIVE (and store relative positions, normals, and tangents since it just adds blend shape positions, normals and tangents together in skeleton.glsl, which assumes blend shape vertices store offsets) is because BLEND_SHAPE_MODE_NORMALIZED behavior seems to have changed in 4.0 and caused #55812 , changing it back to BLEND_SHAPE_MODE_RELATIVE seems to be the lowest impact fix - plus it was my change at #24647 made it NORMALIZED and store whole verts, so i think it should be safe unless there are new thing depending on the behavior after 3.1

@WindyDarian
Copy link
Contributor Author

WindyDarian commented May 20, 2022

A lightning stroke me in the dream and i suddenly understood the blend shape math. #61217 is the actual fix.

So @reduz was right, tangent is wrong.

So BLEND_SHAPE_MODE_NORMALIZED and BLEND_SHAPE_MODE_RELATIVE actually do the same thing when math is fixed, just differ in how blend shape is stored

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants