gboisse / gfx

A minimalist and easy to use graphics API.
MIT License
498 stars 35 forks source link

Add NOMINMAX define. #82

Closed maoliver-amd closed 1 year ago

maoliver-amd commented 1 year ago

Prevents windows headers defining a min/max macro that clashes with std and glm

gboisse commented 1 year ago

Mmh, what kind of problem was this creating? I don't think we've had any issue with this yet?

That's the reason for the brackets around the calls to min and max btw: https://github.com/gboisse/gfx/blob/ab5531fc1e3235e145603f397e5eff6f0db231e6/gfx_core.h#L67-L69

So, you have to call such functions that way: (min)(...) to avoid the macro issue (or just use GFX_MIN()).

gboisse commented 1 year ago

I guess I'd rather the application decides whether to define such macros rather than having it directly at the gfx level.

maoliver-amd commented 1 year ago

The issue was with numeric_limits::max (and also min) being used to init various constexpr variables. I figured that if the application wanted min/max defs they can preinclude windows.h before gfx but otherwise gfx shouldnt pollute the def space of the application

gboisse commented 1 year ago

Well I agree with you that windows.h is a mess, but same as a user can define NOMINMAX when they include that header, they can do just the same when including gfx.h.

maoliver-amd commented 1 year ago

yep true. I was just doing a quick google search for windows,h etiquette for installed headers to see whether there is standard recommended practice. Apparently the recommendation is that windows.h should never get included in a public header at all ☹️

Anyway if you prefer to keep NOMINMAX in capsaicin as a global define and not in gfx then feel free to just close this

gboisse commented 1 year ago

I wonder if we could include the specific header files that we need rather than the whole windows.h. I believe I did try this at some point but it was a mess of compilation errors... not sure it's worth the trouble really.

maoliver-amd commented 1 year ago

I wonder if we could include the specific header files that we need rather than the whole windows.h. I believe I did try this at some point but it was a mess of compilation errors... not sure it's worth the trouble really.

I had a quick look and it might be doable but with issues, gfx_core.h is tricky as it is exposing functions inline as opposed to behind something like GFX_IMPLEMENTATION_DEFINE. If we separated all that between declaration and definitions it would probably minimise it as the public API only really needs HWND and some d3d12 enums. Problem is those enums are in d3d12.h which pull in windows.h and breaks in 6million complicated ways when you try and strip windows.h from it.

So only ""simple"" solution would be to removed d3d12 from public api and create your own equivalent enums etc and then internally convert them. Which is why I just smacked NOMINMAX on it and called it a day 😃

On a side note we probably want WIN32_LEAN_AND_MEAN as well

gboisse commented 1 year ago

Yea, it's a mess indeed:p Re-creating all the enums feels pretty counterproductive tbh. I'd say we close this PR and define whatever macro we want one level up (NOMINMAX, WIN32_LEAD_AND_MEAN, etc.).