Provide a way to pass variables from fragment to light shader function

[Chaosus]

Describe the project you are working on

Not me, but the original issue from godotengine/godot#18460

Describe the problem or limitation you are having in your project

It's impossible to pass a variable from fragment to light. The analog of this is varying but it would work from vertex->fragment/light and this proposal is for fragment->light. Since light internally called from the fragment function I think it's completely possible to compile it with arguments.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

There are multiple ways to solve it by introducing new mechanics, which described below.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

I came up with these ways, of course, they may not be ideal and other contributors may provide their own solutions:

1. By introducing a local varying mechanism to be used inside fragment function:

void fragment() {
	varying vec3 v = vec3(1, 1, 1);
}

void light() {
	DIFFUSE_LIGHT = v;
}

2. By implementing optional parameters parsing in the light function:

void fragment() {
	v = vec3(1, 1, 1);
}

void light(vec3 v) {
	DIFFUSE_LIGHT = v;
}

3. A separate hint for actual varyings:

varying vec3 v : hint_fragment_to_light;

void vertex() {
   v = vec3(1, 1, 1); // ERROR - initialization of this varying is only available in fragment function
}

void fragment() {
	v = vec3(1, 1, 1);
}

void light() {
	DIFFUSE_LIGHT = v;
    v = vec3(1, 1, 1); // setting here is also possible
}

If this enhancement will not be used often, can it be worked around with a few lines of script?

Nope

Is there a reason why this should be core and not an add-on in the asset library?

No way to do it outside

[Chaosus]

If I decide to implement one of them - I need to sure which one of these ways I need to choose, that why I will wait for the opinions of other contributors before.

[clayjohn]

Supercedes: #1057

[vitorbalbio]

Thanks for take a look on it. It's very important and a current limitation that is hard to overcome. AFAIK Varying is defined by opengl as vertex to fragment only. If it's true i don't think it's a good idea to make it also be used in Godot's syntax with other meaning. My suggestion would be follow the option 2 but I don't know how flexible it would be. How it could be possible to pass any number of parameters in this (2) case?

If it is not flexible in the number of parameters it would be possible to pass a user defined struct?

struct f2l {
   float var1;
   fixed var2;
};
            
void fragment(){
   v2f o;
   o.var1 = v.normal * 0.5 + 0.5;
   o.var2 = 1.0
}

 void light( f2l o){
 COLOR = vec4(o.var1,o.var2,1,1);
}

I'm not a Godot developer, only a user. So consider it only a rough suggestion.

[clayjohn]

@vitorbalbio that is very similar to an idea I had. We could do a ftl struct without the user having to declare it in the light function. We would just use a default name and allow the user to fill it as they want.

My concern with both option 2 and the struct idea is that both are inconsistent with the way that varyings are treated. A huge pain point from users is that they don't understand how the relationship between vertex-fragment is different from the relationship between fragment-light.

Therefore, whichever option we choose, we should make it consistent with varyings. For 4.0, this could mean redesigning varyings.

[Megalomaniak]

Not like my voice would carry a lot of weight on this issue, but I'd really prefer nr.2 as a user. K.I.S.S. and all that.

[clayjohn]

Not like my voice would carry a lot of weight on this issue, but I'd really prefer nr.2 as a user. K.I.S.S. and all that.

@Megalomaniak Do you mind explaining why number 2 appears to be the simplest from a user perspective? Also, as an aside, we really need and value users perspectives on things like this. Oftentimes the most challenging piece for us is figuring out what makes sense to users.

CC @lyuma Who I know has a special interest in this as well.

My proposal:

As above, I think the most consistent API choice would be to allow:

1. The fragment shader to read and write to varyings AND
2. for varyings to be accessible from the light shader.

This would be a movement away from how varyings in GLSL work, but I think it presents a simpler and more consistent API to users. I am uncomfortable adding new syntax just to solve this one problem.

API

A simple example is:

varying vec3 v ;

void vertex() {
       v = vec3(1, 1, 1);  //if written to, passes to the fragment and light functions, but does not need to be initialized here. 
}

void fragment() {
	v = vec3(0, 0, 0);
}

void light() {
	DIFFUSE_LIGHT = v; //Diffuse light set to vec3(0, 0, 0)
        v = vec3(1, 1, 1); // setting here is also possible, but does nothing
}

