blender/blender

Compositor: Add experimental option for new CPU compositor #125960

Merged

Omar Emara merged 6 commits from OmarEmaraDev/blender:new-cpu-compositor-experimental-option into main

2024-08-08 15:40:25 +02:00

Omar Emara commented

2024-08-06 13:13:15 +02:00

Member

This patch introduces a new experimental option for the new CPU
compositor under development. This is to make development easier such
that it happens directly in main, but the compositor is not expected to
work and will probably crash.

This patch introduces a new experimental option for the new CPU compositor under development. This is to make development easier such that it happens directly in main, but the compositor is not expected to work and will probably crash.

Omar Emara added the

labels 2024-08-06 13:13:15 +02:00

Omar Emara added 1 commit 2024-08-06 13:13:23 +02:00

Compositor: Add experimental option for new CPU compositor 19939b4e56

This patch introduces a new experimental option for the new CPU
compositor under development. This is to make development easier such
that it happens directly in main, but the compositor is not expected to
work and will probably crash.

Omar Emara requested review from Sergey Sharybin 2024-08-06 13:13:41 +02:00

Omar Emara added 1 commit 2024-08-06 13:15:07 +02:00

Typo bf443e74ba

Sergey Sharybin commented

2024-08-06 15:12:03 +02:00

Owner

Generally it's all fine to have experimental option for such things. Some notes though:

It might worth linking design task in the description here (its linked for the setting in the code, but would be good to also link from the PR description/commit message as well).
I wouldn't introduce new DNA enum value. Do-versioning would be more painful, and the UI kind of becomes more cluttered. I think it's fine to just have "Enable the new CPU compositor implementation" experimental option, and then whenever scene uses CPU compositor, the new implementation is used. Maybe having dedicated task for it linked from the parent one would solve the confusion of what the state is.
The property naming I am not sure. It is probably fine, but maybe there is something better. @dfelinto would be great to have a couple of cents here (it is in a context of unifying CPU and GPU compositor code paths, which can't happen overnight, so we want some experimental option name for it).

Generally it's all fine to have experimental option for such things. Some notes though: - It might worth linking design task in the description here (its linked for the setting in the code, but would be good to also link from the PR description/commit message as well). - I wouldn't introduce new DNA enum value. Do-versioning would be more painful, and the UI kind of becomes more cluttered. I think it's fine to just have "Enable the new CPU compositor implementation" experimental option, and then whenever scene uses CPU compositor, the new implementation is used. Maybe having dedicated task for it linked from the parent one would solve the confusion of what the state is. - The property naming I am not sure. It is probably fine, but maybe there is something better. @dfelinto would be great to have a couple of cents here (it is in a context of unifying CPU and GPU compositor code paths, which can't happen overnight, so we want some experimental option name for it).

Omar Emara commented

2024-08-06 15:19:12 +02:00

Author

Member

@Sergey The idea behind having a dedicated DNA enum value is to allow quickly changing between different compositors, for comparing results and performance. Changing that in the preferences will be a bit less convenient.

But it is probably not a strong enough reason. I can change the behavior and rename the option to use_new_cpu_compositor.

@Sergey The idea behind having a dedicated DNA enum value is to allow quickly changing between different compositors, for comparing results and performance. Changing that in the preferences will be a bit less convenient. But it is probably not a strong enough reason. I can change the behavior and rename the option to `use_new_cpu_compositor`.

Sergey Sharybin commented

2024-08-06 15:25:59 +02:00

Owner

@OmarEmaraDev If you feel strongly about having a way to easily toggle CPU implementation, maybe we can leave the enum-as-is and have a boolean property on the scene "Use new CPU implementation"? That way we'd just remove that option once the new implementation is mature, without needing to worry about do-version , or files which might have been saved with the new-cpu-device-type.

@OmarEmaraDev If you feel strongly about having a way to easily toggle CPU implementation, maybe we can leave the enum-as-is and have a boolean property on the scene "Use new CPU implementation"? That way we'd just remove that option once the new implementation is mature, without needing to worry about do-version , or files which might have been saved with the new-cpu-device-type.

Omar Emara commented

2024-08-06 15:42:07 +02:00

Author

Member

@Sergey That would be a reasonable compromise, I will do that.
I created a to-do task in #125968.

@Sergey That would be a reasonable compromise, I will do that. I created a to-do task in #125968.

Omar Emara added 1 commit 2024-08-06 16:30:13 +02:00

Address review 97b89c584f

Sergey Sharybin approved these changes 2024-08-06 16:34:05 +02:00

Sergey Sharybin left a comment

Owner

From my side it seems fine.
Still wonder if Dalai's fine with the naming of the experimental option. Sometimes he has stronger opinions about it than me :)
Maybe ping him in the chat before landing?

From my side it seems fine. Still wonder if Dalai's fine with the naming of the experimental option. Sometimes he has stronger opinions about it than me :) Maybe ping him in the chat before landing?

Omar Emara added 2 commits 2024-08-06 19:38:16 +02:00

Merge branch 'main' into new-cpu-compositor-experimental-option 5fc878f3ca

Fix crash 51fa612745

Dalai Felinto requested changes 2024-08-08 11:48:23 +02:00

Dismissed

Dalai Felinto left a comment

Owner

Never check for U.experimental directly, use USER_EXPERIMENTAL_TEST instead.
Never use "Use" for booleans (for their UI text).
I find strange that I have to enable this in two places (the experimental options + the use_new_compositor option). Why not to only use the experimental flag.
As far as naming goes I could just call it "CPU Compositor". It is already under experimental, it is implicit that it is new.

Unrelated to this patch: you are not the only one to forget/neglect USER_EXPERIMENTAL_TEST. If you would/could do a pass on the code and replace all the other U.experimental with this call it would be much appreciated.

* Never check for U.experimental directly, use USER_EXPERIMENTAL_TEST instead. * Never use "Use" for booleans (for their UI text). * I find strange that I have to enable this in two places (the experimental options + the use_new_compositor option). Why not to only use the experimental flag. * As far as naming goes I could just call it "CPU Compositor". It is already under experimental, it is implicit that it is new. --- Unrelated to this patch: you are not the only one to forget/neglect USER_EXPERIMENTAL_TEST. If you would/could do a pass on the code and replace all the other U.experimental with this call it would be much appreciated.

Omar Emara commented

2024-08-08 11:51:38 +02:00

Author

Member

@dfelinto The reason for the extra use_new_compositor is now allow quickly changing between current and new implementation for comparing results and performance. But it is not strictly required and I can do without it. Do you want it removed?

@dfelinto The reason for the extra `use_new_compositor` is now allow quickly changing between current and new implementation for comparing results and performance. But it is not strictly required and I can do without it. Do you want it removed?

Dalai Felinto commented

2024-08-08 11:58:29 +02:00

Owner

@OmarEmaraDev I think it is simpler without:

Simpler for anyone to test in the future.
Keep the code closer to a release state.
Make sure the UI is final, while you can work on the internals.

It is not a deal-breaker. But one of the inspirations of the Experimental panel was to prevent having experimental features scattered everyone in Blender (or in CMake building options).

@OmarEmaraDev I think it is simpler without: * Simpler for anyone to test in the future. * Keep the code closer to a release state. * Make sure the UI is final, while you can work on the internals. It is not a deal-breaker. But one of the inspirations of the Experimental panel was to prevent having experimental features scattered everyone in Blender (or in CMake building options).

Sergey Sharybin commented

2024-08-08 11:58:56 +02:00

Owner

I find strange that I have to enable this in two places (the experimental options + the use_new_compositor option). Why not to only use the experimental flag.

We've gone through this, please read discussion above. Overall it will (a) answer your questions (b) saves everyone's time.
It makes it much easier to compare old/new behavior to ensure they match.

Do you want it removed?

I probably would be better to just negate the meaning of DNA flag, and use RNA_def_property_boolean_negative_sdna for the RNA. This way the scene-specific setting is defaulted to the new behavior, solving possible confusion for those brave people testing the new implementation, but keeping it easy for developers to do comparison.

> I find strange that I have to enable this in two places (the experimental options + the use_new_compositor option). Why not to only use the experimental flag. We've gone through this, please read discussion above. Overall it will (a) answer your questions (b) saves everyone's time. It makes it much easier to compare old/new behavior to ensure they match. > Do you want it removed? I probably would be better to just negate the meaning of DNA flag, and use `RNA_def_property_boolean_negative_sdna` for the RNA. This way the scene-specific setting is defaulted to the new behavior, solving possible confusion for those brave people testing the new implementation, but keeping it easy for developers to do comparison.

Sergey Sharybin commented

2024-08-08 12:01:26 +02:00

Owner

But one of the inspirations of the Experimental panel was to prevent having experimental features scattered everyone in Blender (or in CMake building options).

It is a debugging option. Just similar to the ones we have for Cycles and Hydra.
So perhaps solution is to just move this option to a panel which is only visible with Developers Extra, and make it have no effect if Developers Extras are disabled.

> But one of the inspirations of the Experimental panel was to prevent having experimental features scattered everyone in Blender (or in CMake building options). It is a debugging option. Just similar to the ones we have for Cycles and Hydra. So perhaps solution is to just move this option to a panel which is only visible with Developers Extra, and make it have no effect if Developers Extras are disabled.

Dalai Felinto commented

2024-08-08 12:07:25 +02:00

Owner

@OmarEmaraDev you can just assign a shortcut to toggle the experimental feature. Would that make things slower for you?

For example:

@OmarEmaraDev you can just assign a shortcut to toggle the experimental feature. Would that make things slower for you? For example: <img width="907" alt="image" src="attachments/4e873eea-3209-47c0-aaa3-aeadf27e8d00">

57 KiB

Sergey Sharybin commented

2024-08-08 12:14:54 +02:00

Owner

@dfelinto While this is does the job done, it is unideal from development process. Just think of a way how you'd communicate to contributors what the easy way to ensure things render the same in current/new CPU compsiutor.

It is just much easier if it is in a panel next to compositor itself. The setting you could also animate, and toggle frames back-n-forth.
So, ultimately, it is a Debug panel material. Hopefully it is not too annoying to add such panel for compositor for the currently-single-option in there.

@dfelinto While this is does the job done, it is unideal from development process. Just think of a way how you'd communicate to contributors what the easy way to ensure things render the same in current/new CPU compsiutor. It is just much easier if it is in a panel next to compositor itself. The setting you could also animate, and toggle frames back-n-forth. So, ultimately, it is a Debug panel material. Hopefully it is not too annoying to add such panel for compositor for the currently-single-option in there.

Dalai Felinto commented

2024-08-08 12:26:50 +02:00

Owner

+1 then, I have nothing further to add

+1 then, I have nothing further to add

Dalai Felinto approved these changes 2024-08-08 12:27:42 +02:00

Dalai Felinto left a comment

Owner

Approving. See my suggestion about the name and USER_EXPERIMENTAL_TEST.

Approving. See my suggestion about the name and USER_EXPERIMENTAL_TEST.

Omar Emara added 1 commit 2024-08-08 15:32:51 +02:00

Move option to its own debug panel c59630515e

Omar Emara merged commit f40cf759c7 into main

2024-08-08 15:40:25 +02:00

Omar Emara referenced this issue from a commit

2024-08-08 15:40:25 +02:00

Compositor: Add experimental option for new CPU compositor

Omar Emara deleted branch new-cpu-compositor-experimental-option

2024-08-08 15:40:28 +02:00

Sign in to join this conversation.

No reviewers

No Label

Animation & Rigging

Automated Testing

Blender Asset Bundle

Code Documentation

Dependency Graph

Development Management

Images & Movies

Motion Tracking

Nodes & Physics

Platforms, Builds & Tests

Render & Cycles

Render Pipeline

Sculpt, Paint & Texture

Video Sequencer

Viewport & EEVEE

Virtual Reality

Asset Browser Project

Blender 2.8 Project

Milestone 1: Basic, Local Asset Browser

Good First Issue

Animation & Rigging

Development Management

Nodes & Physics

Platforms, Builds & Tests

Render & Cycles

Sculpt, Paint & Texture

Viewport & EEVEE

Needs Info from Developers

Needs Information from User

No Milestone

No project

No Assignees

3 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#125960

No description provided.