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 extension wrapper for VK_KHR_synchronization2 #403

Merged

Conversation

lothran
Copy link
Contributor

@lothran lothran commented Mar 15, 2021

I tried my best to implement Flag64 but i am not a rust god.

Copy link
Collaborator

@MarijnS95 MarijnS95 left a comment

Choose a reason for hiding this comment

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

We'll have to see where to leave vkCmdWriteBufferMarker2AMD and vkGetQueueCheckpointData2NV.

Don't forget to add empty lines between the functions in synchronization2.rs.

By the way, for all the functions returning void (()) you might as well remove the trailing semicolon ;. That way, if a return type is ever added to a function (or a function is copypasted to implement a new function) a missed return type is immediately a compiler error instead of just a #[must_use]-not-used warning on the vk::Result.
Unfortunately I myself have not been very consistent in doing so with previous submissions either.

Thanks!

generator/src/lib.rs Outdated Show resolved Hide resolved
generator/src/lib.rs Outdated Show resolved Hide resolved
generator/src/lib.rs Outdated Show resolved Hide resolved
generator/src/lib.rs Outdated Show resolved Hide resolved
generator/src/lib.rs Outdated Show resolved Hide resolved
ash/src/extensions/khr/synchronization2.rs Outdated Show resolved Hide resolved
ash/src/extensions/khr/synchronization2.rs Outdated Show resolved Hide resolved
ash/src/extensions/khr/synchronization2.rs Show resolved Hide resolved
ash/src/extensions/khr/synchronization2.rs Show resolved Hide resolved
generator/src/lib.rs Outdated Show resolved Hide resolved
@lothran
Copy link
Contributor Author

lothran commented Mar 16, 2021

Thanks for taking the time really helpful.
I cant think of an easy way to get the correct flag type for formatting in the const KNOWN[(Flag,&str)] array.

For the present the flag>u32::max check seems save there are only the two flag64 types from synchronization2.

@MarijnS95
Copy link
Collaborator

MarijnS95 commented Mar 20, 2021

@Caradhrass Thanks for your patience, as mentioned in #403 (comment) / #402 (comment) I have already been working on extending vk-parse with the appropriate field to distinguish the 32- and 64-bit variant, even though the all_bits > u32::MAX hack works for now; see #410 / #411 (look at the individual relevant commits, not the total PR diff, that's humongous).

As non-collaborator I am not allowed to mark my own review comments as resolved; feel free to close everything that has to be fixed so that we have a clear overview of what is done.

It remains to be seen what the best order is to merge all these PRs in; we can possibly get your solution in first including the wrapper for Synchronization2, then I'll have to do some conflict resolution to get the proper bitwidth variant on top.
However, I personally prefer to have everything in separate commits in the history: generator changes, n+1 bumps (separate commit for .169, .170, .171), etc. Though that is not up to me to decide, and the intentionally-clean history created in my PRs is anyway lost due to squash merges - unless I make a separate PR per commit 😑

@MarijnS95
Copy link
Collaborator

@Caradhrass are you available to discuss how to proceed? My plan is to take in #410 / #411 first to use updated vk-parse for accurately distinguishing 32- and 64-bit fields now and in the future. Feel free to update this PR afterwards to resolve conflicts, but I can do that for you while maintaining authorship.

Sounds good?

@lothran
Copy link
Contributor Author

lothran commented Apr 16, 2021

Sure, just keep the synchronization2 part of this and I am happy.
That should stay the same regardless of the Flag64 generator used.

@MarijnS95
Copy link
Collaborator

You've done an excellent job on the non-raw bindings so there's no reason to drop them nor change authorship 😀

I'll take care of rebasing this when the generator and flags are in, thanks!

@MarijnS95 MarijnS95 force-pushed the update-to-1.2.171+-KHR_synchronization2 branch from c813e71 to 20f4d7a Compare May 8, 2021 10:46
@MarijnS95 MarijnS95 force-pushed the update-to-1.2.171+-KHR_synchronization2 branch from 20f4d7a to d22fc11 Compare May 8, 2021 10:46
@MarijnS95 MarijnS95 changed the title Update to 1.2.171 + VK_KHR_synchronization2 Add extension wrapper for VK_KHR_synchronization2 May 8, 2021
@MarijnS95 MarijnS95 requested a review from MaikKlein May 8, 2021 10:50
@MaikKlein MaikKlein merged commit c7216f1 into ash-rs:master May 8, 2021
@MarijnS95 MarijnS95 mentioned this pull request Dec 27, 2021
55 tasks
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