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

Add rotation module and functions #669

Merged
merged 3 commits into from
Jun 13, 2024
Merged

Conversation

ripytide
Copy link
Member

As a part of image-rs/image#2238

@theotherphil
Copy link
Contributor

Thanks! These should go alongside the existing rotation functions in the (clunkily named) geometric_transformation module.

@ripytide
Copy link
Member Author

I've moved them across 👍

@ripytide
Copy link
Member Author

ripytide commented Jun 12, 2024

Do you think we should move the flip functions to geometric_transformations as well since they are like reflections, but then the module might get a bit crowded?

@theotherphil
Copy link
Contributor

Let’s leave them for now and bike shed the module structure with whatever functions we’ve got before the next release :-)

/// 0, 0, 0, 0;
/// 0, 0, 0, 0));
/// ```
pub fn rotate90<P>(image: &Image<P>) -> Image<P>
Copy link
Contributor

Choose a reason for hiding this comment

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

I don’t feel strongly either way but it’s a bit unfortunate to use angles in degrees in the names of these functions when angles everywhere else in this file (/crate) are in radians. Can’t think of better names off the top of my head though.

Copy link
Member Author

Choose a reason for hiding this comment

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

As some prior art, opencv just has a single rotate() function which takes an enum that looks like ROTATE_90_CLOCKWISE

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we should do the same and use a single function with an enum, but I don't feel particularly strongly either way.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wondered the same thing. I’ll just merge as is for now. Thanks.

@theotherphil theotherphil merged commit 75246e3 into image-rs:master Jun 13, 2024
15 checks passed
@ripytide ripytide deleted the rotate branch June 13, 2024 14:21
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