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

Extend platform support for WSI without the need to include windowing API #195

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

RedNicStone
Copy link

At the moment volk allows the user to use WSI functions for the Win32 platform without having to include windows.h.
This is a very useful feature not only to prevent unwanted definitions and reduce compile time but it also allows the developer to separate code handling windowing from the code that handles rendering.

Unfortunately this is not currently available on any other platform.
I have identified the platforms that could benefit from this feature and added it on my fork.
In addition to that I added guards that prevent redefinition of types in case the user includes the library (not that they must be included before volk.h for this to work).

I think this is a good change to take into the master branch.

@zeux
Copy link
Owner

zeux commented Nov 6, 2024

I don't really understand the motivation for this change. With Xlib define on my system volk.c takes 83ms to compile without VK_USE_PLATFORM_XLIB_KHR and 86ms with VK_USE_PLATFORM_XLIB_KHR defined. I don't like the original Windows change because of the maintenance overhead, but the reduction in build cost is too significant to pass. It doesn't look like Xlib or Wayland have anywhere close to the same impact.

Every single define that is not Xlib here is something that I either have no way of testing and isn't tested in CI (e.g. Fuchsia? GGP? QNX?), or something that I don't think is meaningful to almost all volk users (xcb, Xlib randr, directfb).

For Xlib, this change does not replicate the declaration structure; additionally, it removes the typedefs from Windows path if it is already included. The typedefs are intentionally left there to ensure the types are exactly the same; this prevents redefinition errors when the relevant platform headers are explicitly included in the same translation unit but after volk; for example, if I compile the following program with this PR:

#define VK_USE_PLATFORM_XLIB_KHR
#include "volk.h"
#include <X11/Xlib.h>

I will get the following compilation errors:

In file included from /usr/include/X11/Xlib.h:44,
                 from test.c:3:
/usr/include/X11/X.h:76:23: error: conflicting types for ‘VisualID’; have ‘long unsigned int’
   76 | typedef unsigned long VisualID;
      |                       ^~~~~~~~
In file included from test.c:2:
volk.h:81:50: note: previous declaration of ‘VisualID’ with type ‘VisualID’ {aka ‘unsigned int’}
   81 |                                 typedef uint32_t VisualID;
      |                                                  ^~~~~~~~
/usr/include/X11/X.h:96:13: error: conflicting types for ‘Window’; have ‘XID’ {aka ‘long unsigned int’}
   96 | typedef XID Window;
      |             ^~~~~~
volk.h:80:50: note: previous declaration of ‘Window’ with type ‘Window’ {aka ‘unsigned int’}
   80 |                                 typedef uint32_t Window;
      |                                                  ^~~~~~
/usr/include/X11/Xlib.h:487:26: error: conflicting types for ‘Display’; have ‘struct _XDisplay’
  487 | typedef struct _XDisplay Display;
      |                          ^~~~~~~
volk.h:79:56: note: previous declaration of ‘Display’ with type ‘Display’
   79 |                                 typedef struct Display Display;
      |                                                        ^~~~~~~

@RedNicStone
Copy link
Author

RedNicStone commented Nov 7, 2024

The motivation for this change is not a decrease in compile time.
In many applications rendering code is separated from windowing code. This is done so that the rendering code may be platform independent in of itself, and the user passes just a generic set of handles to the graphics abstraction.

This separates out the WSI code which means that the abstraction does not need to link against the window system. This is is especially useful on linux where a user may not have the headers for a specific windowing API, yet a the render code can still compile without explicitly disabling certain platforms.

Its also inherently cross-platform compatible, so the linux WSI headers may be included on windows and vice versa without additional platform checks. This is also my use case.
The Vulkan API already does this for many platforms (all apple platforms, wayland, etc), but strangely not for all.

Every single define that is not Xlib here is something that I either have no way of testing and isn't tested in CI (e.g. Fuchsia? GGP? QNX?), or something that I don't think is meaningful to almost all volk users (xcb, Xlib randr, directfb).

XCB, Xlib-randr and maybe even directfb is certainly useful for most volk users.
The rest is added for extended compatibility. I understand that these platforms are hard to test but since these are rarely targeted issues should suffice.

For Xlib, this change does not replicate the declaration structure

I found the structure was hard to replicate, but looking at it again it doesn't seem problematic as Display can simply be left declared. This could be changed in a future change.

It removes the typedef's from Windows path if it is already included

These guards are added for all platforms. This is done so that if the platform API differs from the one emulated by volk the issue can still be resolved by changing the include order. I don't see how this could cause any problems.

@zeux
Copy link
Owner

zeux commented Nov 7, 2024

issue can still be resolved by changing the include order. I don't see how this could cause any problems

That is literally how this change resulted in errors on the example above.

What applications rely on xcb/xlib-randr/directfb?

I don’t fully understand why being able to build without X headers but with X WSI extension is valuable. Somewhere else in your app you’d have a component that presumably depends on X? Unless you aren’t building parts from source, you’d have the same platform selection somewhere else. You do get cleaner builds in the sense that X headers don’t spill into the rest of the rendering code.

In my ideal world changes like this would be done in Vulkan SDK, the current Windows workaround is a build time hack, nothing else. If this is crucial, it should be done as a minimal sufficient patch that is correct under all circumstances.

@RedNicStone
Copy link
Author

That is literally how this change resulted in errors on the example above.

This would be resolved by properly emulating the Xlib API, not by removing the guards.

What applications rely on xcb/xlib-randr/directfb?

XCB and Xlib-randr are somewhat popular, SDL will try to use XCB for example for its superior event processing. DirectFB is commonly used in embedded applications or other applications that dont use a window system.

I don’t fully understand why being able to build without X headers but with X WSI extension is valuable.

Its mainly just for cleaner API design. An abstraction library may not want to link against those for example. My personal use for this is that the abstraction is distributed as a cross platform shared binary (so it even needs to be able to compile win32 on linux and vice versa, for example) which is loaded into an engine at runtime.

I see how this may not be the usual usecase, but I know of other abstractions that implement other forms of this. Namely Daxa does something similar (but daxa has since removed volk as a dependency, so this is implemented manually).

@zeux
Copy link
Owner

zeux commented Nov 7, 2024

This would be resolved by properly emulating the Xlib API, not by removing the guards.

I don't think I am being heard here. Which is fine, as I am not yet sure this is broadly beneficial. I will wait for other users of volk who would benefit from this change to chime in on this PR and describe their use case.

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.

2 participants