Compositor: add new node: Kuwahara filter #107015

Merged
Habib Gahbiche merged 22 commits from zazizizou/blender:com-kuwahara-filter-node into main 2023-06-08 16:14:51 +02:00
Member

Summary:

The filter is used to reduce noise while preserving edges. It can be used to create a cartoon effect from photorealistic images.

The idea is to offer two modes:

  1. Classic aka isotropic kuwahara filter: simple and faster computation. Algorithm splits an area around a single pixel in four parts and computes the mean of the region with the lowest standard deviation
  2. Anisotropic Kuwahara filter: improve the classical approach by considering the direction of structures of regions

This patch implements both approaches above as multi-threaded operations for the full-frame compositor. The attached gif demonstrates how to use the filter.

WIP Progress:

  • Implement classic kuwahara filter

    •  full-frame / multi-threaded
    • tiled
    • Compute weights on luminance channel instead of all 3 channels
      before/after.
  • Implement anisotropic kuwahara filter for 3 channel RGB images

    • full-frame / multi-threaded
    • tiled
    • Compute weights and orientation on luminance channel
    • Implement orientation from structure tensor as a separate operation. (Won't do)
  • Performance improvements

    • optimize luminance computation for both variations
  • Regression testing (will be submitted after merging)

  • Validate UI with users: https://devtalk.blender.org/t/compositor-new-node-kuwahara-filter/29205

Limitations:

  • [solved] Filter is computed per channel. This produces small artefacts. The problem can be solved by operating on a gray image (e.g. luminance ) to make sure only one region is used to select the relevant region.
  • [solved] There is no special case handling for image borders (affects one line per border). This can cause artefacts when stitching images.
### Summary: The filter is used to reduce noise while preserving edges. It can be used to create a cartoon effect from photorealistic images. The idea is to offer two modes: 1) Classic aka isotropic kuwahara filter: simple and faster computation. Algorithm splits an area around a single pixel in four parts and computes the mean of the region with the lowest standard deviation 2) Anisotropic Kuwahara filter: improve the classical approach by considering the direction of structures of regions This patch implements both approaches above as multi-threaded operations for the full-frame compositor. The attached gif demonstrates how to use the filter. ### WIP Progress: - [x] Implement classic kuwahara filter - [x] full-frame / multi-threaded - [x] tiled - [x] Compute weights on luminance channel instead of all 3 channels ![before/after](https://i.ibb.co/qJtk4G4/Screenshot-2023-04-30-at-07-39-59.png). - [x] Implement anisotropic kuwahara filter for 3 channel RGB images - [x] full-frame / multi-threaded - [x] tiled - [x] Compute weights and orientation on luminance channel - [x] ~~Implement orientation from structure tensor as a separate operation. (Won't do)~~ - [x] Performance improvements - [x] optimize luminance computation for both variations - [x] Regression testing (will be submitted after merging) - [x] Validate UI with users: https://devtalk.blender.org/t/compositor-new-node-kuwahara-filter/29205 ### Limitations: - [solved] Filter is computed per channel. This produces small artefacts. The problem can be solved by operating on a gray image (e.g. luminance ) to make sure only one region is used to select the relevant region. - [solved] There is no special case handling for image borders (affects one line per border). This can cause artefacts when stitching images.
Habib Gahbiche added 1 commit 2023-04-16 23:30:39 +02:00
c1fb210be9 ompositor: add new node: Kuwahara filter
The filter is used to reduce noise while preserving edges. It can be used to create a cartoon effect from photorealistic images.

The idea is to offer two modes:
1) Calssic aka isotropic kuwahara filter: simple and faster computation. Algorithm splits an area around a single pixel in four parts and selects the mean of the region with the lowest standard deviation
2) Anisotropic Kuwahara filter: improve the classical approach by considering the direction of structures of regions

This patch implements the isotropic approach as a single threaded operation for the full-frame compositor
Habib Gahbiche added 2 commits 2023-04-16 23:35:14 +02:00
Habib Gahbiche added 1 commit 2023-04-18 11:19:39 +02:00
Habib Gahbiche changed title from WIP: ompositor: add new node: Kuwahara filter to WIP: Compositor: add new node: Kuwahara filter 2023-04-18 11:46:53 +02:00
Habib Gahbiche added 1 commit 2023-04-18 16:36:29 +02:00
Sergey Sharybin added the
Module
VFX & Video
Interest
Compositing
labels 2023-04-20 09:12:57 +02:00
Sergey Sharybin added this to the Compositing project 2023-04-20 09:13:02 +02:00
Sergey Sharybin reviewed 2023-04-20 09:37:02 +02:00
Sergey Sharybin left a comment
Owner

Just a quick pass over the code.
Did not have chance to apply the patch and play with it yet, but the implementation overall seems fine for the single channel approach.

Just a quick pass over the code. Did not have chance to apply the patch and play with it yet, but the implementation overall seems fine for the single channel approach.
@ -0,0 +1,24 @@
/* SPDX-License-Identifier: GPL-2.0-or-later
* Copyright 2011 Blender Foundation. */

Use the current year for the new files added. This also applies to other new files in this PR.

Use the current year for the new files added. This also applies to other new files in this PR.
zazizizou marked this conversation as resolved
@ -0,0 +9,4 @@
{
this->add_input_socket(DataType::Color);
this->add_output_socket(DataType::Color);
this->set_kernel_size(4.4f);

This is a bit confusing. The kernel size is an integer in the API, and here we pass a floating point of 4.4f. Should this be just 4?

This is a bit confusing. The kernel size is an integer in the API, and here we pass a floating point of `4.4f`. Should this be just 4?
Author
Member

Right, the compiler shows a warning indeed. I was trying to understand where defaults get set. Will fix later

Right, the compiler shows a warning indeed. I was trying to understand where defaults get set. Will fix later
zazizizou marked this conversation as resolved
@ -0,0 +32,4 @@
float input_value[4];
image_reader_->read_sampled(input_value, x, y, sampler);
output[0] = input_value[0] + 1.0;

Is this some stub implementation for tiled system, or is it some sort of step of the actual algorithm?

If it's the former, then it's all fine for now. If that's the latter I'd like to understand how it works :)

Is this some stub implementation for tiled system, or is it some sort of step of the actual algorithm? If it's the former, then it's all fine for now. If that's the latter I'd like to understand how it works :)
Author
Member

This is a stub implementation. I was just making sure I understood the channel ordering correctly. Will remove to avoid confusion.

This is a stub implementation. I was just making sure I understood the channel ordering correctly. Will remove to avoid confusion.
zazizizou marked this conversation as resolved
@ -0,0 +80,4 @@
int xx = x + dx;
int yy = y + dy;
if (xx >= 0 && yy >= 0 && xx < area.xmax && yy < area.ymax) {

Suggest using early continue and un-indent the block.

Suggest using early `continue` and un-indent the block.
Habib Gahbiche reviewed 2023-04-23 21:07:29 +02:00
Habib Gahbiche left a comment
Author
Member

Thanks for looking into the patch @Sergey! As you noticed this is still work in progress, so it was not really ready for code review. I will upload my progress with the anisotropic filter soon. I am planning small changes to file structure and will address your comments in a separate patchset

Thanks for looking into the patch @Sergey! As you noticed this is still work in progress, so it was not really ready for code review. I will upload my progress with the anisotropic filter soon. I am planning small changes to file structure and will address your comments in a separate patchset
Habib Gahbiche added 3 commits 2023-04-25 19:30:45 +02:00
Author
Member

@blender-bot package windows

@blender-bot package windows
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR107015) when ready.
Habib Gahbiche added 1 commit 2023-04-30 07:52:16 +02:00
Author
Member

@blender-bot package windows

@blender-bot package windows
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR107015) when ready.
Habib Gahbiche added 1 commit 2023-05-01 17:40:53 +02:00
Author
Member

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR107015) when ready.
Habib Gahbiche requested review from Jeroen Bakker 2023-05-02 19:09:09 +02:00
Habib Gahbiche requested review from Sergey Sharybin 2023-05-02 19:09:09 +02:00
Sergey Sharybin reviewed 2023-05-04 11:22:56 +02:00
Sergey Sharybin left a comment
Owner

On the code side it seems good, just some cosmetic feedback.

I did not see anything immediately problematic in the code, but I am running into some artifacts when using the anisotropic node. used Einar shading test file from the Studio website. Similar blackening can be seen with other images, i.e. Lenna. Not sure if it is expected. Seems a bit odd.

The performance we might need to look into, but I would not consider it a requirement for the merge.

What we need to figure out is to how to hide the node when full-frame compositor is not used. I did not check on that yet.
Ans also maybe add a warning in the node when Tiled compositor is used. So that if someone opens file saved with full-frame has a better time figuring out why things look different?

On the code side it seems good, just some cosmetic feedback. I did not see anything immediately problematic in the code, but I am running into some artifacts when using the anisotropic node. used [Einar shading test file](https://studio.blender.org/films/charge/gallery/?asset=5969) from the Studio website. Similar blackening can be seen with other images, i.e. Lenna. Not sure if it is expected. Seems a bit odd. The performance we might need to look into, but I would not consider it a requirement for the merge. What we need to figure out is to how to hide the node when full-frame compositor is not used. I did not check on that yet. Ans also maybe add a warning in the node when Tiled compositor is used. So that if someone opens file saved with full-frame has a better time figuring out why things look different?
@ -0,0 +1,22 @@
/* SPDX-License-Identifier: GPL-2.0-or-later
* Copyright 2011 Blender Foundation. */

2023

2023
zazizizou marked this conversation as resolved
@ -69,1 +70,4 @@
void FastGaussianBlurOperation::set_size(int size_x, int size_y)
{
// todo: there should be a better way to use the operation without knowing specifics of the blur

We use C style comments, even in C++: /* ... */

We use C style comments, even in C++: `/* ... */`
zazizizou marked this conversation as resolved
@ -0,0 +19,4 @@
this->add_output_socket(DataType::Color);
// output for debug
// todo: remove

Is it something you planned to do before the actual merge?

Is it something you planned to do before the actual merge?
zazizizou marked this conversation as resolved
@ -0,0 +44,4 @@
float y,
PixelSampler sampler)
{
/* Not implemented */

Should we consider something from:

  • Pass-through the input as-is
  • Output magenta

So that if someone saves file with the Kuwahara node in it and someone opens it without enabling full-frame compositor we do not leave the output uninitialized?

Should we consider something from: - Pass-through the input as-is - Output magenta So that if someone saves file with the Kuwahara node in it and someone opens it without enabling full-frame compositor we do not leave the output uninitialized?
zazizizou marked this conversation as resolved
@ -0,0 +33,4 @@
float y,
PixelSampler sampler)
{
/* Not implemented */

Same as above.

Same as above.
zazizizou marked this conversation as resolved
@ -0,0 +6,4 @@
*/
#include "COM_node_operation.hh"
//#include "node_composite_util.hh"

Remove the commented out code.

Remove the commented out code.
zazizizou marked this conversation as resolved
Habib Gahbiche reviewed 2023-05-06 18:22:30 +02:00
@ -9245,0 +9256,4 @@
prop = RNA_def_property(srna, "size", PROP_INT, PROP_NONE);
RNA_def_property_int_sdna(prop, NULL, "size");
RNA_def_property_ui_range(prop, 4, 50, 1, -1);
Author
Member

This doesn't seem to work. User can still set values outside the specified range. This is also the case for Vector Blur. Is there another way to specify UI range?

This doesn't seem to work. User can still set values outside the specified range. This is also the case for Vector Blur. Is there another way to specify UI range?

The UI range is the soft limit. It applies when an artist drags the slider, but it is still possible to click on the property and type a value which is within a hard limit.

The hard limit is controlled by RNA_def_property_range.

You can see example in the Subsurf modifier:

RNA_def_property_range(prop, 0, 11);
RNA_def_property_ui_range(prop, 0, 6, 1, -1);
The UI range is the soft limit. It applies when an artist drags the slider, but it is still possible to click on the property and type a value which is within a hard limit. The hard limit is controlled by `RNA_def_property_range`. You can see example in the Subsurf modifier: ``` RNA_def_property_range(prop, 0, 11); RNA_def_property_ui_range(prop, 0, 6, 1, -1); ```
Author
Member

This did solve it, thanks!

This did solve it, thanks!
zazizizou marked this conversation as resolved
Habib Gahbiche added 2 commits 2023-05-06 22:20:31 +02:00
Author
Member

@Sergey it was easier to have a naive implementation for tiled compositor than figuring out how to hide the node without causing problems with versioning, so I did that : ).

I am running into some artifacts when using the anisotropic node.

There are two reasons for this:

  1. Comparing very small numbers with very large numbers. Computing weights as double instead of float seems to solve it.
  2. When filter size is smaller than the number of regions the filter is supposed to be divided into. I initially tried to address this by limiting the minimum filter size in UI, but this is apparently not working (see comment rna file).

I also solved the issue with black pixels at the border by clamping, as suggested during our meeting on Tuesday.

@Sergey it was easier to have a naive implementation for tiled compositor than figuring out how to hide the node without causing problems with versioning, so I did that : ). > I am running into some artifacts when using the anisotropic node. There are two reasons for this: 1. Comparing very small numbers with very large numbers. Computing weights as `double` instead of `float` seems to solve it. 2. When filter size is smaller than the number of regions the filter is supposed to be divided into. I initially tried to address this by limiting the minimum filter size in UI, but this is apparently not working (see comment rna file). I also solved the issue with black pixels at the border by clamping, as suggested during our meeting on Tuesday.
Author
Member

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR107015) when ready.
Habib Gahbiche changed title from WIP: Compositor: add new node: Kuwahara filter to Compositor: add new node: Kuwahara filter 2023-05-06 22:43:12 +02:00

Thanks for the update! I'll give it a closer look in a bit. Some quick replies for now.

it was easier to have a naive implementation for tiled compositor than figuring out how to hide the node without causing problems with versioning, so I did that

Lovely! It also makes it easier to merge early on :)

Comparing very small numbers with very large numbers. Computing weights as double instead of float seems to solve it.

On the CPU it is probably fine, but for the GPU it might become a problem.
But to me it seems fine to move on with doubles for now.

When filter size is smaller than the number of regions the filter is supposed to be divided into. I initially tried to address this by limiting the minimum filter size in UI, but this is apparently not working (see comment rna file).

Isn't the number of regions dependent on a frame? So not sure how we can solve it by a static limit.
Also, don't think I've touched any node settings. So maybe the default needs to be changed?

Thanks for the update! I'll give it a closer look in a bit. Some quick replies for now. > it was easier to have a naive implementation for tiled compositor than figuring out how to hide the node without causing problems with versioning, so I did that Lovely! It also makes it easier to merge early on :) > Comparing very small numbers with very large numbers. Computing weights as double instead of float seems to solve it. On the CPU it is probably fine, but for the GPU it might become a problem. But to me it seems fine to move on with doubles for now. > When filter size is smaller than the number of regions the filter is supposed to be divided into. I initially tried to address this by limiting the minimum filter size in UI, but this is apparently not working (see comment rna file). Isn't the number of regions dependent on a frame? So not sure how we can solve it by a static limit. Also, don't think I've touched any node settings. So maybe the default needs to be changed?
Member

I gave this a quick test, super nice, thanks a lot for implementing it!

Two observations:

  • I noticed that the anisotropic version seems to clamp the image at 1, while the classic doesn't. That should probably be fixed, as unclamped values are quite common with rendering.
  • I guess that is to be expected, but the anisotropic method is really significantly slower than classic, even on just 1 smoothing step. I'm not sure, but it feels like there might be some room for optimization left. I haven't looked into the exact algorithm, just an observation from using it. Even on FHD with Size 1 and Smoothing 1 it takes multiple seconds, which seems quite long to me.
I gave this a quick test, super nice, thanks a lot for implementing it! Two observations: - I noticed that the anisotropic version seems to clamp the image at 1, while the classic doesn't. That should probably be fixed, as unclamped values are quite common with rendering. - I guess that is to be expected, but the anisotropic method is really significantly slower than classic, even on just 1 smoothing step. I'm not sure, but it feels like there might be some room for optimization left. I haven't looked into the exact algorithm, just an observation from using it. Even on FHD with Size 1 and Smoothing 1 it takes multiple seconds, which seems quite long to me.
Sergey Sharybin reviewed 2023-05-11 12:39:35 +02:00
Sergey Sharybin left a comment
Owner

Finally got time to test the new build and look into the code.

I think we are pretty much there!

There are few small inlined comments, which can probably be applied in other places as well.

The bigger thing i am not sure about is the duplication of code. Unfortunately, I do not have great solution to avoid it. On the one hand maybe we can template the logic and pass pixel accessor as a template argument, but it will have other issues.

I don't think it worth obfuscating the full-frame compositor code. A bit of a hard call, but maybe it is better to accept some duplication which is locally more readable, and then remove the duplication together with the tiled compositor :)

What do you think?

Finally got time to test the new build and look into the code. I think we are pretty much there! There are few small inlined comments, which can probably be applied in other places as well. The bigger thing i am not sure about is the duplication of code. Unfortunately, I do not have great solution to avoid it. On the one hand maybe we can template the logic and pass pixel accessor as a template argument, but it will have other issues. I don't think it worth obfuscating the full-frame compositor code. A bit of a hard call, but maybe it is better to accept some duplication which is locally more readable, and then remove the duplication together with the tiled compositor :) What do you think?
@ -0,0 +102,4 @@
if (xx < 0) {
xx = 0;
}
if (xx >= this->get_width()) {

It might help making it an explicit const int height = this->get_height() outside of the loop. I am not sure compiler is smart enough to do it for us.
Although, also not suer it will give measurable time impact, but still feels like a good thing to do.

It might help making it an explicit `const int height = this->get_height()` outside of the loop. I am not sure compiler is smart enough to do it for us. Although, also not suer it will give measurable time impact, but still feels like a good thing to do.
zazizizou marked this conversation as resolved
@ -0,0 +115,4 @@
double ddx2 = (double)dx2;
double ddy2 = (double)dy2;
double theta = atan2(ddy2, ddx2) + M_PI;
int t = static_cast<int>(floor(theta / angle)) % n_div_;

For the arithmetic types you can do int(foo) https://wiki.blender.org/wiki/Style_Guide/C_Cpp#C.2B.2B_Type_Cast

Probably will make code a bit easier to read.

For the arithmetic types you can do `int(foo)` https://wiki.blender.org/wiki/Style_Guide/C_Cpp#C.2B.2B_Type_Cast Probably will make code a bit easier to read.
zazizizou marked this conversation as resolved
@ -0,0 +135,4 @@
double de = 0.0;
double nu = 0.0;
for (int i = 0; i < n_div_; i++) {
mean[i] = weight[i] != 0 ? mean[i] / weight[i] : 0.0;

In other performance critical areas we do

const float weight_inv = 1.0f / weight;
a = ... foo * weight_inv ..;
b = ... bar * weight_inv ..;
c = ... baz * weight_inv ..;

Again, not something which i know for sure will show performance impact, but might worth doing so nevertheless.

In other performance critical areas we do ``` const float weight_inv = 1.0f / weight; a = ... foo * weight_inv ..; b = ... bar * weight_inv ..; c = ... baz * weight_inv ..; ``` Again, not something which i know for sure will show performance impact, but might worth doing so nevertheless.
zazizizou marked this conversation as resolved
Author
Member

@Sergey @SimonThommes Thanks for looking into the patch!

Isn't the number of regions dependent on a frame? So not sure how we can solve it by a static limit.
Also, don't think I've touched any node settings. So maybe the default needs to be changed?

The kernel itself is split into n_div regions (see image below from the original paper), that's what I meant by regions. So if we always set n_div = 8 but kernel_size < 4, then some regions might not get covered and get set to 0. I could reproduce this problem only with anisotropic variation.

The bigger thing i am not sure about is the duplication of code.

This is the case for almost all other operations. In my opinion, I don't think it's worth doing it for all operations or even just for the new filter, when we are planning to remove the tiled implementation anyways.

I noticed that the anisotropic version seems to clamp the image at 1

You're right, I removed the clamping. The filter still won't produce values larger than those in the image itself, but now it shouldn't clamp unclamped input images.

the anisotropic method is really significantly slower than classic

I'm hearing this more often than I expected... I see improvement potential in both variations actually (see todos in code), I still want to prioritize other tasks, e.g. merging full-frame compositor, but if the speed makes the filter unusable than I might have to look into it sooner. I will bring up priorities in the next VFX meeting on Tuesday

@Sergey @SimonThommes Thanks for looking into the patch! > Isn't the number of regions dependent on a frame? So not sure how we can solve it by a static limit. Also, don't think I've touched any node settings. So maybe the default needs to be changed? The kernel itself is split into `n_div` regions (see image below from the original paper), that's what I meant by regions. So if we always set `n_div = 8` but `kernel_size < 4`, then some regions might not get covered and get set to 0. I could reproduce this problem only with anisotropic variation. <img src="https://i.ibb.co/rFfhngN/classic-vs-anisotropic.jpg " width="300" > > The bigger thing i am not sure about is the duplication of code. This is the case for almost all other operations. In my opinion, I don't think it's worth doing it for all operations or even just for the new filter, when we are planning to remove the tiled implementation anyways. > I noticed that the anisotropic version seems to clamp the image at 1 You're right, I removed the clamping. The filter still won't produce values larger than those in the image itself, but now it shouldn't clamp unclamped input images. > the anisotropic method is really significantly slower than classic I'm hearing this more often than I expected... I see improvement potential in both variations actually (see todos in code), I still want to prioritize other tasks, e.g. merging full-frame compositor, but if the speed makes the filter unusable than I might have to look into it sooner. I will bring up priorities in the next VFX meeting on Tuesday
Habib Gahbiche added 2 commits 2023-05-13 13:17:25 +02:00
Habib Gahbiche added 1 commit 2023-05-13 13:28:24 +02:00
Sergey Sharybin reviewed 2023-06-07 13:53:37 +02:00
Sergey Sharybin left a comment
Owner

There are some code style and minor performance remarks.

@zazizizou , For the efficiency of moving forward do you mind if i address them in your branch, so that we can merge early on?

I was also experimenting with replacing the per-channel look using float3 and double3. The naive conversion gave 2x speedup. The unfortunate thing is that I will need to first commit some improvements to the BLI_math to implement some missing functionality. And in any case, it would be nice to have the naive implementation first, so that we can create regression test and verify the "vectorized" implementation. I can take care of that once the patch is merged.

There are some code style and minor performance remarks. @zazizizou , For the efficiency of moving forward do you mind if i address them in your branch, so that we can merge early on? I was also experimenting with replacing the per-channel look using `float3` and `double3`. The naive conversion gave 2x speedup. The unfortunate thing is that I will need to first commit some improvements to the BLI_math to implement some missing functionality. And in any case, it would be nice to have the naive implementation first, so that we can create regression test and verify the "vectorized" implementation. I can take care of that once the patch is merged.
@ -0,0 +24,4 @@
converter.add_operation(operation);
converter.map_input_socket(get_input_socket(0), operation->get_input_socket(0));
converter.map_output_socket(get_output_socket(0), operation->get_output_socket());
} break;
`break` should be inside of the block: https://wiki.blender.org/wiki/Style_Guide/C_Cpp#Operators_and_Statements
zazizizou marked this conversation as resolved
@ -0,0 +231,4 @@
for (int ch = 0; ch < 3; ch++) {
Vector<double> mean(n_div_, 0.0f);

I've pretty much skipped the tiled implementation, but same note can be applied there.

I think we should move allocations to as high level as possible.

I've pretty much skipped the tiled implementation, but same note can be applied there. I think we should move allocations to as high level as possible.
zazizizou marked this conversation as resolved
@ -0,0 +251,4 @@
int xx = x + dx2;
int yy = y + dy2;
/* Clamp image to avoid artefacts at borders. */

Think this can be shortened to:

const int xx = math::clamp(x + dx2, 0, width - 1);
const int yy = math::clamp(y + dy2, 0, height - 1);

(add #include BLI_math_base.h at the top of the file).

Think this can be shortened to: ``` const int xx = math::clamp(x + dx2, 0, width - 1); const int yy = math::clamp(y + dy2, 0, height - 1); ``` (add `#include BLI_math_base.h` at the top of the file).
zazizizou marked this conversation as resolved
@ -0,0 +3,4 @@
#include "COM_KuwaharaClassicOperation.h"
extern "C" {

No need to `extern "C"`` here, the header does it already.

Generally, we should not be adding such statements around #include statements, as it makes it very easy to break things when things move to C++. Some headers might need the linking specializer, and for those it is to be changed in the header itself.

No need to `extern "C"`` here, the header does it already. Generally, we should not be adding such statements around `#include` statements, as it makes it very easy to break things when things move to C++. Some headers might need the linking specializer, and for those it is to be changed in the header itself.
zazizizou marked this conversation as resolved
Author
Member

@Sergey thanks for the tips. I updated the patch with the minor changes. Sure, feel free to push further changes! I am planning to look into the performance this weekend.

@Sergey thanks for the tips. I updated the patch with the minor changes. Sure, feel free to push further changes! I am planning to look into the performance this weekend.
Habib Gahbiche added 3 commits 2023-06-08 10:03:01 +02:00
Sergey Sharybin added 4 commits 2023-06-08 15:03:48 +02:00
d89739f3d5 Cleanup: carious changes to match the code style
- Add an empty line after first include.

  The idea is to include own header first, so that it is a bit
  better guaranteed that the header is self-sufficient. In order
  to keep the order an empty line is needed, so that clang-format
  does not move the own include to the bottom to keep an alphabetic
  order.

- Full stop in comments.

- Remove full-stop from the RNA description.

  There is warning about it in debug builds.
401ea11d47 Cleanup: Revert changes in SMAA operation
The changes themselves are good, but is not a part of the
Kuwahara project. Need to somehow remember to re-apply the
changes in the main branch.
Sergey Sharybin approved these changes 2023-06-08 15:06:30 +02:00
Sergey Sharybin left a comment
Owner

I did some final round of cleanup.

As mentioned before, there there are some unideal aspects, but I would like to solve them incrementally from the main branch. This will include:

  • Code duplication (it will be solved by tile compositor removal)
  • Performance optimizations

I already have some local code for the performance, but I'd really like to first have a more simple/naive and tested implementation in-place to generate regression files.

I did some final round of cleanup. As mentioned before, there there are some unideal aspects, but I would like to solve them incrementally from the main branch. This will include: - Code duplication (it will be solved by tile compositor removal) - Performance optimizations I already have some local code for the performance, but I'd really like to first have a more simple/naive and tested implementation in-place to generate regression files.
Author
Member

Thank you @Sergey ! I prepared some files for regression testing already, I will commit them to svn after submitting this patch

Thank you @Sergey ! I prepared some files for regression testing already, I will commit them to svn after submitting this patch
Habib Gahbiche merged commit f3cb157452 into main 2023-06-08 16:14:51 +02:00
Habib Gahbiche deleted branch com-kuwahara-filter-node 2023-06-08 16:14:52 +02:00
Sergey Sharybin removed this from the Compositing project 2023-07-06 14:24:42 +02:00
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
4 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#107015
No description provided.