Implementation

In GLSL the above example wouldn't work. However, Godot runs a pre-compiler that changes variable names already (for example, internally v becomes m_v. So making this work is surprisingly simple. In the fragment function the pre-compiler just replaces v with frag_v (or something similar) and initializes frag_v to the value of v from the vertex shader. Now, in the fragment shader, instead of replacing v with m_v, the compiler just replaces it with frag_v. This allows the user to both read and write to v. Finally, the pre-compiler adds all varyings as extra arguments to the light function (or optionally, just adds all the varyings that are used in the light function).

The one part I don't know how to implement is the error detection. However, allowing the use of varyings in the light processor function will be challenge regardless of the approach and I trust that @Chaosus knows how to do that much better than I do.

[Megalomaniak]

Well unless I'm misunderstanding it option 2 means I can just use variables/values defined in the previous "fragment step" in the light shader(next "fragment shader step"), no extra work on my part involved as a user. As I said K.I.S.S. Least steps to take on the users part, no extra lines needed to define some pseudo varying or anything.

In terms of clarity/user confusion I don't think it is any better or worse. Those familiar with OpenGL might well know that lighting is just another part of fragment processing but those new to shaders might indeed wonder why varying is needed to pass something from vertex to fragment but then not from fragment to lighting.

For the inquiring minds probably good to have some explanation of it in the documentation but I don't think more than that is strictly necessary.

[clayjohn]

Well unless I'm misunderstanding it option 2

You are missing that option two requires defining the variable as a parameter to the light function.

[Megalomaniak]

Ah, so I did.

[vitorbalbio]

In GLSL the above example wouldn't work. However, Godot runs a pre-compiler that changes variable names already

@clayjohn if it's allowed why not move away from the "varying" keyword at once and just add the more generic "var" keyword to variables in Godot shader language? It would at least make it more consistent with GDScript. Of course varying could be kept to backward compatibility.

Anyway I really would like to see any of these implemented even if broke compatibility with vanilla GLSL.

[Zireael07]

No, please do not move away from GLSL even further - many users adapt Unity or Shadertoy shaders to Godot.

[Chaosus]

I personally like 3) - it's not very different from what @clayjohn suggested while kept distance from GLSL by using a hint (maybe a limitation of setting it in vertex function should be removed).

[lyuma]

I am with @Zireael07 in that I do not see the reason to diverge from GLSL syntax.

I discussed this on the Discord, but GLSL already provides support and syntax for the above usecase that does exactly what you propose: top level scope temporary variables (not to be confused with uniform, these are called globals, but only valid within a single pixel or vertex) Basically, the same as proposed but without the word "varying", and it is valid GLSL, too.

This is far easier to implement than a strange overload of the term "varying", as it requires no custom semantics when the variable is accessed differently in one place and another. I have already implemented this, and you can see my implementation here:
https://github.com/lyuma/godot/tree/shader_globals_master

With this implementation, passing variables to light() is easy:

vec3 v = vec3(0.0);

void vertex() {
   v = vec3(1, 1, 1); // No effect, as the vertex program is a separate shader invocation than fragment()
}

void fragment() {
	v = vec3(1, 1, 1);
}

void light() {
	DIFFUSE_LIGHT = v;
    v = vec3(1, 1, 1); // setting here is also possible
}

About safety, global variables can be initialized in case they are never assigned.

If you do intend to diverge from GLSL syntax for the sake of readability, I would follow HLSL and use the static keyword for this purpose, as this is effectively what static does. You could also make up a new keyword like toplevel. But using varying for this is wrong, as these are not proper varying variables. It may be desired to hold arrays or struct data here; and it may be undesired to waste varyings for data being passed from fragment to light.

Note that other functions may wish to share variables. This should be considered as well in these proposals. For example fragment() and light() might both call a function that references a particular shared variable.

[Chaosus]

@lyuma The problem is the @reduz is against global variables in Godot shader language due to confusion they create between stages (since they internally to be created with duplication)

You could try over-persuade him of course. )

[lyuma]

I'd like to object to the above PR.

First, I'll to respond to the comment by @Chaosus about globals: I understand and I'll concede reduz's argument about mixing of vertex and fragment state for now. I still think users are intelligent enough to know which stage they are in, and when varying is needed, and I'd be willing to debate that another time. But anyway, I will proceed for the purposes of this proposal assuming global variables are not an option.

