Transform mesh's AABB to skeleton's space when calculating mesh's bounds #84451

ShirenY · 2023-11-04T14:57:46Z

Fixes #74656 fixes #81983 fixes #74132, and there maybe more duplicated reports I havent found, since this bug is quite common.

This PR is following @clayjohn 's pr #76184. Thanks to guys working in this threads, they have actually identified the cause of this issue:
It's because when resolving animated meshes AABBs with skeleton. The AABBs is actually in mesh instance's space. But skeleton animation assumes differently: all the meshes are in skeleton's space.
That's why the PR tried to solve it by introduce the mesh item's xform to mesh_get_aabb. But we quickly found item's xform is not enough. As it's a local transform, it means it will still has the bug when the Polygon2d in a different hierarchy with the skeleton.

So I come up with a solution:
Introduce a new transform stored in Mesh::Surface, which contains the transform differences between the Polygon2d and its skeleton. This transform is generated when polygon recreating the mesh. So that in run time, it can be used to transform the bones' AABBs back and forth.

I believe in 3d we have same issue, since the code is shared same logic. And I didn't address 3d part, because I believe this bug in 3d is quite rare, since people tends to have same root space for skinned meshes and skeletons in 3d. So I guess we can leave it for further PRs.

Plz NOTE that this PR is NOT tring to fix every edge cases, like if you move around polygon2d or skeleton's position in runtime, the AABBs will be incorrect until regenerating. And if people move around Polygon2ds in the editor when bones are not in their rest pos, there will be issues not just rect culling but also mesh rendering. I believe these cases are not really intentional usage. Since move mesh's transform after skinning in 3d is really weird practice.

This is my first PR to godot, so let me know if I messed up anything:)

fire · 2023-11-04T17:11:19Z

There's a notification for "move mesh's transform" on Node3D. NOTIFICATION_TRANSFORM_CHANGED NOTIFICATION_LOCAL_TRANSFORM_CHANGED you can consider if it's a rare operation.

fire

I have some comments on a paper review. Godot uses snake_case for variable names. It looks ok.

Now to run the Godot Engine pull request in a compile.

Tempted to run 500 avatars moving to see what happens -- with the aabb calculations performance wise.

drivers/gles3/storage/mesh_storage.cpp

ShirenY · 2023-11-05T03:34:33Z

There's a notification for "move mesh's transform" on Node3D. NOTIFICATION_TRANSFORM_CHANGED NOTIFICATION_LOCAL_TRANSFORM_CHANGED you can consider if it's a rare operation.

Pushed a new version to address this. I was intend to only do it in the editor, since I assumes the relative transform between mesh and skeleton won't change in runtime.

ShirenY · 2023-11-05T03:47:43Z

Also hope @clayjohn @SlugFiller @lawnjelly can check this out, as this PR follows their previous work.

ShirenY · 2023-11-05T12:26:13Z

Added Engine::get_singleton()->is_editor_hint() to make sure only recreat mesh for transform change event in Editor.

ShirenY · 2023-11-05T14:29:35Z

Oops, looks like swich-case fall through triggered some warnning in linux. Changed the way how it does the editor check.

ShirenY · 2023-11-06T13:27:52Z

As @fire suggested, I made a performance test project based on @Zoomulator's project at #74656
polygon2dcullbug.zip
It spawns 900 skeletons with meshes.
This is what I got from engine at 4.1.3 without the fix. Frame time in range between 90ms - 110ms

This is the profiler with this fix at 4.1.3. Frame time in range between 120ms - 140ms

Looks like this fix do cause some frame time increase. But to be honest, I'm not sure which part causes this result. Because it costs more when culling working correctly: you got more drawcalls to cover. And godot's profiler looks quite rough to me, I can't see time in details. I'll try to do more test on this.

ShirenY · 2023-11-06T13:58:01Z

So I made second performance test, make a new animation, but everything won't get culled in origin code. So both version will have same amount of meshes draw to the viewport.
As I suspected, the performance shows no measurable differences this time.
This is the orign engine test result,

This is the fixed version test result,

And this is the second test project I used
polygon2dcullbug.zip

fire · 2023-11-07T03:25:14Z

@lawnjelly If you have some spare time, can you review this?

lawnjelly · 2023-11-07T07:15:07Z

scene/2d/polygon_2d.cpp

+
+					const Transform2D meshTransform = this->get_global_transform();
+					const Transform2D skeletonTransform = skeleton_node->get_global_transform();
+					const Transform2D mesh_to_sk2d = meshTransform * skeletonTransform.inverse();


It may be better to use affine_inverse() here (that's what I used in 3.x). inverse() just uses the transpose which is cheaper but doesn't work in some situations (I think transpose depends on being orthonormal but I forget).

Ya, transpose is only valid if the matrix is orthonormal. I don't know if this transform is guaranteed to be orthonormal or not. So it's probably safest to use affine_inverse()

lawnjelly · 2023-11-07T07:23:41Z

servers/rendering/renderer_rd/storage_rd/mesh_storage.cpp

 					} else {
 						laabb.merge_with(baabb);
 					}
 				}
 			}

+			if (found_bone_aabb) {
+				// Transform skeleton bounds back to mesh's space if any animated AABB applied.
+				laabb = surface.mesh_to_skeleton_xform.inverse().xform(laabb);


Consider pre-calculating affine inverse. Again affine_inverse preferred over inverse, and as it is a little more expensive to calculate, it can just be stored.

lawnjelly · 2023-11-07T07:33:18Z

This looks great!

It turns out to be simpler than #75612 in 3.x, maybe the bone AABBs were unavailable there so it had to jump through some extra hoops.

EDIT:
Yes looking back at 3.x, the bone AABBs were not available like in 4.x (there is no mesh involved in 3.x), so I pre-calculated the bone bounds and stored them on the Polygon. Aside from this difference, the PRs look to be doing essentially the same thing so this should hopefully work fine in 4.x.

Re: performance, as the author says if you compare scenes that are culling differently you are not comparing like with like. It should be pretty ok from what I can see, but have mentioned pre-calculating affine inverse which might be a tad quicker / more accurate.

I don't know if @clayjohn might have opinions about storing the relative transform on each surface (even when not used), but maybe it can be reused for 3D skinning (no idea if the same problem exists there in 4.x).

I haven't fully tested, but my old test project when I was looking at this seems to work ok and the debug bounds are correct.

lawnjelly

Approve in principle, see if author wants to make changes for affine inverse.

Also could do with a 4.x maintainer looking over to check they are happy with the housekeeping aspects (I'm not so familiar with rendering in 4.x).

ShirenY · 2023-11-07T08:22:05Z

Thanks @lawnjelly for the carefully review.

About affine inverse(I dont know this before) and storing inverse matrix. I'm totally fine with these two change. I didn't code these just because I want this change as small as possible, and I don't think a inverse matrix calculation can make any differences per instance in performance wise :) I mean, there at least will be dozens matrix operation in cpu when you do a skeleton animation.
But I totally understand good programmers want every cpu cycle counts, I do that too. So I will definitely take the affine one.
And for storing inverse matrix, there could be some concern on memory and software engineer aspect. I personally don't want store more data, because it usually means more bugs. But I'm totally open on this, you guys could made better choice on these changes than me who just get to the engine in two weeks.

Talking about performance, there actually is a more intense place than the inverse matrix. Like, is it correct and faster to mutliply the two matrix before apply to the bone AABB? I just haven't been very clear on the math correction with this. Futher more, can we just generate these AABBs in skeleton space, to save the matrix-aabb operation? Maybe, but there will be more related parts about these AABBs to be consider. I would like to leave these consideration in futher PRs, if we think the time worth it.

And again, this PR is more focused on fixing this common issue than optimization as this is not usually the hotspot, I want to keep it as simple and small as possible.

ShirenY · 2023-11-07T14:14:18Z

Oops, Did I mess up anything? The compare looks so wrong to me

AThousandShips · 2023-11-07T14:16:14Z

You did not, you just updated your branch at the same time

Now when applying the remaining changes you can do so with git commit --amend to not create a new commit

ShirenY · 2023-11-07T14:25:20Z

Looks good to me now. Thanks @AThousandShips
Definitely learned something from this PR XD

lawnjelly · 2023-11-07T15:20:18Z

Looks fine now. 👍

clayjohn

Looks good to me too. Great work! And thanks for your dedication in taking this the extra mile

ShirenY · 2023-11-08T14:01:09Z

Thanks for devs' excellent work 💗

ShirenY · 2023-12-06T09:15:37Z

Hi devs, by no means to push, but is there anything need to be down for this PR to be merged?
From my point of view, all the things need to be down in my side have been down. And all stake holders have approved. It's been quite a while, I've seen a lot of PRs later than this one is merged quickly.
@AThousandShips @clayjohn @lawnjelly @fire

lawnjelly · 2023-12-06T09:36:46Z

This needs production team to merge, no need to poke reviewers (we generally don't merge).
Delay may have been due to feature freeze.

I'm sure they list the approved PRs during merging sprees, but just in case pinging @akien-mga .

akien-mga · 2023-12-06T09:51:21Z

It's tracked for merging soon, but right now we focus on PRs that we want to cherrypick for 4.2.1 imminently, and this seems like one that should have some testing in 4.3-dev before we should consider a cherrypick.

scene/2d/polygon_2d.cpp

akien-mga · 2023-12-11T23:35:17Z

Thanks! And congrats for your first merged Godot contribution 🎉

YuriSizov · 2024-01-24T15:11:13Z

Cherry-picked for 4.2.2.

YuriSizov · 2024-01-24T17:57:40Z

Cherry-picked for 4.1.4.

ShirenY requested review from a team as code owners November 4, 2023 14:57

AThousandShips added bug topic:rendering labels Nov 4, 2023

AThousandShips added this to the 4.3 milestone Nov 4, 2023

ShirenY force-pushed the FixSkeletonMeshCulling branch 2 times, most recently from ef5f774 to c1790ad Compare November 4, 2023 15:18

ShirenY changed the title ~~Transform mesh's AABB to skeleton's space when calculate mesh's bounds for fix incorrect rect culling with Polygon2d~~ Transform mesh's AABB to skeleton's space when calculate mesh's bounds for fixing incorrect rect culling with Polygon2d Nov 4, 2023

ShirenY force-pushed the FixSkeletonMeshCulling branch from c1790ad to 61ce482 Compare November 4, 2023 15:55

fire mentioned this pull request Nov 4, 2023

Skeleton unit scaling is awkward V-Sekai/godot-ufbx#20

Closed

fire reviewed Nov 4, 2023

View reviewed changes

drivers/gles3/storage/mesh_storage.cpp Outdated Show resolved Hide resolved

ShirenY force-pushed the FixSkeletonMeshCulling branch from 61ce482 to 041fe40 Compare November 5, 2023 03:30

ShirenY requested a review from fire November 5, 2023 03:48

ShirenY force-pushed the FixSkeletonMeshCulling branch from 041fe40 to 6424ba8 Compare November 5, 2023 12:23

ShirenY force-pushed the FixSkeletonMeshCulling branch from 6424ba8 to 7d1a69d Compare November 5, 2023 14:28

lawnjelly reviewed Nov 7, 2023

View reviewed changes

lawnjelly approved these changes Nov 7, 2023

View reviewed changes

clayjohn mentioned this pull request Nov 7, 2023

Take item and skeleton transforms into account when calculating mesh AABB in 2D #76184

Closed

ShirenY force-pushed the FixSkeletonMeshCulling branch from 6ddeea0 to f0940e7 Compare November 7, 2023 14:10

ShirenY force-pushed the FixSkeletonMeshCulling branch from f0940e7 to 9966a7e Compare November 7, 2023 14:18

clayjohn approved these changes Nov 8, 2023

View reviewed changes

clayjohn added topic:2d cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Nov 8, 2023

AThousandShips approved these changes Nov 8, 2023

View reviewed changes

viksl mentioned this pull request Nov 21, 2023

AABB broken on imported meshes with skeletons. #85171

Open

YuriSizov reviewed Dec 8, 2023

View reviewed changes

scene/2d/polygon_2d.cpp Outdated Show resolved Hide resolved

ShirenY force-pushed the FixSkeletonMeshCulling branch from 9966a7e to a0cd094 Compare December 9, 2023 03:31

ShirenY requested a review from YuriSizov December 9, 2023 03:36

Transform mesh's AABB to skeleton's space when calculate mesh's bounds.

27f71c4

ShirenY force-pushed the FixSkeletonMeshCulling branch from a0cd094 to 27f71c4 Compare December 9, 2023 03:41

akien-mga merged commit 4943b6e into godotengine:master Dec 11, 2023
15 checks passed

ShirenY deleted the FixSkeletonMeshCulling branch December 12, 2023 06:19

kleonc mentioned this pull request Dec 27, 2023

Fix Polygon2D to Skeleton2D transform calculation #86557

Merged

YuriSizov removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Jan 24, 2024

YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jan 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transform mesh's AABB to skeleton's space when calculating mesh's bounds #84451

Transform mesh's AABB to skeleton's space when calculating mesh's bounds #84451

ShirenY commented Nov 4, 2023 •

edited

Loading

fire commented Nov 4, 2023 •

edited

Loading

fire left a comment •

edited

Loading

ShirenY commented Nov 5, 2023

ShirenY commented Nov 5, 2023

ShirenY commented Nov 5, 2023

ShirenY commented Nov 5, 2023

ShirenY commented Nov 6, 2023

ShirenY commented Nov 6, 2023

fire commented Nov 7, 2023

lawnjelly Nov 7, 2023

clayjohn Nov 7, 2023

lawnjelly Nov 7, 2023

lawnjelly commented Nov 7, 2023 •

edited

Loading

lawnjelly left a comment

ShirenY commented Nov 7, 2023 •

edited

Loading

ShirenY commented Nov 7, 2023 •

edited

Loading

AThousandShips commented Nov 7, 2023

ShirenY commented Nov 7, 2023

lawnjelly commented Nov 7, 2023

clayjohn left a comment •

edited

Loading

ShirenY commented Nov 8, 2023

ShirenY commented Dec 6, 2023

lawnjelly commented Dec 6, 2023

akien-mga commented Dec 6, 2023 •

edited

Loading

akien-mga commented Dec 11, 2023

YuriSizov commented Jan 24, 2024

YuriSizov commented Jan 24, 2024

Transform mesh's AABB to skeleton's space when calculating mesh's bounds #84451

Transform mesh's AABB to skeleton's space when calculating mesh's bounds #84451

Conversation

ShirenY commented Nov 4, 2023 • edited Loading

fire commented Nov 4, 2023 • edited Loading

fire left a comment • edited Loading

Choose a reason for hiding this comment

ShirenY commented Nov 5, 2023

ShirenY commented Nov 5, 2023

ShirenY commented Nov 5, 2023

ShirenY commented Nov 5, 2023

ShirenY commented Nov 6, 2023

ShirenY commented Nov 6, 2023

fire commented Nov 7, 2023

lawnjelly Nov 7, 2023

Choose a reason for hiding this comment

clayjohn Nov 7, 2023

Choose a reason for hiding this comment

lawnjelly Nov 7, 2023

Choose a reason for hiding this comment

lawnjelly commented Nov 7, 2023 • edited Loading

lawnjelly left a comment

Choose a reason for hiding this comment

ShirenY commented Nov 7, 2023 • edited Loading

ShirenY commented Nov 7, 2023 • edited Loading

AThousandShips commented Nov 7, 2023

ShirenY commented Nov 7, 2023

lawnjelly commented Nov 7, 2023

clayjohn left a comment • edited Loading

Choose a reason for hiding this comment

ShirenY commented Nov 8, 2023

ShirenY commented Dec 6, 2023

lawnjelly commented Dec 6, 2023

akien-mga commented Dec 6, 2023 • edited Loading

akien-mga commented Dec 11, 2023

YuriSizov commented Jan 24, 2024

YuriSizov commented Jan 24, 2024

ShirenY commented Nov 4, 2023 •

edited

Loading

fire commented Nov 4, 2023 •

edited

Loading

fire left a comment •

edited

Loading

lawnjelly commented Nov 7, 2023 •

edited

Loading

ShirenY commented Nov 7, 2023 •

edited

Loading

ShirenY commented Nov 7, 2023 •

edited

Loading

clayjohn left a comment •

edited

Loading

akien-mga commented Dec 6, 2023 •

edited

Loading