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

Do not redefine NOMINMAX #801

Merged
merged 1 commit into from
Nov 21, 2023
Merged

Do not redefine NOMINMAX #801

merged 1 commit into from
Nov 21, 2023

Conversation

corporateshark
Copy link
Contributor

No description provided.

@CLAassistant
Copy link

CLAassistant commented Nov 16, 2023

CLA assistant check
All committers have signed the CLA.

@MarkCallow
Copy link
Collaborator

What is the build environment in which you encountered NOMINMAX being set and a compiler that complained? Since this is a plain #define I don't see how it could be redefining NOMINMAX. Compilers usually only complain if there is a redefine not if a previous definition is repeated.

@corporateshark
Copy link
Contributor Author

@MarkCallow There's yet another header which defines NOMINMAX in a similar way. Every library thinks it is the only library and mess up somebody else's macros in its headers ;)

@MarkCallow
Copy link
Collaborator

The file you've changed is internal to libktx so should not be affected and should not affect other headers your application may be using. And, as I said, compilers usually only complain when an item is being defined to a different value than it's current value. Even then they don't complain if the current value is undefined. So I'm curious which compiler you are using.

@corporateshark
Copy link
Contributor Author

So I'm curious which compiler you are using.

Visual Studio 2022.

I'm including that file directly to access OpenGL texture format definitions without including full-sized actual OpenGL headers (of which I have none, because the app is using Vulkan for rendering and needs to render KTX1 textures as well as KTX2).

@MarkCallow
Copy link
Collaborator

I'm including that file directly to access OpenGL texture format definitions

Got it. So you are writing your own KTX texture loader, correct? May I ask why you chose not to use the Vulkan loader in libktx?

I expect to remove gl_format.h from the project eventually.

@corporateshark
Copy link
Contributor Author

May I ask why you chose not to use the Vulkan loader in libktx?

I have a multi-API renderer. Loaded textures are agnostic of any specific API.

@MarkCallow
Copy link
Collaborator

I have a multi-API renderer. Loaded textures are agnostic of any specific API.

Great. I would happily accept PR(s) for D3D12 and Metal loaders if you were so inclined.

Thanks for this fix.

@MarkCallow MarkCallow merged commit 6dbb246 into KhronosGroup:main Nov 21, 2023
14 checks passed
corporateshark referenced this pull request in corporateshark/lightweightvk Nov 22, 2023
KaperD pushed a commit to KaperD/KTX-Software that referenced this pull request Feb 21, 2024
KaperD pushed a commit to KaperD/KTX-Software that referenced this pull request Feb 22, 2024
KaperD pushed a commit to KaperD/KTX-Software that referenced this pull request Feb 22, 2024
KaperD pushed a commit to KaperD/KTX-Software that referenced this pull request Feb 22, 2024
KaperD pushed a commit to KaperD/KTX-Software that referenced this pull request Feb 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants