Shaders: Allow trailing commas in function calls

[akx]

Describe the project you are working on

A shader-heavy video sequence generator

Describe the problem or limitation you are having in your project

Long shading expressions like

vec3 fadeInTex = clamp(mix(-1.0, 2.0, fadeIn) + texture(fade_in_bias, UV).rgb, 0.0, 1.0);

would be nicer formatted as

vec3 fadeInTex = clamp(
  mix(-1.0, 2.0, fadeIn) + texture(fade_in_bias, UV).rgb, 
  0.0, 
  1.0,
);

but the presence of a trailing comma in the argument list (as allowed in GDScript in 4.0+, https://github.com/godotengine/godot/issues/41066) yields

error(69): Expected expression, found: 'PARENTHESIS_CLOSE'

For context, function definition argument lists already accept a single trailing comma; both

vec3 neonChevrons(vec2 uv,) {

and

vec3 neonChevrons(
    vec2 uv,
) {

parse fine.

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

This is somewhat tautological, but:

Fixing the shading language parser to allow trailing commas in function call argument lists in the shader language parser would allow one to use trailing commas in function call argument lists.

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

When the user reformats their shader code to a form where a function call has a trailing comma in its argument list, the comma token would be swallowed, and no error for a missing expression would be emitted.

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

This can not be worked around with script, because it's part of the shading language parser: https://github.com/godotengine/godot/blob/84e205b5a17bfe7ace96b78c410ec10aa52241d2/servers/rendering/shader_language.cpp#L5739-L5743

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

The shading language parser is part of Godot core.

[Mickeon]

Makes sense to me. GDScript allows trailing commas in function arguments, as well (It does not in 3.x I believe).

[Calinou]

This is a good idea in principle, but keep in mind standard GLSL syntax doesn't allow this. As a result, allowing trailing commas will result in the code not being usable outside of Godot shaders without being modified.

Unlike GDScript, the Godot shader language is designed for a high level of interoperability with GLSL, so this change may have negative effects here. It's quite common for people to copy shader code between GLSL and Godot shaders in both directions, so this is definitely a supported use case already.

I'm surprised this is currently allowed in function definitions, as I don't remember GLSL supporting this either (please double-check).

[akx]

@Calinou Good point! However, I've found I need to edit existing GLSL code to port it to Godot shaders anyway, since Godot's shader language doesn't support function overloads or in/out for parameters...

[AThousandShips]

That's the wrong direction of conversion, what Calinou is talking about is taking GDShader code and using it in general GLSL shaders, which is normally possible with minor changes, as it largely doesn't have features not in GLSL

[akx]

@AThousandShips Er, well, Calinou's message does say "copy shader code between GLSL and Godot shaders in both directions", emphasis mine.

[AThousandShips]

Yes, but:

As a result, allowing trailing commas will result in the code not being usable outside of Godot shaders without being modified.

The direction of relevance was Godot -> GLSL 🙂

And this is about fundamental syntax, not specific features IMO

Now I'm biased as I fundamentally oppose trailing commas of this type, but as far as I recall the justification for adding it to GDScript was that it's allowed in Python and that it was a matter of parity in style

Godot shaders are based on GLSL which doesn't have this, and the syntax of Godot shaders and GDScript is fundamentally different, so I don't think there's a strong argument in favor here, and a strong argument against to keep parity with GLSL and make porting code at least somewhat easy and comfortable

[clayjohn]

I really don't think we should be breaking compatibility with GLSL here. A shading language is not the right place to by adding pythonic syntax for the sake of it.

[akx]

I'd like to note that the trailing comma for function arguments is not just pythonic; it's also a thing for JavaScript since ES2017 :)

Anyway, I suppose someone needs to make a decision as to what the level of GLSL syntax parity should be to guide discussions like these going forward. (I suppose we're only talking about parity in function blocks here, not preludes such as shader_type, or the instance modifier for uniforms, etc.)

As mentioned above,

float foo(float a,) {
    return a + 5.0;
}

is valid Godot shader code (v4.2.1.stable.official [b09f793f5]), but not valid GLSL, so there's already a break there.

For the ease-of-porting argument, "fixing" argument-list-trailing-commas (be they in function declarations, or in function calls) for porting Godot shader code to GLSL is (in the absence of comments) a matter of replacing the regexp ,\s*\) with ).

[clayjohn]

As mentioned above,

float foo(float a,) {
  return a + 5.0;
}

is valid Godot shader code (v4.2.1.stable.official [b09f793f5]), but not valid GLSL, so there's already a break there.

Honestly, this looks like a bug to me. If it works, its a happy accident and doesn't seem intentional.

[AThousandShips]

It looks like an oversight to me, since it's just for declarations, and the part of the code that handles the commas doesn't make any mention of allowing trailing comma which I'd assume it would if it was intentional, and if it was intentional the missing support in calls would have been noticed IMO, the code here is already quite hard to read and I'd suspect it was a simple oversight in the code flow

https://github.com/godotengine/godot/blob/352434668923978f54f2236f20116fc96ebc9173/servers/rendering/shader_language.cpp#L9734

[AThousandShips]

My suggestion would be to fix the trialing comma in declarations, possibly with a deprecation clause allowing it without DISABLE_DEPRECATED

Edit: I will look into fixing this

[akx]

I'm still wondering whether the strict parity with GLSL, one way or the other, is the pragmatic choice... There's already this document about porting GLSL to Godot shaders, which says

Godot uses a shading language based on GLSL with the addition of a few quality-of-life features.

(emphasis mine) – I think trailing commas definitely are a quality-of-life feature.

The Shading language page says

Godot uses a shading language similar to GLSL ES 3.0. Most datatypes and functions are supported, and the few remaining ones will likely be added over time.

(emphasis mine, again).

(As an aside, it would be an interesting exercise to write a tool that would let you port various dialects of these GLSL-ish languages into one another...)

[AThousandShips]

I wouldn't agree that unusual syntax that differs from the language it is based on is a question of QOL, the same could be said for many syntax and style quirks

[AThousandShips]

Further inconsistency:

float arr[3] = { 1.0, 2.0, 3.0, };

Is also disallowed, as is:

MyStruct my_struct = MyStruct(foo, bar, );

Which uses a distinct call, as is:

uniform float my_uniform : hint_range(0, 1, );

So this applies to most cases

[akx]

To bring @AThousandShips's comment https://github.com/godotengine/godot/pull/86991#issuecomment-1887473439 into this conversation and not just that PR:

seems glsl allows trailing comma in arrays

so there's another break to GLSL ES 3.0 compatibility here (since Godot doesn't allow those at present).

