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

Use ash::Entry instead of FunctionPointers #1876

Closed
wants to merge 15 commits into from

Conversation

Neo-Zhixing
Copy link

@Neo-Zhixing Neo-Zhixing commented Apr 13, 2022

This PR removed FunctionPointers and the loader implementation in vulkano in favor of ash's implementation of the loader.

Related: #1864

@Rua
Copy link
Contributor

Rua commented Apr 13, 2022

This is a change I wanted to make myself at some point, so good idea! Some comments though:

  • I think a Vulkano type that wraps Entry would be better than using Ash's type directly. This is because there are safe wrappers that Vulkano would probably want to add to it at some point.
  • I don't think Instance::entry() is needed. I would prefer it if the user did this manually, by constructing the entry type themselves. Moreover, the unsafety of load isn't mitigated at all by this function, so that's unsound.
  • Entry isn't a very clear name. Something like VulkanLibrary might be clearer to the user.

vulkano/src/instance/mod.rs Show resolved Hide resolved
vulkano/src/instance/mod.rs Outdated Show resolved Hide resolved
vulkano/src/instance/mod.rs Show resolved Hide resolved
vulkano/src/instance/layers.rs Show resolved Hide resolved
properties.set_len(num as usize);
properties
};
pub fn supported_by_core_with_loader(entry: &ash::Entry) -> Result<Self, OomError> {
Copy link
Author

Choose a reason for hiding this comment

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

Do we want to rename this function, now that we require ash::Entry at all times?

Copy link
Contributor

Choose a reason for hiding this comment

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

A while back, the way DeviceExtensions is constructed was changed, so that supported extensions are now retrieved from PhysicalDevice. Entry (or whatever its new name will be) is a precursor to Instance in much the same way that PhysicalDevice is a precursor to Device. So instance extensions could follow this model and be retrieved from the Entry type.

@Neo-Zhixing Neo-Zhixing marked this pull request as ready for review April 13, 2022 09:18
@Neo-Zhixing
Copy link
Author

Neo-Zhixing commented Apr 13, 2022

Just some general questions before I move forward:

  1. How much backward compatibility do we want to maintain here?
  2. How do we want to allow the user to choose between linked versus loaded vulkan library? Do we want to keep them separate like ash, or do we want to maintain the current behavior (linked for iOS, loaded for everything else)

@Rua
Copy link
Contributor

Rua commented Apr 13, 2022

Backwards compatibility is not that important for the moment, as every release of Vulkano so far has breaking changes in it anyway.

I think keeping them separate the way Ash does is good. That gives the user more control over how they want their application to behave, the same level of control that Ash now exposes. True, it means that loading a dynamic library is unsafe, but really it was unsafe all along.

@Neo-Zhixing Neo-Zhixing requested a review from Rua April 14, 2022 08:33
@Rua
Copy link
Contributor

Rua commented Apr 14, 2022

This looks good. There are some changes I'd like to make, but if you don't mind I'll make those myself in my own PR rather than burdening you with all the details.

@Neo-Zhixing
Copy link
Author

Thanks @Rua ! You can add me as a reviewer to that different PR too.

@Rua
Copy link
Contributor

Rua commented Apr 14, 2022

The tests are failing, can you fix that please?

@Neo-Zhixing
Copy link
Author

Not sure why Windows was failing, both ash and vulkano should be using the same dll name which is vulkan-1.dll. I'll look into this.

@Neo-Zhixing
Copy link
Author

Neo-Zhixing commented Apr 15, 2022

@Rua

Turns out the GitHub actions Windows and macOS environment does not have Vulkan installed. That make sense - macOS don't generally have MoltenVK installed.

Err(_) => return,

Correct me if I'm wrong, but what this is saying is essentially: If we fail to load the vulkan library, or if instance creation fails for any reason, simply return from the test and mark it as success.

So for those tests that failed - they should've failed a long time ago in #1437 but didn't.

I can probably do something similar and maintain the current behavior, but this is a bug IMO and needs to be fixed. Either

  • Fix the CI pipeline so that they install Vulkan, or
  • Remove Windows and Linux from the tests

This should probably be addressed in a separate PR.

@Rua
Copy link
Contributor

Rua commented Apr 15, 2022

I ran into this problem in #1841 (comment), but it's not so easy to solve. In this PR, you can return with no error like before, and then make a new issue about it separately.

@Rua
Copy link
Contributor

Rua commented May 13, 2022

@Neo-Zhixing Are you still interested in merging this?

@Neo-Zhixing
Copy link
Author

Neo-Zhixing commented May 13, 2022

@Rua I don't have the time to fix all the unit tests so feel free to take this over or redo this. Thanks!

@Rua
Copy link
Contributor

Rua commented Jul 24, 2022

@Neo-Zhixing I made an alternative implementation #1932, that takes some pieces from your PR but with a different approach.

@Neo-Zhixing
Copy link
Author

@Rua Thanks for the work to clean this up! I'll do some reviews too

@Rua Rua closed this Jul 27, 2022
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.

None yet

2 participants