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

extensions/khr: Add VK_KHR_external_fence_fd wrapper #413

Merged
merged 1 commit into from
Mar 26, 2021

Conversation

MarijnS95
Copy link
Collaborator

This extension wrapper is identical to the one for VK_KHR_external_semaphore_fd introduced in #395 with 'semaphore' substitued for 'fence'.

@MarijnS95
Copy link
Collaborator Author

@MaikKlein Given your question about POSIX fd's in #395 (comment) I've been tempting to replace the types with std::os::unix::io::RawFd for clarity (also a c_int) but that requires all these extensions to be wrapped in #[cfg(unix)]. On the one hand conditional compilation might be desired (and is in some areas favoured by me) to disallow use of interfaces on a platform that couldn't possibly work at compile-time, on the other applications might (already) be doing just fine with runtime conditionals.

Either way we should probably be nice and provide the Win32 variants of these import/export functions as well.

@Ralith
Copy link
Collaborator

Ralith commented Mar 23, 2021

I think it's preferable to be able to build on any platform, so an application can rely on e.g. advertised extension support to select code paths without any special effort.

@MarijnS95
Copy link
Collaborator Author

MarijnS95 commented Mar 23, 2021

@Ralith For one it's a nice way to force users to write:

let extensions = vec![
    ...
    #[cfg(unix)]
    ash::extensions::khr::ExternalFenceFd::name().as_ptr(),
    ...
];

When creating this list of desired extensions. Same when using this API, these codepaths already have to be made optional to function correctly which might as well be enforced at compile time? For example when porting a Linux/UNIX application using this API to Windows it is immediately clear that this isn't going to work, rather than having to find that out at runtime.

EDIT: I personally appreciated Rusts (and the crates around here) handholding when it comes to not exposing features that are not supported on a platform, rather than running into obscure errors at runtime. At the same time I can see that it might be desired to look at supported extensions and go from there, even though these extensions are rather specific and already need a platform-specific abstraction (Fd vs Win32 handle) anyway.

@Ralith
Copy link
Collaborator

Ralith commented Mar 23, 2021

It's already fairly common in Vulkan to branch between significantly different code paths depending on device capabilities, after all.

This extension wrapper is identical to the one for
VK_KHR_external_semaphore_fd introduced in ash-rs#395 with 'semaphore'
substitued for 'fence'.
Copy link
Member

@MaikKlein MaikKlein left a comment

Choose a reason for hiding this comment

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

This looks good as is it with i32 as fd. We don't have platform specific types in ash currently as well.

@MaikKlein MaikKlein merged commit 8d60a7e into ash-rs:master Mar 26, 2021
@MarijnS95 MarijnS95 deleted the external-fence-fd branch March 26, 2021 19:12
@MarijnS95
Copy link
Collaborator Author

@MaikKlein Thanks! Allright, let's leave it like this, the underlying type is the same anyway. Maybe someone in the future has reasons to constrain these extensions to be available on the desired (cfg(unix)) platform only, but the other extensions are anyway already exposed as-is.

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