I really like the results. However I have concerns about the implementation.

The first 2 nodes can be done with relatively simple nodegroups. They break PBR badly but still use Closures as inputs and outputs.
Closures are not to be treated as simple RGBA that's the whole point of having the conversion.
Also they won't work in Cycles.
So I'm not really ok with adding them. We need to keep things somewhat consistent and predictable. Your nodes don't suggest they break PBR and loose the screenspace datas.

For the Closure to RGBA conversion see my comment.

In D3205#75791, @Clément Foucault (fclem) wrote:

I really like the results. However I have concerns about the implementation.

The first 2 nodes can be done with relatively simple nodegroups. They break PBR badly but still use Closures as inputs and outputs.
Closures are not to be treated as simple RGBA that's the whole point of having the conversion.
Also they won't work in Cycles.
So I'm not really ok with adding them. We need to keep things somewhat consistent and predictable. Your nodes don't suggest they break PBR and loose the screenspace datas.

For the Closure to RGBA conversion see my comment.

I see now, rereading the source it seems EEVEE Closures have some extra data fields. I did not read properly and I thought SSR was still SSS. I will work on the implementation some more to not break the screen-space data.

Slice Closure can indeed be implemented with a bunch of nodes and Closure to RGBA, but the half lambert needs setting the eevee_closure_diffuse AO paramater to 0 to work correctly. Also, let's say Slice Closure gets turned into a node group instead of an actual node. How would it be usable by default then? Blender doesn't have a set of default node groups that are always present.

Are the first two nodes not to be accepted at all? Or would they be fine if the implementation is fixed? I guess the challenge is implementing NPR stuff in a PBR engine without breaking things...

In D3205#75797, @Kanzaki Wataru (kanzwataru) wrote:

Are the first two nodes not to be accepted at all? Or would they be fine if the implementation is fixed? I guess the challenge is implementing NPR stuff in a PBR engine without breaking things...

I think it would be better to distribute them as groupnode inside an addon once the closure to rgba conversion is handled.

In D3205#75839, @Clément Foucault (fclem) wrote:

In D3205#75797, @Kanzaki Wataru (kanzwataru) wrote:

Are the first two nodes not to be accepted at all? Or would they be fine if the implementation is fixed? I guess the challenge is implementing NPR stuff in a PBR engine without breaking things...

I think it would be better to distribute them as groupnode inside an addon once the closure to rgba conversion is handled.

Alright so in that case I will focus on the Shader to RGBA and see about bundling the other two nodes inside of an official addon. I guess the extranous ambient light issue with the Diffuse BSDF will just have to be worked around by (the user) setting the world background colour to black.

@Clément Foucault (fclem)

So I took a look at fallback_cubemap() and how it evaluates the screen space inputs, but it seems that it depends on a lot of things that get calculated in the main() function. Is there a way to split this out into its own function so that it can be cleanly called from the Shader to RGBA node without duplication?

In D3205#78002, @Kanzaki Wataru (kanzwataru) wrote:

@Clément Foucault (fclem)

So I took a look at fallback_cubemap() and how it evaluates the screen space inputs, but it seems that it depends on a lot of things that get calculated in the main() function. Is there a way to split this out into its own function so that it can be cleanly called from the Shader to RGBA node without duplication?

void fallback_cubemap(vec3 N, vec3 V, vec3 W, vec3 viewPosition, float roughness, float roughnessSquared, inout vec4 spec_accum)

V is world space view vector.
W is world space position.
viewPosition is view space position.
All of these are just varyings attributes converted (or not) to world space, but you do need to recompute them yes.

N is world space normal vector.
You just need to decode the normal from the SSR parameters and convert to world space orientation.

roughness and roughnessSquared can be derived from the SSR inputs.

There will be no redundancy because all the inputs needs to precomputed from different sources.

In D3205#78024, @Clément Foucault (fclem) wrote:

In D3205#78002, @Kanzaki Wataru (kanzwataru) wrote:

@Clément Foucault (fclem)

So I took a look at fallback_cubemap() and how it evaluates the screen space inputs, but it seems that it depends on a lot of things that get calculated in the main() function. Is there a way to split this out into its own function so that it can be cleanly called from the Shader to RGBA node without duplication?

void fallback_cubemap(vec3 N, vec3 V, vec3 W, vec3 viewPosition, float roughness, float roughnessSquared, inout vec4 spec_accum)

V is world space view vector.
W is world space position.
viewPosition is view space position.
All of these are just varyings attributes converted (or not) to world space, but you do need to recompute them yes.

N is world space normal vector.
You just need to decode the normal from the SSR parameters and convert to world space orientation.

roughness and roughnessSquared can be derived from the SSR inputs.

There will be no redundancy because all the inputs needs to precomputed from different sources.

Thanks for the hint, but I admit this SSR stuff is out of my league. From what I gather, the SSR reflections/refractions are a post-process effect? I'm not sure how I can use the fallback_cubemap(). I understand that in theory I need to calculate the SSR and bake it into the output in node_shadertorgb(), but I'm having some trouble seeing the big picture.

In D3205#78057, @Kanzaki Wataru (kanzwataru) wrote:

Thanks for the hint, but I admit this SSR stuff is out of my league. From what I gather, the SSR reflections/refractions are a post-process effect? I'm not sure how I can use the fallback_cubemap(). I understand that in theory I need to calculate the SSR and bake it into the output in node_shadertorgb(), but I'm having some trouble seeing the big picture.

I don't want you to do the SSR stuff but only to compute the fallback lighting when doing the conversion in node_shadertorgb().

The fallback function can be shared by putting it somewhere like lightprobe_lib.glsl.

fallback_cubemap basically evaluate all reflections probes. So you want to evaluate them with what should have been the SSR data sent to the post process pass.

In D3205#78068, @Clément Foucault (fclem) wrote:

In D3205#78057, @Kanzaki Wataru (kanzwataru) wrote:

Thanks for the hint, but I admit this SSR stuff is out of my league. From what I gather, the SSR reflections/refractions are a post-process effect? I'm not sure how I can use the fallback_cubemap(). I understand that in theory I need to calculate the SSR and bake it into the output in node_shadertorgb(), but I'm having some trouble seeing the big picture.

I don't want you to do the SSR stuff but only to compute the fallback lighting when doing the conversion in node_shadertorgb().

The fallback function can be shared by putting it somewhere like lightprobe_lib.glsl.

fallback_cubemap basically evaluate all reflections probes. So you want to evaluate them with what should have been the SSR data sent to the post process pass.

Here's what I've got so far, which is no doubt wrong since I don't understand how this all fits together. What is spec_accum supposed to be, and where do the rgb parts of ssr_data come into play? Am I supposed to be doing something like what is in lit_surface_frag.glsl? What is ssr_id and what am I supposed to get back from the spec_accum output of the fallback_cubemap()?

Just so I know what to look for, I assume that if I do it correctly I'm supposed to get working reflections if I plug a Glossy BSDF into Shader to RGBA?

void node_shadertorgb(Closure cl, out vec4 outcol, out float outalpha)
{
	vec3 V = cameraVec;
	vec3 vN = normal_decode(cl.ssr_normal, viewCameraVec);
	vec3 N = transform_direction(ViewMatrixInverse, vN);
	float roughness = cl.ssr_data.a;
	float roughnessSquared = max(1e-3, roughness * roughness);
	
	vec4 spec_accum = vec4(0.0);
	fallback_cubemap(N, V, worldPosition, viewPosition, roughness, roughnessSquared, spec_accum);

	outcol = vec4(spec_accum.rgb + cl.radiance, 1.0);
	outalpha = cl.opacity;
}

(I'm so sorry to keep sending stupid questions!)

In D3205#78077, @Kanzaki Wataru (kanzwataru) wrote:

Here's what I've got so far, which is no doubt wrong since I don't understand how this all fits together.
What is spec_accum supposed to be, and where do the rgb parts of ssr_data come into play?

spec_accum is the output specular of this function. It's an inout arg because you can feed it with an already existing specular data (to blend with SSR). In this case, you did well by passing vec4(0.0).
The rgb part of ssr_data is the "glossy color" you must multiply the result of fallback_cubemap by it.

Am I supposed to be doing something like what is in lit_surface_frag.glsl?

I'm not quite sure I understand. fallback_cubemap is replacing the use of lit_surface_frag.glsl by itself.

What is ssr_id and what am I supposed to get back from the spec_accum output of the fallback_cubemap()?

ssr_id is a tag to only compute one bsdf for the SSR pass. You should do the fallback_cubemap only if (ssrToggle && ssr_id == outputSsrId) otherwise, the lighting is already computed in lit_surface_frag.glsl.
spec_accum is in fact the received specular light in a specific direction.

Just so I know what to look for, I assume that if I do it correctly I'm supposed to get working reflections if I plug a Glossy BSDF into Shader to RGBA?

Yes, but only probe reflections, since SSR are computed in Post FX and we are on purpose bypassing them.

void node_shadertorgb(Closure cl, out vec4 outcol, out float outalpha)
{
	vec3 V = cameraVec;
	vec3 vN = normal_decode(cl.ssr_normal, viewCameraVec);
	vec3 N = transform_direction(ViewMatrixInverse, vN);
	float roughness = cl.ssr_data.a;
	float roughnessSquared = max(1e-3, roughness * roughness);
	
	vec4 spec_accum = vec4(0.0);
	fallback_cubemap(N, V, worldPosition, viewPosition, roughness, roughnessSquared, spec_accum);

	outcol = vec4(spec_accum.rgb + cl.radiance, 1.0);
	outalpha = cl.opacity;
}

Just missing the few thing I wrote above and it's good :).

(I'm so sorry to keep sending stupid questions!)

Don't be! If my code is not easily understandable, i'm all to blame!

In D3205#78123, @Kanzaki Wataru (kanzwataru) wrote:

I'm not sure if this is correct yet, and I think I'm still missing SSS and an ifdef for whether EEVEE is on or not.

Looks correct to me. If you upload the patch using arcanist I will test it. And yes missing the check and the SSS. But SSS is just adding the SSS radiance (or SSS radiance * SSS color in the case of separate albedo) to the overall radiance.

As for the add-on to add the other two nodes as node-groups, I'm guessing that needs to be in a different patch?

Yes as it's more to addons repo maintainers and other coredevs to have an opinion on it.

I think that ssrToggle in gpu_shader_material.glsl is unnecessary.

Why don't you move cubemap code into if clause. This avoid normal decode and transform when no need to do that.

void node_shadertorgb(Closure cl, out vec4 outcol, out float outalpha)
{
	outcol = vec4(cl.radiance, 1.0);
	outalpha = cl.opacity;

	// add cubemap
	if (cl.ssr_id == outputSsrId) {
		vec3 V = cameraVec;
		vec3 vN = normal_decode(cl.ssr_normal, viewCameraVec);
		vec3 N = transform_direction(ViewMatrixInverse, vN);
		float roughness = cl.ssr_data.a;
		float roughnessSquared = max(1e-3, roughness * roughness);
		
		vec4 spec_accum = vec4(0.0);
		fallback_cubemap(N, V, worldPosition, viewPosition, roughness, roughnessSquared, spec_accum);
		outcol += vec4(spec_accum.rgb * cl.ssr_data.rgb, 0.0);
	}
}

In D3205#78174, @Kinouti Takahiro (dskjal) wrote:

I think that ssrToggle in gpu_shader_material.glsl is unnecessary.

Why don't you move cubemap code into if clause. This avoid normal decode and transform when no need to do that.

Good idea. But outcol still needs ssr_data.rgb and sss_data added to it. I'll update the patch.

@Clément Foucault (fclem)

What should I do for the ifdef? There doesn't seem to be anywhere else in the code that detects if EEVEE is used or not.

In D3205#78176, @Kanzaki Wataru (kanzwataru) wrote:

In D3205#78174, @Kinouti Takahiro (dskjal) wrote:

I think that ssrToggle in gpu_shader_material.glsl is unnecessary.

Why don't you move cubemap code into if clause. This avoid normal decode and transform when no need to do that.

Good idea. But outcol still needs ssr_data.rgb and sss_data added to it. I'll update the patch.

If cl.ssr_id != outputSsrId, spec_accum is zero vector because of the initialization. Thus spec_accum.rgb * cl.ssr_data.rgb is also zero vector.

But the cost of this code is not problem. Please select the one you like.

In D3205#78192, @Clément Foucault (fclem) wrote:

In D3205#78183, @Kanzaki Wataru (kanzwataru) wrote:

@Clément Foucault (fclem)

What should I do for the ifdef? There doesn't seem to be anywhere else in the code that detects if EEVEE is used or not.

Well after searching, it seems these #ifdef were removed at some point so I believed it's not usefull to add them back now.

So I think I'll accept the patch as is.

Good job and thank you for your contribution :) will commit this tomorrow after final review.

Thanks for your help in figuring it out!
One last question. Where should I discuss the addon? It seems node groups are contained inside of .blend files and aren't global, so I'm not sure how to approach bundling them into an addon in a way that matches the Blender design.

I have found a bug. Following code in gpu_shader_material.glsl

outcol += (cl.sss_data * cl.sss_albedo);

should be as follow.

outcol += vec4(cl.sss_data.rgb * cl.sss_albedo, 0.0);

In D3205#78827, @Kinouti Takahiro (dskjal) wrote:

I have found a bug. Following code in gpu_shader_material.glsl

outcol += (cl.sss_data * cl.sss_albedo);

should be as follow.

outcol += vec4(cl.sss_data.rgb * cl.sss_albedo, 0.0);

yep fixed by rB83c2febaeeb2