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

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

Closed
wants to merge 1 commit into from

Conversation

clayjohn
Copy link
Member

Fixes: #74656

There were two issues here:

  1. The AABB calculation code wasn't properly checking for invalid bones (this affected both 2D and 3D) so the default unused bones slipped through with size -1, -1, -1 and a position of 0, 0, 0. This made the AABB stretch back to the item's origin. The fix was to check if the bone size is -1, -1, -1 and to check each bone instead of just checking the first bone in the array.
  2. The transform was in the wrong space. This fix is the same as Automatically transform Skeleton2D calculations so pivots are not needed #72214. We need to transform the bone into local space before applying the bone transform

This PR also forward ports the debug rect code from #75612 to properly validate that the changes were made correctly.

I tested using the MRP from #74656 (comment). But I modified it to use the debugging rects from the MRP in #75612
polygon2dcullbug-with-debug.zip

Before merging I would like to test this further in other cases. Perhaps @SlugFiller can confirm this fixes their original issue as well

Before this PR:
Screenshot from 2023-04-17 14-21-20

After this PR
Screenshot from 2023-04-17 14-15-18

@clayjohn clayjohn added this to the 4.1 milestone Apr 17, 2023
@clayjohn clayjohn requested a review from lawnjelly April 17, 2023 21:35
@clayjohn clayjohn requested a review from a team as a code owner April 17, 2023 21:35
@SlugFiller
Copy link
Contributor

image

Works if you move the element, but does not work if you move the element's parent, i.e. change the element's global transform.

Here, you can use my MRP:
reproduce_canvasgroup_rotate.zip

Just pick Node2D/Node2D or Node2D/Polygon2D3 or Node2D/CanvasGroup2 and use the Move Mode tool in the editor, and just drag around. Notice the slightly different behavior of each.

@lawnjelly
Copy link
Member

lawnjelly commented Apr 18, 2023

If it is anything like in 3.x, the skeleton base transform is in global space, and I have a feeling the transform you are passing to the function for the mesh may be in local space, whereas afaik you may need it in global space to calculate the relative xform. This could be why it works with no global space offset, but breaks as soon as the polygon is moved around outside of local space.

In 3.x at least, all the canvas item xforms are in local space. They are only concatenated to "final space" during the render. Final space is a combination of global space and the camera transform (I'm not at all sure this makes things simpler, versus just calculating final space from global space in the shader, but that's how it currently works).

This meant getting the Polygon2D in global space was actually surprisingly tricky, I ended up calculating it scene side during Polygon2D::_skeleton_bone_setup_changed() as a one off. But the good news is that providing you aren't moving the skeleton and the Polygon2D relative to each other, it just works.

I haven't examined master in detail to know whether this system is the same or not.

@clayjohn clayjohn requested a review from a team as a code owner April 18, 2023 16:38
@lawnjelly
Copy link
Member

Just to confirm this is still broken with the latest commit, if you load the example project "polygon2dcullbug" and move Human from 0,0 to 500,0 the bounds are incorrect.

@clayjohn
Copy link
Member Author

Just to confirm this is still broken with the latest commit, if you load the example project "polygon2dcullbug" and move Human from 0,0 to 500,0 the bounds are incorrect.

That's right. More work will need to be done to properly expose the "final" transform of the Node. Right now we only have access to the Node's transform, not its parent. The trouble is the "final" transform bakes in the viewport transform, which we don't want. For updating the skeleton, we have to extract the "final" transform just before updating. We will have to do something similar here as well

@fire
Copy link
Member

fire commented May 16, 2023

@lyuma Do you think you can review this for fixing the broken lod issue?

@fire
Copy link
Member

fire commented May 16, 2023

Clayjohn said that the 3d case should be extracted into its own PR as we didn't fix the issue with Skeleton2D so we haven't merged this yet.

@lawnjelly
Copy link
Member

lawnjelly commented Jun 14, 2023

Started a little of trying to port #75612 this morning to master.

However I didn't realise just how much master has diverged from 3.x. Skinned Polygon2Ds are now no longer stored in canvas polygons, they are stored in canvas meshes, so the best method of getting access to the points and bones and weights in order to make the calculations required is not super clear. It would probably take a while to familiarize myself enough with master to make sensible decisions on this.

It's also some time since I did the original PR so it's not super fresh in memory if there might be an alternate way of doing this in master.

@clayjohn
Copy link
Member Author

clayjohn commented Nov 7, 2023

Superseded by #84451

@clayjohn clayjohn closed this Nov 7, 2023
@clayjohn clayjohn removed this from the 4.x milestone Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mismatch between AABB and mesh when mesh and skeleton don't have the same transform
5 participants