Closed lilleyse closed 7 months ago
Have you considered using a scoped enum instead? (C++ 11 feature)
struct AlphaMode
becomes
enum struct AlphaMode
This way we can keep our enum names, but also not play the "this platform's scope is polluted with X" game.
The original reason for using a struct with constants as opposed to an enum is described here: https://github.com/CesiumGS/cesium-native/issues/258#issuecomment-859280034
Good example. I can't get that to work either.
Looks like if OPAQUE
is already defined, this will cause problems defining AlphaMode
, regardless if it's scoped or not.
I suppose the code that includes wingdi.h
can try to stuff it into a separate namespace, but if we are looking for simple, it's hard to beat the renaming approach you did.
Good example. I can't get that to work either.
Looks like if
OPAQUE
is already defined, this will cause problems definingAlphaMode
, regardless if it's scoped or not.I suppose the code that includes
wingdi.h
can try to stuff it into a separate namespace, but if we are looking for simple, it's hard to beat the renaming approach you did.
I think that is going to be unwinnable, given that the offending symbols are macros.
I believe that spdlog is sucking wingdi.h into Cesium Native headers by including windows.h somewhere, and it might be possible rearrange or forward-declare the spdlog declarations so that wouldn't happen anymore in Cesium Native clients. But then any user who included windows.h themselves would run into this problem again.
Possible related: https://github.com/CesiumGS/cesium-native/issues/584
I have more to say on this, will type it up soon.
In my view, windows.h
and friends are a total disaster. Including it in most projects, or alongside many third-party libraries, will cause all kinds of confusing errors. It has macros for all kinds of common things; not just OPAQUE
but also MIN
and MAX
(also min
and max
I believe). It appends - again via the preprocessor - nonsense like _A
and _W
to the end of a bunch of common-ish names.
The only winning move, IMO, is not to play. Don't include windows.h
. In the rare case that you're writing non-portable code that needs to use windows functionality directly, include windows.h
only in a .cpp
file that implements functions, declared in a header file (that does not include Windows headers), that can be called by the rest of your application. In other words, isolate the functionality in a single compilation unit that provides an interface to Windows without including any third-party libraries.
Unless there's a case I'm not aware of where this sort of approach creates an undue burden, I'd much prefer it over modifying cesium-native to avoid Windows' many macros.
Something else must be including windows.h
in cesium-omniverse because we already have exceptions enabled. @timoore mentioned spdlog. It could also be a library in omniverse. I haven't looked deeper yet.
I'm going to close this PR, but it would still be nice to fix this somehow.
Renamed
CesiumGltf::Material::AlphaMode::OPAQUE
toOPAQUE_ENUM
to deconflict with the define inwingdi.h
The fix was to add
OPAQUE
to the reserved words list and add a suffix to enums using reserved words.Now projects using
CesiumGltf
don't need to add code like this everywhere:Unfortunately, this is a breaking change and it looks a little odd to see
OPAQUE_ENUM
,MASK
,BLEND
. So I'm open to alternatives here.