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

Improve the default size of 3D shapes and meshes (Box, Capsule, and Cylinder) #56365

Merged
merged 1 commit into from
Feb 3, 2022

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Dec 31, 2021

Implements and closes godotengine/godot-proposals#3732

The specific values are up for discussion in the above proposal, but I think what's currently in this PR is quite good.

I don't really have an opinion on the CSGTorus3D changes, they were copied from https://github.com/godotengine/godot/pull/49729/files#diff-a81f2e3a53dfacfdb0037de9538db28d5646ae0c3a11a52eca1caa5d665dfd1cR1676-R1678

Breaks compat, but we have already broken compatibility with these shapes for 4.0 so changing this is not a big deal.

@JFonS
Copy link
Contributor

JFonS commented Jan 6, 2022

I agree with the new Box defaults, but I don't really like the cylinder/capsule ones.

I would rather have a 0.5 default radius and a 1.0 default height. So they follow the same rationale of "a scale of 7 means it's 7 meters wide" that is mentioned in the proposal.

In any case, there's never going to be a perfect default value, but if we are going to change any defaults, it should be done before alpha.

@akien-mga
Copy link
Member

After discussion in a PR meeting we suggesting using:

  • 1.0 for Box (as in this PR)
  • Radius 0.5 and height 2.0 for capsule/cylinder.

@@ -239,7 +239,7 @@ class CSGBox3D : public CSGPrimitive3D {
virtual CSGBrush *_build_brush() override;

Ref<Material> material;
Vector3 size = Vector3(2, 2, 2);
Vector3 size = Vector3(1, 1, 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

As long as it follows the style guide,

Suggested change
Vector3 size = Vector3(1, 1, 1);
Vector3 size(1, 1, 1);

Copy link
Member

Choose a reason for hiding this comment

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

That's actually a style that we don't really use in the codebase so far, so it's better not to start using it as it adds inconsistency in the codebase.

@aaronfranke
Copy link
Member Author

This PR has been updated so that the capsule and cylinder defaults are now 0.5 radius and 2.0 height. While I still think 0.25 radius and 1.75 height is better, 0.5 radius and 2.0 height is still very good and it has consensus.

@akien-mga akien-mga merged commit 45d5aa5 into godotengine:master Feb 3, 2022
@akien-mga
Copy link
Member

Thanks!

@aaronfranke aaronfranke deleted the default-shape-size branch February 3, 2022 16:01
@Calinou
Copy link
Member

Calinou commented Feb 4, 2022

It seems SphereMesh is now twice as large as BoxMesh by default. Should we make spheres smaller to match BoxMesh again? The same applies to CSGSphere3D.

@aaronfranke
Copy link
Member Author

aaronfranke commented Feb 4, 2022

@Calinou I'm not sure. On one hand, that makes sense. On the other hand, it's simpler to have the sphere be a radius of 1.0 as the default, and people usually think of circular shapes in terms of the radius. Either way seems fine to me.

EDIT: Also, a radius of 0.5 would be more consistent with the ends of the new capsule size. So perhaps it's better.

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.

Improve the default size of 3D shapes and meshes (Box, Capsule, and Cylinder)
5 participants