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

fix: sdk7 primitives radius defaults #5018

Merged
merged 4 commits into from
Apr 26, 2023
Merged

Conversation

pbosio
Copy link
Contributor

@pbosio pbosio commented Apr 20, 2023

What does this PR change?

changed default radius values for cylinder and sphere
fixes decentraland/sdk#558

Our Code Review Standards

https://github.com/decentraland/unity-renderer/blob/master/docs/code-review-standards.md

Copilot summary

🤖 Generated by Copilot at 1e96f9f

Reduced the default radius of sphere and cylinder mesh colliders and renderers to improve the consistency and accuracy of the physics simulation and the visual representation of primitive meshes. Adjusted the default sizes of sphere and cylinder mesh builders to match the corresponding mesh models.

@pbosio pbosio self-assigned this Apr 20, 2023
@pbosio pbosio added the No QA Needed Issues which do not require QA testing label Apr 20, 2023
@pbosio pbosio marked this pull request as ready for review April 20, 2023 20:14
@pbosio pbosio requested a review from a team as a code owner April 20, 2023 20:14
@pbosio pbosio requested review from pravusjif and AjimenezDCL April 20, 2023 20:14
@@ -29,7 +29,7 @@ public static Mesh CreateMesh(AssetPromise_PrimitiveMesh_Model meshModelModel)
}
else
{
mesh = PrimitiveMeshBuilder.BuildSphere(1f);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can take the opportunity to extract a constant, so next time we only have to change it in one place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You made me realize that I should have removed that line 😅
Actually default value it's already defaulted in the sphere property constructor here:

public PropertySphere(float radius = 0.5f, int longitude = 24, int latitude = 16)

@pbosio pbosio requested a review from AjimenezDCL April 25, 2023 14:24
@pbosio pbosio enabled auto-merge (squash) April 26, 2023 13:16
@pbosio pbosio merged commit c7d12a3 into dev Apr 26, 2023
@pbosio pbosio deleted the fix/sdk7-primitives-defaults branch April 26, 2023 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
No QA Needed Issues which do not require QA testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check cyllinder default values
3 participants