Fullframe compositor: unify scaling behavior with realtime compositor #114764

Habib Gahbiche · 2023-11-12T17:28:41+01:00

Habib Gahbiche commented

2023-11-12 17:28:41 +01:00

This patch unifies the behavior of the scale node by removing the arbitrary limit of scaling images. The change applies for scale and transform node.

	main	this patch
Scaling
Scale undo

Note: with this change, Blender can crash for large resizing factors (around 10 000 x 10 000). We think it's very unlikely users will run into this issue, so we agreed errors coming from failed memory allocation won't be handled, as is the case for GPU compositor.

This patch unifies the behavior of the scale node by removing the arbitrary limit of scaling images. The change applies for scale and transform node. | | main | this patch | | ----------- | ----------- | ----------- | | Scaling | ![before scale](/attachments/d2ce1a41-a37f-4f55-b80a-03cfa6162c6e) | ![after scale](/attachments/1cf29788-9535-41a0-9f52-4d50b1a5da54) | |Scale undo | ![Scale undo before](/attachments/f0510bc7-6315-42c9-ad96-135d83146197) | ![Scale undo after](/attachments/5e4f3936-fb71-443c-a81a-e0e3457ce926) | Note: with this change, Blender can crash for large resizing factors (around 10 000 x 10 000). We think it's very unlikely users will run into this issue, so we agreed errors coming from failed memory allocation won't be handled, as is the case for GPU compositor.

Scale undo before.jpg

143 KiB

after scale.jpg

180 KiB

before scale.jpg

136 KiB

Scale undo after.jpg

133 KiB

Habib Gahbiche added 1 commit 2023-11-12 17:28:52 +01:00

buildbot/vexp-code-patch-coordinator Build done. Details

03954953ac Fullframe compositor: unify scaling behavior with realtime compositor

Habib Gahbiche commented

2023-11-12 17:29:18 +01:00

@blender-bot build

Habib Gahbiche commented

2023-11-13 21:09:28 +01:00

@Jeroen-Bakker do you know the historic reasons behind limiting the image size for scale operation? I was not able to get Blender to crash when scaling images with very large factors, but the image viewer does seem to limit the maximum size of images. Do you think it's ok to eliminate this limitation in order to unify behavior with realtime compositor?

Note: GPU compositor does crash at somepoint when memory runs out, so maybe a limit should be considered to prevent the loss of unsaved files.

@Jeroen-Bakker do you know the historic reasons behind limiting the image size for scale operation? I was not able to get Blender to crash when scaling images with very large factors, but the image viewer does seem to limit the maximum size of images. Do you think it's ok to eliminate this limitation in order to unify behavior with realtime compositor? Note: GPU compositor does crash at somepoint when memory runs out, so maybe a limit should be considered to prevent the loss of unsaved files.

Habib Gahbiche commented

2023-11-17 12:29:30 +01:00

@OmarEmaraDev @Sergey maybe you have opinions here too...

Omar Emara commented

2023-11-17 12:42:17 +01:00

@zazizizou There was no reason for size limiting, it is a documented limitation and the plan was to remove it. This is discussed in D12466.

The fact that the GPU compositor crashes for huge textures is a bug, and should be fixed, can you open a bug report for it? But the fact that it crashes due to insufficient memory might indicate that it is in fact killed, not crashed.

Ideally, we would have support for huge textures on the GPU as well, but this is not a short term target as was agreed before.

@zazizizou There was no reason for size limiting, it is a documented limitation and the plan was to remove it. This is discussed in D12466. The fact that the GPU compositor crashes for huge textures is a bug, and should be fixed, can you open a bug report for it? But the fact that it crashes due to insufficient memory might indicate that it is in fact killed, not crashed. Ideally, we would have support for huge textures on the GPU as well, but this is not a short term target as was agreed before.

Habib Gahbiche commented

2023-11-17 14:17:34 +01:00

I see thanks. Here is the report: #115043

Omar Emara requested review from Omar Emara 2023-11-21 14:39:25 +01:00

Sergey Sharybin added this to the Compositing project 2023-11-21 14:42:16 +01:00

Omar Emara requested changes 2023-11-21 15:39:24 +01:00

Omar Emara left a comment

We still have an interenal limit of DEFAULT_MAX_SCALE_CANVAS_SIZE. Maybe remove max_scale_canvas_size_ entirely and adjust it users accordingly (Mask node only I think).

We still have an interenal limit of `DEFAULT_MAX_SCALE_CANVAS_SIZE`. Maybe remove `max_scale_canvas_size_` entirely and adjust it users accordingly (Mask node only I think).

Habib Gahbiche changed title from ~~Fullframe compositor: unify scaling behavior with realtime compositor~~ to WIP: Fullframe compositor: unify scaling behavior with realtime compositor

2023-11-21 17:30:45 +01:00

Habib Gahbiche added 4 commits 2023-12-26 11:12:45 +01:00

c10a5b8612 Merge remote-tracking branch 'origin/main' into com-unify-scale

73b1023c9c Remove default upper limit for scale. Crashes for large (relative) scaling factors

7fb91a4cfa Merge remote-tracking branch 'origin/main' into com-unify-scale

1fd500e67b Enable constant folding for scaling operation

Habib Gahbiche changed title from ~~WIP: Fullframe compositor: unify scaling behavior with realtime compositor~~ to Fullframe compositor: unify scaling behavior with realtime compositor

2023-12-26 11:20:39 +01:00

Habib Gahbiche requested review from Omar Emara 2023-12-26 11:23:35 +01:00

Omar Emara approved these changes 2023-12-26 11:44:16 +01:00

Habib Gahbiche merged commit 57df35c8f9 into main

2023-12-26 12:58:56 +01:00

Habib Gahbiche referenced this issue from a commit

2023-12-26 12:58:57 +01:00

Fullframe compositor: unify scaling behavior with realtime compositor

Habib Gahbiche deleted branch com-unify-scale

2023-12-26 12:58:58 +01:00

Guillermo Venegas referenced this issue from a commit

2023-12-26 22:06:36 +01:00