My chief complaint about the proposed PR is the overloaded use of the varying keyword. Assuming the compiler is smart enough to optimize out the extra unused varying interpolators, I still object on the basis that it's not only non-standard, but non-obviously so. I also think that implementation could have subtle bugs in some cases. Additionally, varying in the PR does not allow struct, but I think fragment-to-light should allow struct types, because no reason not to.

Ok, so with that out of the way, what do I propose:

A: Simplest modification of the above proposal: use a keyword other than varying: maybe light_input or frag_out or frag_to_light or light_in etc. If a change to some such keyword is made, I'd be okay with this proposed change. I'd also appreciate if the new keyword allows struct variables.

But now let's consider for a bit what this "fragment-to-light" proposal is actually doing: it's really trying to output data from the fragment function and feed arguments to the light function. If we view this proposal semantically, this is best thought of as function arguments. Option 2 above therefore feels much better to me and less cobbled together, than Option 3 which was implemented in the PR.

B: So I'll propose a slight modification to Option 2: instead of variable v existing in the scope of fragment() by magic, I'd rather have it explicit, using an out keyword. Consider this possibility:

struct FragmentOutput {
    vec4 v;
};

void fragment(out FragmentOutput fOut) {
    fOut.v = vec4(4.0);
    detailAlbedoColor = vec4(1.0);
}

void light(FragmentOutput fOut) {
    DIFFUSE_LIGHT = fOut.v;
}

Note that it would also be possible for the user to write out arugments directly, without use of struct, for example void fragment(out vec4 detailAlbedoColor)

[clayjohn]

@lyuma I am unconvinced by your objections to using the varying keyword. We can easily optimize out the vertex varying when not used. Even if that was a challenge, then we can provide a type hint as chaosus originally proposed. (Note for other reasons, low end devices have a limit of 15 or so varyings, so there is good reason to keep the number down)

This leaves your objections as follows:

I still object on the basis that it's not only non-standard, but non-obviously so. I also think that implementation could have subtle bugs in some cases. Additionally, varying in the PR does not allow struct, but I think fragment-to-light should allow struct types, because no reason not to.

None of these justify adding a new, complex API to accomplish a task that users are already familiar with. Keep in mind, Godot seeks to make things easy for beginners. More important than anything else for us is to keep things consistent and accessible. Introducing an entirely new API to pass data from fragment to light is going to confuse users immensely and adds no obvious benefit.

[lyuma]

I mostly just think it's misleading. There isn't a magical third shader stage called "light": data passed from fragment to light is not varying, in the GPU sense. It's not written per vertex and then interpolated, but it's written and read for the same pixel. So in my mind the varying keyword doesn't make sense and seems confusing, even for beginners.

Also, looking at the way it was implemented just seems ripe for mistakes and bugs. Using the same keyword for two different purposes has made the parser more complicated. What happens if the compiler reads from the struct copy when it should have read from the varying copy, and vice versa? For example, I'm not sure what will happen if a varying value is used from a function called by fragment(). We wouldn't even be here talking about a 15 varying limit if any other keyword had been used.

So, I'd be ok with this change if we make this particular case use a different keyword. In terms of the code, you could even replace any (x == "varying") with (x == "varying" || x == "lightdata"). I'm not ok with a hint because it doesn't address the other concerns I had.

About the last argument, is this really about making it easy for some hypothetical beginners? The light function requires about 30 lines of boilerplate code ported from glsl just to replicate the hardcoded default diffuse lambert and specular schlick_ggx lighting. To improve things for beginners, we should work on making some lighting templates or expose some utility functions.

[clayjohn]

I mostly just think it's misleading. There isn't a magical third shader stage called "light": data passed from fragment to light is not varying, in the GPU sense. It's not written per vertex and then interpolated, but it's written and read for the same pixel. So in my mind the varying keyword doesn't make sense and seems confusing, even for beginners.

@lyuma For most users, the Godot Shader Language is their first exposure to shaders and in Godot light is a magical third shader stage. Users don't care what the underlying GLSL code looks like. The fact that varying is only used for passing data from vertex to fragment in GLSL is irrelevant. It is vastly more important for us to create an API that logically extends what we have, is consistent, and is intuitive for new users.

