Skip to content

Axis-aware coordinate helpers on Point, Vec2, Size, and Rect - #592

Open
waywardmonkeys wants to merge 1 commit into
linebender:mainfrom
waywardmonkeys:axis-coordinate-helpers
Open

Axis-aware coordinate helpers on Point, Vec2, Size, and Rect#592
waywardmonkeys wants to merge 1 commit into
linebender:mainfrom
waywardmonkeys:axis-coordinate-helpers

Conversation

@waywardmonkeys

Copy link
Copy Markdown
Contributor

No description provided.

@DJMcNab DJMcNab left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I do want to make sure that we've thought about these names, because none of them particularly spark joy. Even if the response to each question is just "I can't think of anything better", that's reasonable.

This really just continues the existing naming scheme from the previously added methods of this flavour. Looking back on them, those names are not something I'm hugely happy with. At least this is consistent

Comment thread kurbo/src/point.rs
/// assert_eq!(point, Point::new(2.0, 5.0));
/// ```
#[inline]
pub const fn with_coord(self, axis: Axis, value: f64) -> Point {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What do you think about a name like snap_to_coord. That name also doesn't quite have the right ring to it, but I find that with_coord is a name which really leans on the docs to explain it.

Comment thread kurbo/src/rect.rs
/// assert_eq!(rect.min_coord(Axis::Vertical), 2.0);
/// ```
#[inline]
pub const fn min_coord(&self, axis: Axis) -> f64 {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If I read this name in a vacuum, I'd expect it to be (self.min_x(), self.min_y()). Especially since that method doesn't exist, the idea of adding it with these semantics at least gives me pause.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Since this is an axis-specific method, one option might be min_along / max_along ...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I like that!

Comment thread kurbo/src/rect.rs
/// assert_eq!(rect, Rect::new(10.0, 2.0, 3.0, 7.0));
/// ```
#[inline]
pub const fn with_coords(self, axis: Axis, v0: f64, v1: f64) -> Rect {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does a name like with_extents make sense to you? That feels a little bit too jargony. Wow, what a fine line naming things has...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah. This is not the first version of this PR that I've written. Perhaps the third, all with different names!

@waywardmonkeys

Copy link
Copy Markdown
Contributor Author

I do want to make sure that we've thought about these names, because none of them particularly spark joy. Even if the response to each question is just "I can't think of anything better", that's reasonable.

This really just continues the existing naming scheme from the previously added methods of this flavour. Looking back on them, those names are not something I'm hugely happy with. At least this is consistent

I agree on both counts. The names don't really spark joy, but neither have any others that I've come up with.

I'm happy for this to take a short period of time to see if we can find better names.

@nicoburns

Copy link
Copy Markdown

Also agree that these are helpful additions but the names don't spark joy. In Taffy I have used start and end for min and max, but I guess that might be confusing in kurbo :/

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.

3 participants