Reworked Segment types into their cartesian forms #17404
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jondolf
added
C-Usability
IQuick143
reviewed
Jan 16, 2025
Co-authored-by: Alice Cecile <[email protected]>
alice-i-cecile
requested changes
Jan 16, 2025
alice-i-cecile
approved these changes
Jan 16, 2025
Jondolf
suggested changes
Jan 16, 2025
| @@ -359,22 +359,25 @@ impl Primitive3d for Line3d {} | |||
| reflect(Serialize, Deserialize) | |||
| )] | |||
| pub struct Segment3d { | |||
There was a problem hiding this comment.
I won't copy all of my comments from the 2D version, but most of them also apply here in 3D.
There was a problem hiding this comment.
For the things you havn't specificially mentionned for the Segment3d, I have done the translation and rotation changes, tell me if I missed anything else
Jondolf
reviewed
Jan 16, 2025
Co-authored-by: Joona Aalto <[email protected]>
…into cartesian-segments
Co-authored-by: Joona Aalto <[email protected]>
Co-authored-by: Joona Aalto <[email protected]>
Co-authored-by: Joona Aalto <[email protected]>
|
If I didn't miss anything (unlikely) I should have addressed everything except the stuff I left questions about. |
Jondolf
reviewed
Jan 18, 2025
Co-authored-by: Joona Aalto <[email protected]>
Co-authored-by: Joona Aalto <[email protected]>
|
Everything should be fixed now |
|
This is still unadressed, though I'm fine fixing it in a follow-up too if you'd prefer #17404 (comment) |
Jondolf
approved these changes
Jan 18, 2025
Jondolf
added
S-Ready-For-Final-Review
|
I appreciate your patience as well, there was quite a lot to fix |
Co-authored-by: Joona Aalto <[email protected]>
Co-authored-by: Joona Aalto <[email protected]>
Co-authored-by: Joona Aalto <[email protected]>
Co-authored-by: Joona Aalto <[email protected]>
Carter0
approved these changes
Jan 18, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
Segment2d and Segment3d are currently hard to work with because unlike many other primary shapes, they are bound to the origin.
The objective of this PR is to allow these segments to exist anywhere in cartesian space, making them much more useful in a variety of contexts.
Solution
Reworking the existing segment type's internal fields and methods to allow them to exist anywhere in cartesian space.
I have done both reworks for 2d and 3d segments but I was unsure if I should just have it all here or not so feel free to tell me how I should proceed, for now I have only pushed Segment2d changes.
As I am not a very seasoned contributor, this first implementation is very likely sloppy and will need some additional work from my end, I am open to all criticisms and willing to work to get this to bevy's standards.
Testing
I am not very familiar with the standards of testing. Of course my changes had to pass the thorough existing tests for primitive shapes.
I also checked the gizmo 2d shapes intersection example and everything looked fine.
I did add a few utility methods to the types that have no tests yet. I am willing to implement some if it is deemed necessary
Migration Guide
The segment type constructors changed so if someone previously created a Segment2d with a direction and length they would now need to use the
from_directionconstructor