Also, looking at the way it was implemented just seems ripe for mistakes and bugs. Using the same keyword for two different purposes has made the parser more complicated. What happens if the compiler reads from the struct copy when it should have read from the varying copy, and vice versa? For example, I'm not sure what will happen if a varying value is used from a function called by fragment(). We wouldn't even be here talking about a 15 varying limit if any other keyword had been used.

You seem to still be hung up on the idea of using global variables. If a user tries to use the varying outside of the processor functions it will just throw an error because the variable wont be defined in that scope.

So, I'd be ok with this change if we make this particular case use a different keyword. In terms of the code, you could even replace any (x == "varying") with (x == "varying" || x == "lightdata"). I'm not ok with a hint because it doesn't address the other concerns I had.

I like the idea of replacing varying with another more descriptive name, we could consider vertex_out and frag_out. But frankly I'm not sure that gets us anywhere better than just using varying.

About the last argument, is this really about making it easy for some hypothetical beginners? The light function requires about 30 lines of boilerplate code ported from glsl just to replicate the hardcoded default diffuse lambert and specular schlick_ggx lighting. To improve things for beginners, we should work on making some lighting templates or expose some utility functions.

I don't disagree, but this is way outside the scope of this proposal. This proposal solves the issue of users wanting access to more custom information during the light pass.

[Megalomaniak]

Don't forget that in legacy fixed function pipeline lighting was a "magic third stage".

[s-ilent]

Users don't care what the underlying GLSL code looks like.
It is vastly more important for us to create an API that logically extends what we have, is consistent, and is intuitive for new users.

I don't really understand your argument here. Let me explain why.

When users learn to code shaders, how many of the resources are they going to find that relate specifically to Godot shader coding?

How many of them are going to cover this piece of syntax or any other special sauce added to the Godot shader system?

If a user has a problem, where are they going to search for answers? Godot is not the only platform that supports shaders.

It doesn't matter what a hypothetical user who starts off reading the Godot docs thinks, because as soon as they type a query into a search engine, they are getting results that relate to the wider HLSL/GLSL world, and then they do care about the underlying code.

More important than anything else for us is to keep things consistent and accessible. Introducing an entirely new API to pass data from fragment to light is going to confuse users immensely

What I'm reading from this is that you believe users will find an explicit declaration harder to understand than one that is implicit. Can you explain your reasoning here?

What is introduced here acts via side-effects - there's nothing in the fragment or light functions to indicate that this variable is in their scope.

It is good to provide users the opportunity to learn more about things that are useful in a wider context, like structs. But what you're arguing for seems to goes against convention in both Godot and the outside world.

[vitorbalbio]

Personally as a technical artist I'm quite against use the "varying" keyword for this if other options are on the table. Part of the "easy to learn" aspect for newcomers is to have access to many sources of materials and make Godot a exception about how it define varying from the rest of the world seems to be a bad idea. We can make a Godot only new syntax, new variable declaration type, new struct but a new meaning for a basic stuff like varying seem just wrong. That said, I will be happy in just have a solution, any solution.

[clayjohn]

Well, I'm happy to discuss an improved keyword over varying.

GLSL hasn't used varyings in ages. Using varying with GL 3.3+ is something Godot specific.

The argument that users can build off of GLSL tutorials isn't really accurate. Using non-godot tutorials would only be helpful if users restricted themselves to tutorials from 10 or so years ago when varying was in common usage.

Also keep in mind the concept of the light shader is also Godot specific. There is no "proper" or "explicit" way to pass data between fragment and light. We are in the position of crafting the API.

[2plus2makes5]

I personally like ClayJohn's proposal, it's simple, clean, general, consistent, powerful and doesn't introduce unnecessary new keywords or limitations.

[lawnjelly]

Seems like a bit of a hornet's nest 😁 , but my 2p...

I'm also in the not altogether convinced of using the varying keyword camp, partly due to potential for confusion and conflict with an existing keyword, but also:

• These parameters don't appear to be varying?

The word varying was chosen afaik to emphasize that parameters will be interpolated from vertex to fragment shader (unless flat modifier is also used). In this case the parameters do not vary as I understand it, they are simply passed directly from the fragment to the light function.

Almost any keyword would be better imo, light_parameter, light_argument etc etc.

[Megalomaniak]

Yes a passing keyword might make more sense than varying, but then does it even need a special keyword at all?