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

Move Circle Gizmos to Their Own File #10631

Merged
merged 10 commits into from
Nov 20, 2023
Merged

Move Circle Gizmos to Their Own File #10631

merged 10 commits into from
Nov 20, 2023

Conversation

SIGSTACKFAULT
Copy link
Contributor

@SIGSTACKFAULT SIGSTACKFAULT commented Nov 18, 2023 •
edited
Loading

Objective

  • Give all the intuitive groups of gizmos their own file
  • don't be a breaking change we decided it's fine to break this
  • don't change Gizmos interface
  • eventually do arcs too
  • future types of gizmos should be in their own files
  • see also Tracking issue: Amazing Gizmos #9400

Solution

  • Moved gizmos.circle, gizmos.2d_circle, and assorted helpers into circles.rs
  • Can also do arcs in this MR if y'all want; just figured I should do one thing at a time.

Changelog

  • Changed
    • gizmos::CircleBuilder moved to gizmos::circles::Circle2dBuilder
    • gizmos::Circle2dBuilder moved to gizmos::circles::Circle2dBuilder

Migration Guide

  • change gizmos::CircleBuilder to gizmos::circles::Circle2dBuilder
  • change gizmos::Circle2dBuilder to gizmos::circles::Circle2dBuilder

alice-i-cecile
alice-i-cecile reviewed Nov 18, 2023
View reviewed changes
crates/bevy_gizmos/src/lib.rs Outdated
@@ -17,6 +17,7 @@
//! See the documentation on [`Gizmos`] for more examples.

mod arrows;
mod circles;
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be pub? Along with arrows. Currently I don't see any way to access these types externally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TIL it's possible to have a struct returned by a function which isn't exported anywhere

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, Rust's module privacy rules are bizarre, where you can create "sealed" traits and structs.

This got used to create unimplementable traits, so now they can't fix it.

@alice-i-cecile alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change A-Gizmos Visual editor and debug gizmos labels Nov 18, 2023
@alice-i-cecile
Copy link
Member

Currently this is a breaking change, since we're not re-exporting.

@SIGSTACKFAULT
Revert "pub use DEFAULT_CIRCLE_SEGMENTS"
60395b8 
turns out DEFAULT_CIRCLE_SEGMENTS wasn't public before and I want to
work on one thing at a time

This reverts commit 89211d2.
@SIGSTACKFAULT
make circle_inner private
84acba4 
not sure why I made it public in the first place
@SIGSTACKFAULT
make circles and arrows public
39dc3a5
@SIGSTACKFAULT
document ArrowBuilder
52311a8 
it was already documented but apparently past-me put the doc-comment in
the wrong place, but it happened to be technically private so the linter
didn't complain
@SIGSTACKFAULT
Copy link
Contributor Author

SIGSTACKFAULT commented Nov 18, 2023 •
edited
Loading

Need to re-export CircleBuilder and CircleBuilder2d inside gizmos.rs, i presume?

(Neglected to mention it but I don't want this to be a breaking change)

@alice-i-cecile
Copy link
Member

Need to re-export CircleBuilder and CircleBuilder2d inside gizmos.rs, i presume?

Yeah, if you want to avoid breaking changes that's the plan. Personally I think we should just make the breaking change now :)

@alice-i-cecile alice-i-cecile added M-Needs-Migration-Guide 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 labels Nov 18, 2023
@SIGSTACKFAULT
Copy link
Contributor Author

I mean if you want it to be a breaking change that's also fine by me.

@SIGSTACKFAULT @mockersf
Spell "Circles" correctly
edce806 
Co-authored-by: François <mockersf@gmail.com>
@SIGSTACKFAULT
Copy link
Contributor Author

snaps fingers what i should've said in the first place is "I didn't plan for this to be a breaking change but i'm fine with breaking or non-breaking"

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Nov 19, 2023
@SIGSTACKFAULT
Copy link
Contributor Author

added migration guide to description

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 19, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Nov 19, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 19, 2023
@SIGSTACKFAULT
Copy link
Contributor Author

check-unused-dependencies: "failed to compile cargo-udeps v0.1.43".
Doesn't even get to actual dependency checking.

@SIGSTACKFAULT
Copy link
Contributor Author

SIGSTACKFAULT commented Nov 20, 2023 •
edited
Loading

upstream est31/cargo-udeps#228 and rust-lang/cargo#13004

can run the problematic job locally (to check if it's fixed) with act -W .github/workflows/validation-jobs.yml -j check-unused-dependencies merge_group

@mockersf mockersf added this pull request to the merge queue Nov 20, 2023
Merged via the queue into bevyengine:main with commit 04ab9a0 Nov 20, 2023
21 checks passed
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
@SIGSTACKFAULT @mockersf
Move Circle Gizmos to Their Own File (bevyengine#10631)
ff87da8 
## Objective

- Give all the intuitive groups of gizmos their own file
- don't be a breaking change
- don't change Gizmos interface
- eventually do arcs too
- future types of gizmos should be in their own files
- see also bevyengine#9400

## Solution

- Moved `gizmos.circle`, `gizmos.2d_circle`, and assorted helpers into
`circles.rs`
- Can also do arcs in this MR if y'all want; just figured I should do
one thing at a time.

## Changelog

- Changed
  - `gizmos::CircleBuilder` moved to `gizmos::circles::Circle2dBuilder`
- `gizmos::Circle2dBuilder` moved to `gizmos::circles::Circle2dBuilder`

## Migration Guide

- change `gizmos::CircleBuilder` to `gizmos::circles::Circle2dBuilder`
- change `gizmos::Circle2dBuilder` to `gizmos::circles::Circle2dBuilder`

---------

Co-authored-by: François <mockersf@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tim-blackbird tim-blackbird tim-blackbird approved these changes

@mockersf mockersf mockersf approved these changes

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

Assignees
No one assigned
Labels
A-Gizmos Visual editor and debug gizmos C-Code-Quality A section of code that is hard to understand or change M-Needs-Migration-Guide 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.
4 participants
@SIGSTACKFAULT @alice-i-cecile @mockersf @tim-blackbird