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

Rect refactors #638

Closed
ripytide opened this issue May 22, 2024 · 2 comments
Closed

Rect refactors #638

ripytide opened this issue May 22, 2024 · 2 comments

Comments

@ripytide
Copy link
Member

I came across the Rect type when starting on some functions which take rectangular regions such as crop(). I would like to use this type to take arguments to the crop() function. However, there are several issues I have with it at the moment that make me reluctant to go near it.

Currently defined as:

pub struct Rect {
    left: i32,
    top: i32,
    width: u32,
    height: u32,
}
  • Naming: I dislike the non-intuitive left and top members which don't define which is the x and which is the y.
  • Visibility: Could we make the members public since this is a very simple data struct.
  • Data-types: I don't understand why left and top are i32 and not u32 since negative values don't make any sense in normal image coordinates. Also various Image methods take x: u32 which makes this very un-ergonomic to use.

Proposal:
May I suggest we change Rect to be defined as:

pub struct Rect {
    pub x: u32,
    pub y: u32,
    pub width: u32,
    pub height: u32,
}

The docs can then state that the xy define the top-left point of the rectangle.

This also matches other Rect types in the rust ecosystem such as the Rectangle from iced.

@ripytide
Copy link
Member Author

ripytide commented May 25, 2024

The motivation for this change was me wanting to implement crop() which would take a Rect but the current Rect is not conducive to the crop() functions requirements as described above.

crop() and similar functions are what I'm implementing as a part of image-rs/image#2238

@ripytide
Copy link
Member Author

ripytide commented May 26, 2024

Actually, I've just found the Rect from image which is already fits the exact pattern I'd proposed. I'm going to close this and open a different issue that changes the suggestion to migrating to image::Rect.

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 a pull request may close this issue.

1 participant