Support shader preprocessor concatenation symbol

[JohanAR]

No description provided.

[JohanAR]

I removed the padding " " because they made unit testing much more complicated. Have not found any case where this caused problems.

[bitsawer]

Should be fine, like I mentioned in RocketChat adding those spaces might have been there "just in case".

[JohanAR]

I found one case where it changes the behaviour:

#define MOD +
float a MOD= 1.0;

Previously this would've been expanded to invalid code, while now it works. I did some tests with a C++ compiler and it doesn't work there.

Should I revert this line and update the tests to require the surrounding spaces? I think it would be difficult to relax the test to allow them without adding a full blown glsl parser/tokenizer.

On the other hand, this is probably an edge case, and I would say that (with my change) it works like I would expect it to.

[JohanAR]

Was about to revert this padding change, but then I realised that the example above would not be affected since the padding was only done for macros with arguments.

E.g. before my PR the following defines:

#define A -10
#define B(x) -x
int n = 1-A;
int m = 1-B(3);

Would be expanded to:

int n = 1--10;
int m = 1- -3 ;

I.e. the behaviour changes depending on if you have args or not, and leading to one case where the code becomes invalid.

Kind of leaning towards adding the padding for argument-less macros too now, even if it'll make my unit tests uglier.

[clayjohn]

CC @bitsawer

[JohanAR]

And in case anyone was wondering, this is my actual use case that motivated this PR. I wanted to make a sorting network more easily.

[bitsawer]

Overall, looks pretty good! Some small changes needed, but nothing fundamental. Gave it a few quick tests and nothing exploded. Don't worry about making those tests look ugly and full of whitespace with weird indentation and lax comparison rules. As discussed on RocketChat, I'm working on that shader preprocessor test suite so I'll add and format your tests into it and eventually try to make a PR once I have some extra time to finish it. I can give this another test later and a core team member will also have to give their review, but I don't see any major issues.

[bitsawer]

This TEST_CASE_MAY_FAIL() is bit ugly in the test output, printing a big red error and the failed comparison text in the console even though the full test suite still passes. It might be better to make it a normal TEST_CASE() and "fail succesfully" to keep the "noise" in the testing results output minimal. The comment there should be enough to clarify the test case.

[JohanAR]

Before I discovered the TEST_CASE_MAY_FAIL macro I had it marked both as may_fail and skip, but I'll do as you suggest.

[Calinou]

Remember to document this change in https://docs.godotengine.org/en/latest/tutorials/shaders/shader_reference/shader_preprocessor.html 🙂

You can open a draft pull request on godot-docs' master branch now – no need to wait for this PR being merged.

[JohanAR]

Docs PR godotengine/godot-docs#6979

[bitsawer]

Looks good to me after the changes. Tested with my own projects and tests, seems to work as expected. I'll add those tests to my preprocessor test suite and try to eventually make a public PR for it once I have some extra time.

@Chaosus might still want to take a look and give the final approval.

[Chaosus]

Tested and it works fine for me - just add the missing dots to the commentaries.

[JohanAR]

Tested and it works fine for me - just add the missing dots to the commentaries.

Thanks! I'm not that used to working with github's web UI, but I think I got all the suggestions. Should I try to squash the branch or will that be done when merging?

[Chaosus]

Should I try to squash the branch or will that be done when merging?

Yes, you should.

[akien-mga]

Thanks!