UI: Image Rotate #117352

Harley Acheson · 2024-01-20T04:50:42+01:00

Harley Acheson commented

2024-01-20 04:50:42 +01:00

Operator to rotate images in 90 degree increments.

This PR changes the "Flip" menu to "Transform" and adds options to rotate the image by 90 degrees clockwise, 90 counter-clockwise, and 180 degrees.

Operator to rotate images in 90 degree increments. --- This PR changes the "Flip" menu to "Transform" and adds options to rotate the image by 90 degrees clockwise, 90 counter-clockwise, and 180 degrees. ![image](/attachments/88e90d22-f5bc-473c-a166-ceb81ea7cb27)

image.png

212 KiB

image.png

34 KiB

👀 6 🎉 1

Harley Acheson added 1 commit 2024-01-20 04:50:50 +01:00

02d9c2b8be UI: Image Rotate

Operator to rotate images in 90 degree increments.

Harley Acheson changed title from ~~UI: Image Rotate~~ to WIP: UI: Image Rotate

2024-01-20 04:52:43 +01:00

Harley Acheson added this to the User Interface project 2024-01-20 04:54:02 +01:00

Aras Pranckevicius commented

2024-01-20 08:40:22 +01:00

I think IMB_transform could be used for this, even if it's somewhat overkill. You would get multi-threading "for free" though.

If not that, I think factoring out the actual rotation function into separate piece of code would be good. Like there already exist IMB_flipx, IMB_flipy, rotation by (multiple of) 90 degrees could be a function next to them. Which is fairly similar to what soemeone else is doing over at #115661 (in order to auto-rotate videos based on their orientation metadata).

I think `IMB_transform` could be used for this, even if it's somewhat overkill. You would get multi-threading "for free" though. If not that, I think factoring out the actual rotation function into separate piece of code would be good. Like there already exist `IMB_flipx`, `IMB_flipy`, rotation by (multiple of) 90 degrees could be a function next to them. Which is fairly similar to what soemeone else is doing over at #115661 (in order to auto-rotate videos based on their orientation metadata).

👍 1 👀 1

Harley Acheson added 1 commit 2024-01-20 19:15:42 +01:00

a0070e3ee9 Simplification

Harley Acheson commented

2024-01-20 19:35:00 +01:00

@aras_p - I think IMB_transform could be used for this, even if it's somewhat overkill. You would get multi-threading "for free" though.

Ohhhh. That sounds like a perfect routine to help make an "Arbitrary Rotation" for any angle and selectable center? Any hints for what such a transform matrix would look like - for someone clueless about such things? Or how I would find out ahead of time how big of a rectangle would comprise the result without cutting anything off? I'm assuming I'd have to know this in order to allocate the target bitmap correctly.

If not that, I think factoring out the actual rotation function into separate piece of code would be good. Like there already exist IMB_flipx, IMB_flipy, rotation by (multiple of) 90 degrees could be a function next to them.

I just cleaned up my mess of code so the different rotations only vary by a single line.

I did wonder where it should go and only stuck it all in image_ops because of the similar code for IMAGE_OT_flip. But yes, now wondering why that code doesn't use IMB_flipx and IMB_flipy.

Aras, if this code is moved to rotate.cc, what would you suggest for rotational arguments? Three enums? Integers of degrees but only working on 90, 180, etc?

> @aras_p - I think `IMB_transform` could be used for this, even if it's somewhat overkill. You would get multi-threading "for free" though. Ohhhh. That sounds like a perfect routine to help make an "Arbitrary Rotation" for any angle and selectable center? Any hints for what such a transform matrix would look like - for someone clueless about such things? Or how I would find out ahead of time how big of a rectangle would comprise the result without cutting anything off? I'm assuming I'd have to know this in order to allocate the target bitmap correctly. > If not that, I think factoring out the actual rotation function into separate piece of code would be good. Like there already exist `IMB_flipx`, `IMB_flipy`, rotation by (multiple of) 90 degrees could be a function next to them. I just cleaned up my mess of code so the different rotations only vary by a single line. I did wonder where it should go and only stuck it all in `image_ops` because of the similar code for IMAGE_OT_flip. But yes, now wondering why that code doesn't use `IMB_flipx` and `IMB_flipy`. Aras, if this code is moved to `rotate.cc`, what would you suggest for rotational arguments? Three enums? Integers of degrees but only working on 90, 180, etc?

Harley Acheson added 1 commit 2024-01-20 20:21:39 +01:00

4a2b17c5f5 Changing to "degrees". Better folding of any positive or negative rotation.

Harley Acheson added 1 commit 2024-01-20 20:25:03 +01:00

27e1520dcc Comment changes

Harley Acheson added 1 commit 2024-01-20 21:39:32 +01:00

e0746fab2e Moving the rotate code into ImBuf rotate.cc

Harley Acheson changed title from ~~WIP: UI: Image Rotate~~ to UI: Image Rotate

2024-01-20 21:41:57 +01:00

Harley Acheson requested review from Campbell Barton 2024-01-20 21:42:30 +01:00

Brecht Van Lommel commented

2024-01-22 10:53:10 +01:00

That sounds like a perfect routine to help make an "Arbitrary Rotation" for any angle and selectable center?

I would not implement that arbitrary angle functionality now. It may be tempting to add a bunch image editing operations because they are relatively straightforward. But there are a of design and implementation questions if someone wanted to turn the image editor into more a a complete editor, and I rather see us focus our efforts on other things than go into that rabbit hole.

> That sounds like a perfect routine to help make an "Arbitrary Rotation" for any angle and selectable center? I would not implement that arbitrary angle functionality now. It may be tempting to add a bunch image editing operations because they are relatively straightforward. But there are a of design and implementation questions if someone wanted to turn the image editor into more a a complete editor, and I rather see us focus our efforts on other things than go into that rabbit hole.

Harley Acheson commented

2024-01-22 20:38:44 +01:00

@brecht - I would not implement that arbitrary angle functionality now...

No worries.

I also really don't mind dropping this orthogonal rotation PR; was just a shiny thing to keep me occupied on the weekend for a bit. This 90 degree rotation seems a bit more foundational though since this needs to happen often when taking images off camera. And it sounds like VSE might want similar.

> @brecht - I would not implement that arbitrary angle functionality now... No worries. I also really don't mind dropping **this** orthogonal rotation PR; was just a shiny thing to keep me occupied on the weekend for a bit. This 90 degree rotation seems a bit more foundational though since this needs to happen often when taking images off camera. And it sounds like VSE might want similar.

Brecht Van Lommel commented

2024-01-22 21:37:18 +01:00

Rotating by 90° doesn't really raise any design question, so I have no objection to that.

Harley Acheson added 2 commits 2024-01-30 19:36:32 +01:00

ca253877bb Merge branch 'main' of projects.blender.org:blender/blender into ImageRotate

5a3250fc5a Capitalization changes

Harley Acheson requested review from Brecht Van Lommel 2024-01-30 19:39:54 +01:00

Brecht Van Lommel requested changes 2024-01-30 20:05:17 +01:00

source/blender/editors/space_image/image_ops.cc

						
				@ -2844,0 +2901,4 @@

				  ot->poll = image_from_context_has_data_poll_active_tile;

				  /* properties */

				  PropertyRNA *prop;

Brecht Van Lommel commented

2024-01-30 19:57:38 +01:00

This should be an enum as it doesn't actually support more values.

Harley marked this conversation as resolved

source/blender/imbuf/intern/rotate.cc Outdated

						
				@ -16,1 +16,4 @@

				bool IMB_rotate_orthogonal(ImBuf *ibuf, int degrees)

				{

				  if (ibuf == nullptr) {

Brecht Van Lommel commented

2024-01-30 20:02:10 +01:00

This should not be needed.

Harley marked this conversation as resolved

source/blender/imbuf/intern/rotate.cc

						
				@ -17,0 +21,4 @@

				  }

				  /* Fold to -359 to 359. Note Mod operator is machine-dependent for negative operands. */

				  degrees = degrees - (degrees / 360) * 360;

Brecht Van Lommel commented

2024-01-30 20:01:21 +01:00

This shouldn't be needed if only fixed degrees are supported.

Harley marked this conversation as resolved

source/blender/imbuf/intern/rotate.cc Outdated

						
				@ -17,0 +42,4 @@

				    for (int y = 0; y < size_y; y++) {

				      for (int x = 0; x < size_x; x++) {

				        const float *source_pixel = &orig_float_pixels[(y * size_x + x) * 4];

				        if (degrees == 90) {

Brecht Van Lommel commented

2024-01-30 20:03:10 +01:00

Move this check out of the loop and duplicate the for loops. More efficient that way.

Harley marked this conversation as resolved

source/blender/imbuf/intern/rotate.cc

						
				@ -17,0 +45,4 @@

				        if (degrees == 90) {

				          memcpy(&float_pixels[(y + ((size_x - x - 1) * size_y)) * 4],

				                 source_pixel,

				                 sizeof(float[4]));