Ipotrick / Daxa

Daxa is a convenient, simple and modern gpu abstraction built on vulkan
MIT License
381 stars 28 forks source link

Change daxa::None to daxa::Nothing to prevent collision with X.h #85

Closed OmniSudo closed 7 months ago

OmniSudo commented 7 months ago

I fixed Daxa compiling on Linux with X11 by changing daxa::None to daxa::Nothing

GabeRundlett commented 7 months ago

You should #undef None from X.h instead

OmniSudo commented 7 months ago

What if I want to use X None?

GabeRundlett commented 7 months ago

You don't

OmniSudo commented 7 months ago

That could create issues if files are included in a certain order

GabeRundlett commented 7 months ago

Libraries that have generic #defines should never dictate what the naming choices of other libraries are, when libraries like daxa have well scoped names (namespaced)

OmniSudo commented 7 months ago

It just seems like a high coupling, low cohesion problem when daxa is responsible for X11 stuff

GabeRundlett commented 7 months ago

See https://github.com/OneLoneCoder/olcPixelGameEngine/issues/195#issuecomment-1249979632

OmniSudo commented 7 months ago

Right but why should daxa be concerned about what another library does in its defines? Daxa shouldn't be interfering with another library imo

GabeRundlett commented 7 months ago

it shouldn't be?? That's what I'm arguing. You're saying we should change daxa's code to play nicely with another library

GabeRundlett commented 7 months ago

Daxa isn't interfering with X11, X11 is interfering with Daxa. Daxa has scoped names. X11 does not.

OmniSudo commented 7 months ago

Okay see, both sides are right. Its just a matter of which one is the better decision. Im fine with either I guess

OmniSudo commented 7 months ago

But having daxa undefine it would cause potential for other libraries that depend on x11 to break?

GabeRundlett commented 7 months ago

Daxa does NOT include X11 in its headers. This should not be an issue at all.

OmniSudo commented 7 months ago

Okay X11 isnt even relevant. How would I go about using a scoped None in the source when X11 is included in my ecosystem with glfw3

GabeRundlett commented 7 months ago

I linked you a solution and told you as well to #undef it.

OmniSudo commented 7 months ago

That wont work if it has to be in my code base. It has to be in daxa to compile. When I have nothing but daxa, X.h is included somewhere in limbo.

GabeRundlett commented 7 months ago

it does not have to be in Daxa to compile. Daxa does not include it in its public headers. You have to be doing something insanely wrong to have X.h included without you knowing.

OmniSudo commented 7 months ago

Doesnt glfw include x?

GabeRundlett commented 7 months ago

Daxa does not include glfw. Also, glfw will not include X11 if you just include GLFW/glfw3.h. You need to do #define GLFW_EXPOSE_NATIVE_X11 and #include <GLFW/glfw3native.h> for glfw to include X11.

OmniSudo commented 7 months ago

A real mystery

MatejSakmary commented 7 months ago

You are probably leaking from your glfw includes just like Gabe described, it's not Daxas fault

OmniSudo commented 7 months ago

Even by its lonesome the Daxa project complains about X11 so I'm uncertain

MatejSakmary commented 7 months ago

Where?

GabeRundlett commented 7 months ago

You can not be complaining about Daxa if you don't even know the source of the error.

Ipotrick commented 7 months ago

Im fine with renaming None to Nullopt

Ipotrick commented 7 months ago

I hate libs declaring things without namespace esp with macros

GabeRundlett commented 7 months ago

Omni must find the source of the error if I am to continue this conversation. If he doesn't even know where he's including the X11 header from, it's a really silly conversation IMO

GabeRundlett commented 7 months ago

This build error should be fixed by https://github.com/Ipotrick/Daxa/commit/ed6318ab3d15f7dd6dc92cd6e4700be22fd382d0. Another user sent a vcpkg build log so I could figure out what was going on. Unity build was making the error not reproduce-able for me, so I had no clue what was going on. Closing this issue now, if the issue persists, feel free to open a new issue.