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

3D selection box transforms incorrectly if object is being moved by PhysicsServer when made active in the editor #86861

Closed
ryevdokimov opened this issue Jan 6, 2024 · 5 comments · Fixed by #87132

Comments

@ryevdokimov
Copy link
Contributor

Tested versions

Godot v4.2.1.stable.mono

System information

Windows 10.0.22631 - Vulkan (Forward+) - integrated AMD Radeon(TM) Graphics (Advanced Micro Devices, Inc.; 31.0.23013.1023) - AMD Ryzen 7 PRO 6850U with Radeon Graphics (16 Threads)

Issue description

Title.

Conditions:

Collision and Mesh shape are just boxes.

image

Top level transform is enabled on the RigidBody:

image

Parent Node is setup to "follow" the RigidBody

image

Result:

bug

Steps to reproduce

Open node_3d.tscn
Select RigidBody3D
Unfreeze
Select Parent Node3D
Observe Selection Box

Minimal reproduction project (MRP)

bug.zip

@ryevdokimov
Copy link
Contributor Author

ryevdokimov commented Jan 9, 2024

Scale is affected too if script is changed to:

image

image

@Calinou Calinou changed the title 3D selection box transforms incorrect with specific conditions. 3D selection box transforms incorrectly if object is being moved by PhysicsServer when made active in the editor Jan 10, 2024
@Calinou
Copy link
Member

Calinou commented Jan 10, 2024

cc @rcorre

@ryevdokimov
Copy link
Contributor Author

ryevdokimov commented Jan 10, 2024

Thanks for taking a look, just wanted to clarify due to the name change that the rigidbody itself has a correct selection box, it's the parent Node3D that is being programmatically forced to follow the rigidbody transforms that gets wonky. That's why the name was kind of vague. One of those easier to show than explain things.

@cooperra
Copy link
Contributor

I'm working with @ryevdokimov on a fix for this.

ryevdokimov pushed a commit to Open-Industry-Project/godot that referenced this issue Jan 12, 2024
…awing selection box

Previously, children with `top_level=true` were bounded as if they were `top_level=false`. This led to situations where selection boxes were drawn incorrectly. The box would appear offset from the object it was meant to render onto.

`Node3DEditorViewport::_calculate_spatial_bounds` traverses the scene tree to calculate bounds in a bottom-up fashion. Children boxes are transformed relative to their parents until we reach the top-level parent (the one the function was initially called on). The final return value is a bounding box in the top-level parent's local space.

On [line 2812][] in the caller, the top-level node's transform is applied to get the final selection box in global space.

This fix applies the inverse of the parent's global transform to children with `top_level=true`. This counteracts the transforms we apply while traversing the scene tree. It also counteracts the caller's final transform on [line 2812][].

[line 2812]: https://github.com/godotengine/godot/blob/9b522ac1a85cab1a7a867b7a9f3bb102d9376ac2/editor/plugins/node_3d_editor_plugin.cpp#L2812
ryevdokimov added a commit to Open-Industry-Project/godot that referenced this issue Jan 12, 2024
Fix godotengine#86861: Respect top_level property of children when drawing selection box
ryevdokimov pushed a commit to Open-Industry-Project/godot that referenced this issue Jan 12, 2024
…awing selection box

Previously, children with `top_level=true` were bounded as if they were `top_level=false`. This led to situations where selection boxes were drawn incorrectly. The box would appear offset from the object it was meant to render onto.

`Node3DEditorViewport::_calculate_spatial_bounds` traverses the scene tree to calculate bounds in a bottom-up fashion. Children boxes are transformed relative to their parents until we reach the top-level parent (the one the function was initially called on). The final return value is a bounding box in the top-level parent's local space.

On [line 2812][] in the caller, the top-level node's transform is applied to get the final selection box in global space.

This fix applies the inverse of the parent's global transform to children with `top_level=true`. This counteracts the transforms we apply while traversing the scene tree. It also counteracts the caller's final transform on [line 2812][].

[line 2812]: https://github.com/godotengine/godot/blob/9b522ac1a85cab1a7a867b7a9f3bb102d9376ac2/editor/plugins/node_3d_editor_plugin.cpp#L2812
@ryevdokimov
Copy link
Contributor Author

This is confirmed nothing to do with the PhysicsServer, and simply how AABB bounds are calculated when using top-level nodes.

cooperra added a commit to cooperra/godot that referenced this issue Jan 12, 2024
Each time an AABB is rotated, it gets bigger. That means opposite rotations don't cancel out.

The previous implementation repeatedly rotates children AABBs as it climbs up the tree. This often resulted in selection boxes looking bigger than their contents.

This implementation calculates and applies a single final transformation to each AABB before it is merged with the others. After merging, there are no additional rotations, so AABBs remain accurate.

Also fixes godotengine#86861, because `get_global_transform` implicitly respects `top_level`.

Another note: The previous implementation only boxed visual instances. Non-visual nodes, such as basic Node3Ds were left out, substituting the bounds of their children, if any. This one boxes all nodes. The positions of all non-visual nodes are contained in the final AABB.
@akien-mga akien-mga added this to the 4.3 milestone Feb 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment