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.

Sign up for GitHub

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

Jump to bottom

Use a well defined type for sides in RegularPolygon #13837

Merged
merged 2 commits into from
Jun 19, 2024
Merged

Use a well defined type for sides in RegularPolygon #13837

merged 2 commits into from
Jun 19, 2024

Conversation

NiseVoid
Copy link
Contributor

@NiseVoid NiseVoid commented Jun 14, 2024 •
edited by alice-i-cecile
Loading

Objective

  • Primitives should not use poorly defined types like usize, especially since they are serializable

Solution

  • Use u32 instead of usize
  • The generic array types do not need to be changed because this size is not actually stored or serialized anywhere

Migration Guide

  • RegularPolygon now uses u32 instead of usize for the number of sides

@NiseVoid NiseVoid added A-Math Fundamental domain-agnostic mathematical operations C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 14, 2024
alice-i-cecile
alice-i-cecile reviewed Jun 14, 2024
View reviewed changes
crates/bevy_gizmos/src/primitives/dim2.rs
.map(|p| {
single_circle_coordinate(
primitive.circumcircle.radius,
primitive.sides as usize,
Copy link
Member

Choose a reason for hiding this comment

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

We should probably change the argument type accepted here too.

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 14, 2024
@alice-i-cecile alice-i-cecile added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jun 19, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 19, 2024
@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 19, 2024
Merged via the queue into bevyengine:main with commit 524dce7 Jun 19, 2024
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mweatherley mweatherley mweatherley approved these changes

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@Chubercik Chubercik Chubercik approved these changes

Assignees
No one assigned
Labels
A-Math Fundamental domain-agnostic mathematical operations C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@NiseVoid @mweatherley @alice-i-cecile @Chubercik