Right now there's no formal specification for the shading language that I know of, just (paraphrased) "similar to GLSL ES 3.0 with some QOL features" and the C++ implementation is the living spec. (To save someone a google, the grammar for the GLSL ES 3.0 shading language can be found in this PDF, section 9, here in text form.)

I'd personally like to work with a slightly more pragmatic language than GLSL; for instance, automatic casting of arguments for built-in functions like clamp(), vec*(), etc. would probably be pretty useful, and so on, but that's another discussion and will require someone to have made the decision-in-principle whether breaking from GLSL is okay at all.

I understand that it's nice to be able to be able to copy Godot shader code to GLSL contexts (and even then, you need to go through the code and rename uniforms and so on), but it feels to me like strict 1:1 compatibility shouldn't be the priority.

[AThousandShips]

While absolute compliance doesn't have to be a priority, sure, that doesn't equate to adding complex code with possible points of failure though, so the validity of supporting this in a wider context, both from style, necessity, and cost should be the focus, unless we do argue that making a quite major departure from the style and syntax of GLSL is not a good idea (which is what I argue)

The shader language code is extremely convoluted and difficult to piece out, it's very old as far as I can tell and has a lot of quirks, it is likely very prone to failure points due to this, and I think that this plays into the cost of adding a feature like this, trailing commas like this are not supported in GLSL, or C the language it borrows its syntax from

If the shader code was less of a bowl of spaghetti I'd say this would be less of an issue, but the code as it exists is a ball of yarn that very easily comes apart, at least that's the impression I've gotten working with it recently, for example the parsing of array constructors is done in no less than 4 places, which is extremely confusing