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

More gizmos builders #13261

Merged
merged 12 commits into from
Jun 3, 2024
Merged

Conversation

lynn-lumen
Copy link
Contributor

@lynn-lumen lynn-lumen commented May 6, 2024

Objective

Solution

  • gizmos.primitive_2d(CIRCLE) and gizmos.primitive_2d(ELLIPSE) now return Ellipse2dBuilder aswell.
  • gizmos.primitive_3d(SPHERE) and gizmos.sphere() now return the same SphereBuilder.
    • the .circle_segments method on the SphereBuilder that used to be returned by .sphere() is now called .segments
    • the sphere primitive gizmo now matches the gizmos.sphere gizmo
  • gizmos.primitive_2d(ANNULUS) now returns a Annulus2dBuilder allowing the configuration of the segments
  • gizmos cylinders and capsules now have only 1 line per axis, similar to gizmos.sphere

Migration Guide

  • Some gizmos.primitive_nd methods now return some or different builders. You may need to adjust types and match statements
  • Replace any calls to circle_segments() with .segments()

@pablo-lua pablo-lua added C-Enhancement A new feature A-Gizmos Visual editor and debug gizmos labels May 6, 2024
Copy link
Contributor

@Jondolf Jondolf left a comment

Choose a reason for hiding this comment

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

Looks pretty good! This seems to be a breaking change though since it renames circle_segments to segments, and also changes the way the sphere primitive gizmo is rendered.

crates/bevy_gizmos/src/primitives/dim2.rs Outdated Show resolved Hide resolved
crates/bevy_gizmos/src/circles.rs Outdated Show resolved Hide resolved
@Jondolf Jondolf added the C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide label May 6, 2024
Copy link
Contributor

github-actions bot commented May 6, 2024

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

@lynn-lumen lynn-lumen requested a review from Jondolf May 6, 2024 22:49
Copy link
Contributor

@pablo-lua pablo-lua left a comment

Choose a reason for hiding this comment

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

I just have a few questions, but overall this is good!

crates/bevy_gizmos/src/primitives/dim2.rs Show resolved Hide resolved
crates/bevy_gizmos/src/circles.rs Outdated Show resolved Hide resolved
@pablo-lua pablo-lua added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label May 18, 2024

// draws one great circle around each of the local axes
Vec3::AXES.into_iter().for_each(|axis| {
let normal = rotation * axis;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let normal = rotation * axis;
let normal = *rotation * axis;

getting a compile error here.

@hymm hymm 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 May 29, 2024
@alice-i-cecile alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jun 3, 2024
@alice-i-cecile
Copy link
Member

@lynn-lumen once CI is green and merge conflicts are resolved I'll merge this in.

@alice-i-cecile alice-i-cecile removed the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label Jun 3, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 3, 2024
Merged via the queue into bevyengine:main with commit e6a0f75 Jun 3, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Gizmos Visual editor and debug gizmos C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Code-Quality A section of code that is hard to understand or change C-Enhancement A new feature 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
Development

Successfully merging this pull request may close these issues.

None yet

5 participants