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

CircleSegment: Add inner_arc and outer_arc #368

Merged
merged 1 commit into from
Aug 26, 2024

Conversation

waywardmonkeys
Copy link
Contributor

No description provided.

@waywardmonkeys
Copy link
Contributor Author

Draft because:

  • Depends on Add Arc::flipped #367, Arc::flipped for a doc comment link.
  • Am I right to have the inner_arc flipped from the outer_arc?
  • Should we add similar things for the two radial lines connecting the arcs?

@waywardmonkeys waywardmonkeys force-pushed the inner_outer_radius branch 3 times, most recently from d7b2313 to 6e36b16 Compare August 22, 2024 05:18
@waywardmonkeys waywardmonkeys marked this pull request as ready for review August 22, 2024 17:07
Copy link
Collaborator

@richard-uk1 richard-uk1 left a comment

Choose a reason for hiding this comment

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

LGTM

  • I think it's right that the direction matches the way the arc is drawn
  • we can always add radial lines later (and personally I doubt they're useful)

@waywardmonkeys
Copy link
Contributor Author

I thought it might be useful to have the line for the inner and outer chords ... but if we expose inner / outer start and end points (rather than just center + radius + angle), then we can more readily construct whatever.

@waywardmonkeys waywardmonkeys added this pull request to the merge queue Aug 26, 2024
@waywardmonkeys
Copy link
Contributor Author

(but we could also do that from the arc ... let the arc expose start / end points or a chord)

Merged via the queue into linebender:main with commit 9c77957 Aug 26, 2024
15 checks passed
@waywardmonkeys waywardmonkeys deleted the inner_outer_radius branch August 26, 2024 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants