-
Notifications
You must be signed in to change notification settings - Fork 132
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Anisotropic Kuwahara Filter #68
base: master
Are you sure you want to change the base?
Conversation
- New StructuredTensor mode in the EdgeDetect node to output the StructuredTensor - New LineIntegralConvolution node to compute Line Integral Convolution (imaging vector field using flow lines) - New GaussianKernel node that compute and output a ComputeBuffer based on a kernel radius and sigma - New Edge Tangent Flow node that compute the Vector Field (TFM) - New Anisotropic Kuwahara Filter Node that process the painterly filter - New CompareTextures node (only shader, need a Shader Node with the shader assign) TODO : - Add Kuwahara Parameters as Input - Cleanup code - Add Heightmap Generation and composition with Kuwahara Output
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of questions and minor comments, otherwise it's good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the license of this image?
public override Texture previewTexture => output; | ||
public override bool showDefaultInspector => true; | ||
[Input("Source")] public Texture source; | ||
[Input("TFM")] public Texture tfm; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try to avoid using abbreviations in node parameters :)
if (!base.ProcessNode(cmd) || tfm == null || source == null) | ||
return false; | ||
ValidateTempRenderTexture(); | ||
for (int i = 0; i < passCount; i++) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is an acceptable range of value for the pass count? It's to know if it's worth avoiding the copy inside the loop or not.
namespace Mixture | ||
{ | ||
[System.Serializable, NodeMenuItem("Operator/Edge Tengent Flow")] | ||
public class EdgeTengentFlow : FixedShaderNode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you write a bit of documentation on this node using the [Documentation] attribute?
namespace Mixture | ||
{ | ||
[System.Serializable][NodeMenuItem("Operators/Anisotropic Kuwahara Filter")] | ||
public class AnisotropicKuwaharaFilter : ComputeShaderNode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you write a bit of documentation on this node using the [Documentation] attribute?
{ | ||
var kernel = OneDimensinalKernel(gaussianRadius, gaussianSigma);// GenerateGaussianKernel(gaussianRadius, gaussianSigma); | ||
var buffer = new ComputeBuffer(kernel.Length, sizeof(float), ComputeBufferType.Default); | ||
buffer.SetData(kernel); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use CommandBuffer.SetBufferData instead otherwise the execution of the graph and the data will be out of sync.
Alternatively, you can also dispatch a compute that updates this buffer.
float2 v = mul(SR, float2(x, y)); | ||
if (dot(v, v) <= 0.25f) | ||
{ | ||
float3 c = _Source[id.xy + int2(x, y)].rgb; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a particular reason to not use a sampler for reading source values?
output += float4(m[k].rgb * w, w); | ||
} | ||
|
||
_Output[id.xy] = saturate(output / output.w); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Saturate clamps all values outside of 0;1 range, which implicitly means that this algorithm doesn't work outside this range (HDR). Is that correct?
half4 sumWeight = 0.0h; | ||
|
||
for (int xi = 1; xi <= __KernelRadius; xi++){ | ||
half2 G = __TFM.SampleLevel(point_clamp_sampler, position, 0).xy; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For point sampling Load() is faster, unless you're using the clamp functionality of the sampler in case position goes outside of the [0; 1] bounds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this shader used?
I think this is something that already exists with the Difference node
Thanks for the review ! I still have some features of the nodes to implement and I will make sure to follow your guidelines before asking for review :) |
TODO :