KhronosGroup / KTX-Software

KTX (Khronos Texture) Library and Tools
Other
874 stars 229 forks source link

OpenGL loader macros generate invalid code in MSVC #504

Closed jansol closed 2 years ago

jansol commented 2 years ago

Seems an extra ## has snuck in: https://github.com/KhronosGroup/KTX-Software/blob/3c1fa2a/lib/gl_funcs.c#L111

Also, disabling the gl loader when using libktx as a cmake submodule does not seem to work. Might be related to the cmake settings being implemented as variables set(...) rather than options option(...)?

MarkCallow commented 2 years ago

Which compiler are you using that is causing issues with the extraneous ##? The VS compilers up to VS 2019 happily paste gl. and the value of func giving the right result. I haven't tried anything later.

I'm not familiar with cmake submodules nor the ins and outs of option vs set. Please expand on "does not seem to work".

@atteneder please comment , particularly on why set is used here not option.

atteneder commented 2 years ago

Hi @MarkCallow ,

@jansol has a point. set as used right now overwrites the variable, thus any attempt to set it to OFF is discarded.

Two solutions:

There are more settings that could be subject to change here:

Note: This is not thoroughly tested, so there might be combination of settings that don't work

I'd say we expose those settings as set+CACHE now and with option once we're certain all combinations work as expected.

The code would be:

set( KTX_FEATURE_KTX1 ON  "" ON CACHE BOOL "Enable KTX 1 support" )
set( KTX_FEATURE_KTX2 ON  "" ON CACHE BOOL "Enable KTX 2 support" )
set( KTX_FEATURE_VULKAN ON  "" ON CACHE BOOL "Enable Vulkan texture upload" )
set( KTX_FEATURE_GL_UPLOAD "" ON CACHE BOOL "Enable OpenGL texture upload" )
MarkCallow commented 2 years ago

Thanks @atteneder. I will open a new issue for the changes you've recommended.

jansol commented 2 years ago

FWIW this was in Visual Studio 2022, both with the msvc and clang frontends IIRC.