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

Add support for creating Vulkan window surfaces. #1557

Merged
merged 1 commit into from Oct 16, 2019

Conversation

binary1248
Copy link
Member

Add support for creating Vulkan window surfaces.

Example is provided in the patch.

@binary1248 binary1248 self-assigned this Feb 15, 2019
@eXpl0it3r eXpl0it3r added this to Discussion in SFML 2.6.0 via automation Feb 19, 2019
@eXpl0it3r eXpl0it3r added this to the 2.6 milestone Feb 19, 2019
@eXpl0it3r eXpl0it3r moved this from Discussion to Review & Testing in SFML 2.6.0 Feb 19, 2019
@eliasdaler
Copy link
Contributor

Can you please give me some ideas at what and how do you want this to be reviewed?

Reviewing such a huge patch is scary, but I think if you tell me what to check here, I'll be happy to devote some time to reviewing this thoroughly. :)

@binary1248
Copy link
Member Author

binary1248 commented Feb 22, 2019

The funny thing is that over 70% of the patch lines aren't even SFML code, it is all from the Vulkan headers.

Then the next bigger chunk (20%) of changes come from the new Vulkan example, which you could look at... but if you aren't experienced with Vulkan you would also find pretty daunting (still worth a look though).

The smallest chunk and the code that should be "most" reviewed are the actual changes to the library and its public interface. These can be found in the files in the src and include directories as usual.

Copy link
Member

@eXpl0it3r eXpl0it3r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few minor nitpicks as comments.

  • Any real impact on requiring dl on Linux even when people aren't using Vulkan?
  • What about macOS?

examples/vulkan/CMakeLists.txt Outdated Show resolved Hide resolved
examples/vulkan/Vulkan.cpp Outdated Show resolved Hide resolved
examples/vulkan/Vulkan.cpp Outdated Show resolved Hide resolved
examples/vulkan/Vulkan.cpp Show resolved Hide resolved
include/SFML/Window/WindowBase.hpp Show resolved Hide resolved
include/SFML/Window/Vulkan.hpp Show resolved Hide resolved
src/SFML/Window/Unix/VulkanImplX11.cpp Show resolved Hide resolved
src/SFML/Window/Vulkan.cpp Outdated Show resolved Hide resolved
src/SFML/Window/Win32/VulkanImplWin32.cpp Outdated Show resolved Hide resolved
src/SFML/Window/Win32/VulkanImplWin32.cpp Outdated Show resolved Hide resolved
@binary1248
Copy link
Member Author

Updated with requested changes.

@JonnyPtn
Copy link
Contributor

JonnyPtn commented Feb 27, 2019

It doesn't work on macOS, of course (vulkan isn't available)

It's my opinion this is not a useful addition to SFML, as it's a platform specific renderer which isn't compatible with SFML's graphics module, and adds 16k lines of source code just to save a very niche group of users some boilerplate. Might be missing something though?

@binary1248 binary1248 force-pushed the feature/window_vulkan_surface branch from fce52bd to bba6627 Compare July 1, 2019 21:19
@binary1248
Copy link
Member Author

Fixed missing check for a valid device in the destructor.

@eXpl0it3r eXpl0it3r moved this from Review & Testing to Ready in SFML 2.6.0 Jul 1, 2019
@john01dav
Copy link

* Any real impact on requiring `dl` on Linux even when people aren't using Vulkan?

I use Linux and develop C++ here very frequently. There is no impact on requiring dl. Any system will have it as it is part of the system that loads shared libraries, even if it isn't explicitly being used.

@john01dav
Copy link

It's my opinion this is not a useful addition to SFML, as it's a platform specific renderer which isn't compatible with SFML's graphics module, and adds 16k lines of source code just to save a very niche group of users some boilerplate. Might be missing something though?

I disagree with your assertion that it is not a useful addition to SFML. Please let me explain why:

  • Vulkan is not a platform specific renderer. It has support on Windows, Linux, and even Mac. It also works on Android, although I am uncertain if that matters to SFML. In addition to this, there is an active initiative to make it available in as many places as possible.
  • Vulkan would be an extremely useful addition to SFML to allow SFML to be used an OS-abstraction layer with a very good C++ API instead of some awful C monstrosity from the likes of SDL or native windowing APIs, which is how I use SFML. This works well with OpenGl as I can create a context with SFML and then not use SFML to render to it. It would be very useful to be able to do the same think with Vulkan, and thus would allow SFML to serve as a better OS-abstraction layer, which, to me, is its best quality.
  • Without this patch, the best alternative is to bring in some other C library with a far inferior API to that of SFML, something that is best avoided both due to the inferior API and due to the extra dependency.

@eXpl0it3r
Copy link
Member

Anyone else willing to review this?

@eXpl0it3r
Copy link
Member

Looks like there are no further reviews, so I'll merge this PR soon.

@eXpl0it3r eXpl0it3r merged commit 6272f85 into master Oct 16, 2019
SFML 2.6.0 automation moved this from Ready to Done Oct 16, 2019
@eXpl0it3r eXpl0it3r deleted the feature/window_vulkan_surface branch October 16, 2019 19:48
ChrisThrasher added a commit to SFML/CSFML that referenced this pull request Jul 5, 2023
ChrisThrasher added a commit to SFML/CSFML that referenced this pull request Jul 5, 2023
ChrisThrasher added a commit to SFML/CSFML that referenced this pull request Jul 5, 2023
ChrisThrasher added a commit to SFML/CSFML that referenced this pull request Jul 6, 2023
ChrisThrasher added a commit to SFML/CSFML that referenced this pull request Jul 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
SFML 2.6.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants