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

[3.x] Make CollisionShape selection box use shape AABB #71320

Merged
merged 1 commit into from
Jan 13, 2023

Conversation

timothyqiu
Copy link
Member

The orange selection box is always shown for Spatial nodes in 3.x.

A CollisionShape is not a VisualInstance so it does not have a AABB itself, and the editor used a fixed fallback AABB. This can produce unexpected selection box for ancestor nodes.

This PR makes use of the shape's AABB. When no shape is assigned, it still falls back to the fixed size AABB.

Before After
before after

@timothyqiu timothyqiu added this to the 3.6 milestone Jan 13, 2023
@timothyqiu timothyqiu requested a review from a team as a code owner January 13, 2023 10:28
Copy link
Member

@lawnjelly lawnjelly left a comment

Choose a reason for hiding this comment

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

Thought I recognised the call and indeed I added get_fallback_gizmo_aabb() for the occluders as there was no way of handling non-visual instances in the editor.

This is a good use imo, agree about the comment about the const_cast and mutable data, how big a bag of worms that would be to do I don't know.

Would this also be worth doing in 4.x? If so, you may need to include the machinery for get_fallback_gizmo_aabb() as it was only included for geometric occluders (which aren't in 4.x), but it's only a few lines if I remember right.

@timothyqiu
Copy link
Member Author

Would this also be worth doing in 4.x? If so, you may need to include the machinery for get_fallback_gizmo_aabb() as it was only included for geometric occluders (which aren't in 4.x), but it's only a few lines if I remember right.

I thought the orange box for non-visual instances was intentionally removed on 4.x 😅 But some nodes do look better without the box, e.g. Camera3D and GridMap. We can probably make the box optional by returning an empty AABB in get_fallback_gizmo_aabb()?

@lawnjelly
Copy link
Member

I thought the orange box for non-visual instances was intentionally removed on 4.x sweat_smile But some nodes do look better without the box, e.g. Camera3D and GridMap. We can probably make the box optional by returning an empty AABB in get_fallback_gizmo_aabb()?

Yup sure that could work.

Another alternative I considered at the time was moving the get_aabb() function from VisualInstance to Spatial, and have it return a default empty AABB. That would remove the need for the specific function for non-visual instances.

As far as I remember I didn't attempt this at the time just because I didn't want to change too much in 3.x, but it might be worth considering for 4.x. I'm sure this would come up as a possibility in review for a 4.x PR.

@akien-mga akien-mga merged commit 031401f into godotengine:3.x Jan 13, 2023
@akien-mga
Copy link
Member

Thanks!

@timothyqiu timothyqiu deleted the collision-shape-aabb branch January 14, 2023 02:58
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.

3 participants