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 a size Vector for adjusting the size of Rectangles and Boxes #44183

Merged
merged 3 commits into from
Dec 28, 2020

Conversation

madmiraal
Copy link
Contributor

Currently, to adjust the size of a RectangleShape2D or a BoxShape3D you modify the extents, which are half the size of the rectangle or box. This is confusing for the user. Furthermore, although a BoxMesh3D (a CubeMesh in 3.2) uses a Vector3 for size, a CSGBox3D uses three floats: width, height and depth.

This PR allows a users to adjust the size of a RectangleShape, a BoxShape, a BoxMesh and a CSGBox consistently using a Vector property called size.

Part of #16863.

@aaronfranke
Copy link
Member

It still says "Extents" in the corner when dragging the handles, I think this is not intentional:

Screenshot from 2020-12-08 06-13-53

For CSG, it says the name of which handle you are manipulating, but then the value is the size vector:

Screenshot from 2020-12-08 06-15-51

@madmiraal
Copy link
Contributor Author

Updated to correctly display "Size" and the Vector3 in the corners.

@aaronfranke
Copy link
Member

This code looks good, and I agree that having a size property makes sense. It's 100% an improvement for CSG.

However, there is an argument for having both size and extents properties. There are a lot of places in this code where size / 2 is used. More importantly, while a Rect2 and AABB have their position defined as the negative corner (top-left for Rect2, bottom-north-west for AABB), the position for BoxShape3D, RectangleShape2D, and CSG nodes is defined as the center, not the corner. To find the edges/corners of a Rect2 or AABB, you can take the position for one edge/corner, and the position plus size for the others. To do the same with BoxShape3D, RectangleShape2D, and CSG nodes, you would need to take the position and add/subtract the extents (half the size). So BoxShape3D currently behaves like Unity's Bounds.

Anyway, I would like @akien-mga's feedback on this (and anyone else who wants to chime in).

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

We discussed this in a PR review meeting and agreed with the change.

(Half) extents are still used internally for performance, but it makes sense to standardize everything in the higher level API around sizes.

@akien-mga akien-mga merged commit 4ca98c7 into godotengine:master Dec 28, 2020
@akien-mga
Copy link
Member

Thanks!

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