blender
/
blender
136
419
Fork 600
Code Issues 6.3k Pull Requests 779 Projects 19 Wiki Activity

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
Conversation 0 Commits 5 Files Changed 8 +73 -37
7 changed files with 73 additions and 37 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 3a7291a877 - Show all commits

51
source/blender/compositor/realtime_compositor/algorithms/intern/smaa.cc
View File

@ -19,6 +19,34 @@
namespace blender::realtime_compositor {
static 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;
}
case ResultType::Int2: {
/* SMAA does not support integer types. */
break;
}
}
BLI_assert_unreachable();
OmarEmaraDev marked this conversation as resolved Outdated
Sergey Sharybin commented 2023-09-18 21:42:09 +02:00
Outdated
Review
Copy Link

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
Outdated
Review
Copy Link

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
Outdated
Review
Copy Link
  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 :)
}
static Result detect_edges(Context &context,
Result &input,
float threshold,
@ -27,28 +55,7 @@ static Result detect_edges(Context &context,
GPUShader *shader = context.shader_manager().get("compositor_smaa_edge_detection");
GPU_shader_bind(shader);
switch (input.type()) {
case ResultType::Color: {
float luminance_coefficients[3];
IMB_colormanagement_get_luminance_coefficients(luminance_coefficients);
GPU_shader_uniform_3fv(shader, "luminance_coefficients", luminance_coefficients);
break;
}
case ResultType::Vector: {
float luminance_coefficients[3] = {1.0f, 1.0f, 1.0f};
GPU_shader_uniform_3fv(shader, "luminance_coefficients", luminance_coefficients);
break;
}
case ResultType::Float: {
float luminance_coefficients[3] = {1.0f, 0.0f, 0.0f};
GPU_shader_uniform_3fv(shader, "luminance_coefficients", luminance_coefficients);
break;
}
default:
BLI_assert_unreachable();
break;
}
set_shader_luminance_coefficients(shader, input.type());
GPU_shader_uniform_1f(shader, "smaa_threshold", threshold);
GPU_shader_uniform_1f(
shader, "smaa_local_contrast_adaptation_factor", local_contrast_adaptation_factor);

9
source/blender/compositor/realtime_compositor/algorithms/intern/symmetric_separable_blur.cc
View File

@ -28,10 +28,13 @@ static const char *get_blur_shader(ResultType type)
case ResultType::Vector:
case ResultType::Color:
return "compositor_symmetric_separable_blur_color";
default:
BLI_assert_unreachable();
return nullptr;
case ResultType::Int2:
OmarEmaraDev marked this conversation as resolved Outdated
Sergey Sharybin commented 2023-09-18 21:43:15 +02:00
Outdated
Review
Copy Link

Move the content outside of the switch.

Move the content outside of the `switch`.
/* Blur does not support integer types. */
break;
}
BLI_assert_unreachable();
return nullptr;
}
static Result horizontal_pass(Context &context,

1
source/blender/compositor/realtime_compositor/intern/input_single_value_operation.cc
View File

@ -47,6 +47,7 @@ void InputSingleValueOperation::execute()
result.set_color_value(float4(bsocket->default_value_typed<bNodeSocketValueRGBA>()->value));
break;
default:
/* Other types are internal and needn't be handled by operations. */
BLI_assert_unreachable();
break;
}

2
source/blender/compositor/realtime_compositor/intern/realize_on_domain_operation.cc
View File

@ -100,6 +100,7 @@ GPUShader *RealizeOnDomainOperation::get_realization_shader()
case ResultType::Float:
return shader_manager().get("compositor_realize_on_domain_bicubic_float");
default:
/* Other types are internal and needn't be handled by operations. */
BLI_assert_unreachable();
return nullptr;
}
@ -113,6 +114,7 @@ GPUShader *RealizeOnDomainOperation::get_realization_shader()
case ResultType::Float:
return shader_manager().get("compositor_realize_on_domain_float");
default:
/* Other types are internal and needn't be handled by operations. */
BLI_assert_unreachable();
return nullptr;
}

1
source/blender/compositor/realtime_compositor/intern/reduce_to_single_value_operation.cc
View File

@ -46,6 +46,7 @@ void ReduceToSingleValueOperation::execute()
result.set_float_value(*pixel);
break;
default:
/* Other types are internal and needn't be handled by operations. */
BLI_assert_unreachable();
break;
}

3
source/blender/compositor/realtime_compositor/intern/result.cc
View File

@ -98,6 +98,7 @@ void Result::allocate_invalid()
set_color_value(float4(0.0f));
break;
default:
/* Other types are internal and do not support single values. */
BLI_assert_unreachable();
break;
}
@ -170,6 +171,8 @@ void Result::steal_data(Result &source)
color_value_ = source.color_value_;
break;
default:
/* Other types are internal and do not support single values. */
BLI_assert_unreachable();
break;
}

43
source/blender/compositor/realtime_compositor/intern/shader_operation.cc
View File

@ -229,9 +229,12 @@ static const char *get_set_function_name(ResultType type)
case ResultType::Color:
return "set_rgba";
default:
BLI_assert_unreachable();
return nullptr;
/* Other types are internal and needn't be handled by operations. */
break;
}
BLI_assert_unreachable();
return nullptr;
}
void ShaderOperation::declare_operation_input(DInputSocket input_socket,
@ -314,9 +317,12 @@ static const char *get_store_function_name(ResultType type)
case ResultType::Color:
return "node_compositor_store_output_color";
default:
BLI_assert_unreachable();
return nullptr;
/* Other types are internal and needn't be handled by operations. */
break;
}
BLI_assert_unreachable();
return nullptr;
}
void ShaderOperation::populate_operation_result(DOutputSocket output_socket, GPUMaterial *material)
@ -404,9 +410,12 @@ static eGPUTextureFormat texture_format_from_result_type(ResultType type)
case ResultType::Color:
return GPU_RGBA16F;
default:
BLI_assert_unreachable();
return GPU_RGBA16F;
/* Other types are internal and needn't be handled by operations. */
break;
}
BLI_assert_unreachable();
return GPU_RGBA16F;
}
/* Texture storers in the shader always take a vec4 as an argument, so encode each type in a vec4
@ -421,9 +430,12 @@ static const char *glsl_store_expression_from_result_type(ResultType type)
case ResultType::Color:
return "color";
default:
BLI_assert_unreachable();
return nullptr;
/* Other types are internal and needn't be handled by operations. */
break;
}
BLI_assert_unreachable();
return nullptr;
}
void ShaderOperation::generate_code_for_outputs(ShaderCreateInfo &shader_create_info)
@ -484,6 +496,7 @@ void ShaderOperation::generate_code_for_outputs(ShaderCreateInfo &shader_create_
store_color_function << case_code.str();
break;
default:
/* Other types are internal and needn't be handled by operations. */
BLI_assert_unreachable();
break;
}
@ -510,9 +523,12 @@ static const char *glsl_type_from_result_type(ResultType type)
case ResultType::Color:
return "vec4";
default:
BLI_assert_unreachable();
return nullptr;
/* Other types are internal and needn't be handled by operations. */
break;
}
BLI_assert_unreachable();
return nullptr;
}
/* Texture loaders in the shader always return a vec4, so a swizzle is needed to retrieve the
@ -527,9 +543,12 @@ static const char *glsl_swizzle_from_result_type(ResultType type)
case ResultType::Color:
return "rgba";
default:
BLI_assert_unreachable();
return nullptr;
/* Other types are internal and needn't be handled by operations. */
break;
}
BLI_assert_unreachable();
return nullptr;
}
void ShaderOperation::generate_code_for_inputs(GPUMaterial *material,