Skip to content
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

GDShader float literal syntax doesn't match GLSL ES in some cases #61448

Closed
AlfishSoftware opened this issue May 26, 2022 · 0 comments · Fixed by #61881
Closed

GDShader float literal syntax doesn't match GLSL ES in some cases #61448

AlfishSoftware opened this issue May 26, 2022 · 0 comments · Fixed by #61881

Comments

@AlfishSoftware
Copy link

Godot version

v3.4.4.stable.mono.official [419e713], v4.0.alpha8.official [cc3ed63]

System information

Kubuntu 22.04 x64

Issue description

(from godotengine/godot-vscode-plugin#360)

GDShader grammar for float literals doesn't seem to match the one from GLSL ES 3.0 spec (section 4.1.4):

// Equivalent ANTLR4 syntax:
FLOATING_CONSTANT
: Digit+ ExponentPart FloatingSuffix?
| FractionalConstant ExponentPart? FloatingSuffix?
;
fragment FractionalConstant: Digit* '.' Digit+ | Digit+ '.' ;
fragment ExponentPart: [eE] [+-]? Digit+ ;
fragment Digit: [0-9] ;
fragment FloatingSuffix: [fF] ;

From my interpretation, an equivalent JS regex would be something like this:
/\b(?:\d+[eE][-+]?\d+|(?:\d*[.]\d+|\d+[.])(?:[eE][-+]?\d+)?)[fF]?/

Below is some cases I tested (might not cover all problems).

Godot 4.0.alpha8

It's accepting floats like 0ef, 0e+f, 0.0ef, 0.0e-f, which are invalid in GLSL. Which is strange because invalids like 0e, 0.0e are being rejected correctly. At least in the GLSL grammar above, the digits are always required after the e in exponent notation. But something strange in gdshader grammar is accepting an empty number when f is present.

Godot 3.4.4

This, on the other hand, seems to not allow lowercase e and f together at all, even when the exponent number is present.
It's rejecting valid: 0e0f, 0e+0f, 0.0e0f, 0.0e-0f. When removing either e... or f part, then it's accepting it as normal.
It's also rejecting uppercase E exponents when they have a sign, even though it's fine in lowercase, e.g.: 0E+0, 0.0E-0.

Steps to reproduce

Just create a test.gdshader file in a new Godot project and double click to open it in the internal Shader editor.
Paste text below, and watch it for errors / non-errors.
Also syntax coloring is not exactly matching all tokens, even when compiler accepts them. Uppercase, for example, seems to mess with it.

Minimal reproduction project

This is not to cover all test cases by any means, just to facilitate testing some of the examples above. In a quick estimate, if all test cases were written (using just 0 in numbers), there should be ~81 (?) variations that should be accepted ... not to mention the ones to test rejection. So, it's better to just try to really carefully match the grammar/regex from the spec part by part and in tests try to break it with some edge cases. Multi-cursor feature in editors like VSCode may help with editing this to test other cases.

test.gdshader

shader_type spatial;

// should accept:
const float ok01 = 0e0f;
const float ok02 = 0e+0f;
const float ok03 = 0.0e0f;
const float ok04 = 0.0e-0f;
const float ok05 = 0E0f;
const float ok06 = 0E+0f;
const float ok07 = 0.0E0f;
const float ok08 = 0.0E-0f;
const float ok09 = 0E0F;
const float ok10 = 0E+0F;
const float ok11 = 0.0E0F;
const float ok12 = 0.0E-0F;
const float ok13 = 0E0f;
const float ok14 = 0E+0f;
const float ok15 = 0.0E0f;
const float ok16 = 0.0E-0f;

// should reject:
//const float no01 = 0ef;
//const float no02 = 0e+f;
//const float no03 = 0.0ef;
//const float no04 = 0.0e-f;
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants