d4rkc0d3r / d4rkAvatarOptimizer

d4rkpl4y3r's VRChat Avatar 3.0 optimizer
MIT License
400 stars 17 forks source link

put geom wrapper struct inside same #ifs as geom function #124

Closed PiMaker closed 2 months ago

PiMaker commented 2 months ago

Fixes a miscompilation for code like this:

#if defined(FOO)
[maxvertexcount(3)]
void geom(...)
{
    ...
}
#endif

#if defined(BAR)
[maxvertexcount(3)]
void geom(...)
{
    ...
}
#endif

Previously the wrapper structs would end up outside the #if defined(FOO) defines unnecessarily (they aren't needed when the corresponding function is not compiled anyway). This leads to a duplicate struct definition regardless of defines:

struct geometryInputWrapper
{
    ...
}
#if defined(FOO)
[maxvertexcount(3)]
void geom(...)
{
    ...
}
#endif

struct geometryInputWrapper        // <- duplicate definition!
{
    ...
}
#if defined(BAR)
[maxvertexcount(3)]
void geom(...)
{
    ...
}
#endif

The original code is perfectly valid though, as long as FOO and BAR are exclusive keywords.

This fix moves the struct definition below the #if which makes them exclusive to each other similar to the function itself.

Alternative Fixes

The reason for searching until the top-most s.StartsWith("[") is obvious, but I wasn't sure if there was a case where the search for s.StartsWith("#") was needed too, so I restricted it to only exclude conditionals.

An alternative solution would be to rename the structs, e.g. encode dummyLineIndex (function name wouldn't work as can be the same) into it.

d4rkc0d3r commented 2 months ago

but I wasn't sure if there was a case where the search for s.StartsWith("#") was needed too

it is absolutely needed. an example would be:

#if defined(FOO)
[maxvertexcount(3)]
#else
[maxvertexcount(6)]
#endif
void geom(...)
{

So this would need the alternate solution with differently named wrapper structs.

PiMaker commented 2 months ago

Of course it's not that simple, should have went with my gut on the renaming solution 🤦🏻

Changed to that approach in fccfdd0 and confirmed it still works on my test shader.