Realtime Compositor: Allow more result types #112414

Merged

Omar Emara merged 5 commits from OmarEmaraDev/blender:allow-more-result-types into main

2023-09-25 15:13:02 +02:00

Omar Emara commented

2023-09-15 13:32:21 +02:00

Member

Previously, the Result class was reserved for inputs and outputs of
operations, so its allowed types were naturally those exposed to the
user. However, we now use the Result class internally for intermediate
results, so it now makes sense to expend the allowed types.

The types are now divided into two categories, those that are user
facing and need to be handled in implicit operations and those that are
internal can be exempt from such handling. Internal types are reserved
for texture results, as the single value mechanism is only there for
user facing results.

The patch merely adjusts the switch cases across the code base, adding
one new internal type as an example.

Previously, the Result class was reserved for inputs and outputs of operations, so its allowed types were naturally those exposed to the user. However, we now use the Result class internally for intermediate results, so it now makes sense to expend the allowed types. The types are now divided into two categories, those that are user facing and need to be handled in implicit operations and those that are internal can be exempt from such handling. Internal types are reserved for texture results, as the single value mechanism is only there for user facing results. The patch merely adjusts the switch cases across the code base, adding one new internal type as an example.

Omar Emara added the

labels 2023-09-15 13:32:21 +02:00

Omar Emara added 1 commit 2023-09-15 13:32:34 +02:00

cee265ee8a Realtime Compositor: Allow more result types

Previously, the Result class was reserved for inputs and outputs of
operations, so its allowed types were naturally those exposed to the
user. However, we now use the Result class internally for intermediate
results, so it now makes sense to expend the allowed types.

The types are now divided into two categories, those that are user
facing and need to be handled in implicit operations and those that are
internal can be exempt from such handling. Internal types are reserved
for texture results, as the single value mechanism is only there for
user facing results.

The patch merely adjusts the switch cases across the codebase, adding
one new internal type as an example.

Omar Emara requested review from Sergey Sharybin 2023-09-15 13:32:45 +02:00

Sergey Sharybin reviewed 2023-09-18 21:48:35 +02:00

Sergey Sharybin left a comment

Owner

Generally looks fine.

Have a little comment about use of default which I try to avoid in own code. Use some common sense though, it might very well be that the places where it is used is not genetic enough (i.e. limited by the socket type of a specific node -- aka, purely user-facing things).

Do not consider it as a stopper and if you believe the code as it is in the current PR is better that trying to do things explicitly please just go ahead and commit :)

Generally looks fine. Have a little comment about use of `default` which I try to avoid in own code. Use some common sense though, it might very well be that the places where it is used is not genetic enough (i.e. limited by the socket type of a specific node -- aka, purely user-facing things). Do not consider it as a stopper and if you believe the code as it is in the current PR is better that trying to do things explicitly please just go ahead and commit :)

source/blender/compositor/realtime_compositor/algorithms/intern/smaa.cc Outdated

						
				@ -42,6 +44,9 @@ static Result detect_edges(Context &context,

				      GPU_shader_uniform_3fv(shader, "luminance_coefficients", luminance_coefficients);

				      break;

				    }

				    default:

Sergey Sharybin commented

2023-09-18 21:42:09 +02:00

Owner

Typically in such situations I try to avoid default as it helps a lot when new types are added and do an assert if none of the explicit cases matched. It might be easier to do if this piece of code is moved to own small function like set_shader_luminance_coefficients so that early return in place of break are possible.

The reason for this is that if a new type is added in the future it is easier to see where its handling might be needed by utilizing compiler's warnings.

Typically in such situations I try to avoid `default` as it helps a lot when new types are added and do an assert if none of the explicit cases matched. It might be easier to do if this piece of code is moved to own small function like `set_shader_luminance_coefficients` so that early return in place of `break` are possible. The reason for this is that if a new type is added in the future it is easier to see where its handling might be needed by utilizing compiler's warnings.

Omar Emara commented

2023-09-22 13:54:46 +02:00

Author

Member

But if we don't have a default, we will always get such warnings:

warning: enumeration value 'Int2' not handled in switch [-Wswitch]

This is the code I have:

void set_shader_luminance_coefficients(GPUShader *shader, ResultType type)
{
  switch (type) {
    case ResultType::Color: {
      float luminance_coefficients[3];
      IMB_colormanagement_get_luminance_coefficients(luminance_coefficients);
      GPU_shader_uniform_3fv(shader, "luminance_coefficients", luminance_coefficients);
      return;
    }
    case ResultType::Vector: {
      float luminance_coefficients[3] = {1.0f, 1.0f, 1.0f};
      GPU_shader_uniform_3fv(shader, "luminance_coefficients", luminance_coefficients);
      return;
    }
    case ResultType::Float: {
      float luminance_coefficients[3] = {1.0f, 0.0f, 0.0f};
      GPU_shader_uniform_3fv(shader, "luminance_coefficients", luminance_coefficients);
      return;
    }
  }

  BLI_assert_unreachable();
}

Or should we add empty assert casses for Int2 and other similar types with a comment that says why they aren't handled?

But if we don't have a default, we will always get such warnings: ``` warning: enumeration value 'Int2' not handled in switch [-Wswitch] ``` This is the code I have: ```cpp void set_shader_luminance_coefficients(GPUShader *shader, ResultType type) { switch (type) { case ResultType::Color: { float luminance_coefficients[3]; IMB_colormanagement_get_luminance_coefficients(luminance_coefficients); GPU_shader_uniform_3fv(shader, "luminance_coefficients", luminance_coefficients); return; } case ResultType::Vector: { float luminance_coefficients[3] = {1.0f, 1.0f, 1.0f}; GPU_shader_uniform_3fv(shader, "luminance_coefficients", luminance_coefficients); return; } case ResultType::Float: { float luminance_coefficients[3] = {1.0f, 0.0f, 0.0f}; GPU_shader_uniform_3fv(shader, "luminance_coefficients", luminance_coefficients); return; } } BLI_assert_unreachable(); } ``` Or should we add empty assert casses for `Int2` and other similar types with a comment that says why they aren't handled?

Sergey Sharybin commented

2023-09-22 17:30:47 +02:00

Owner

  switch (...) {
    ...
    case ResultType::Int2:
      /* Types which are not supposed to be passed into this node under normal operations. */
      break;
  }
  BLI_assert_unreachable();

But, again, it was more of a generic comment. To keep it practical we can say: if the items to be handled in the switch depends on the node configuration then the current default approach is cleaner and there is no need to make things more verbose than they should be. However, if the addition of a new data type requires update of this switch statement then its better not to have default branch. Hope it makes sense :)

``` switch (...) { ... case ResultType::Int2: /* Types which are not supposed to be passed into this node under normal operations. */ break; } BLI_assert_unreachable(); ``` But, again, it was more of a generic comment. To keep it practical we can say: if the items to be handled in the switch depends on the node configuration then the current `default` approach is cleaner and there is no need to make things more verbose than they should be. However, if the addition of a new data type requires update of this `switch` statement then its better not to have `default` branch. Hope it makes sense :)

OmarEmaraDev marked this conversation as resolved

source/blender/compositor/realtime_compositor/algorithms/intern/symmetric_separable_blur.cc Outdated

						
				@ -26,0 +28,4 @@

				    case ResultType::Vector:

				    case ResultType::Color:

				      return "compositor_symmetric_separable_blur_color";

				    default:

Sergey Sharybin commented

2023-09-18 21:43:15 +02:00

Owner

Move the content outside of the switch.

Move the content outside of the `switch`.

OmarEmaraDev marked this conversation as resolved

Omar Emara added 4 commits 2023-09-25 13:53:13 +02:00

c4e8f2aa5d Merge branch 'main' into allow-more-result-types

494341750f Merge branch 'main' into allow-more-result-types

24144daacd Merge branch 'main' into allow-more-result-types

3a7291a877 Replace defaults when needed

Sergey Sharybin approved these changes 2023-09-25 14:48:49 +02:00

Sergey Sharybin left a comment

Owner

Thanks for the update! Makes more sense now. Also nice to see explicit comments about unexpected code paths in certain case branches!

Thanks for the update! Makes more sense now. Also nice to see explicit comments about unexpected code paths in certain `case` branches!

Omar Emara referenced this issue from a commit

2023-09-25 15:13:02 +02:00

Realtime Compositor: Allow more result types

Omar Emara merged commit 6b53fd5cf6 into main

2023-09-25 15:13:02 +02:00

Omar Emara deleted branch allow-more-result-types

2023-09-25 15:13:04 +02:00

Colin Basnett referenced this issue from a commit

2023-09-27 07:30:22 +02:00

Realtime Compositor: Allow more result types

Sign in to join this conversation.

No reviewers

No Label

Animation & Rigging

Asset Browser Project Overview

Automated Testing

Blender Asset Bundle

Dependency Graph

Development Management

EEVEE & Viewport

Images & Movies

Motion Tracking

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Render Pipeline

Sculpt, Paint & Texture

Video Sequencer

Virtual Reality

Blender 2.8 Project

Milestone 1: Basic, Local Asset Browser

Good First Issue

Animation & Rigging

Development Management

EEVEE & Viewport

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Sculpt, Paint & Texture

Needs Info from Developers

Needs Information from User

No Milestone

No project

No Assignees

2 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#112414

No description provided.