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.

Sign up for GitHub

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

Jump to bottom

Upgrade to ash 0.38 #2510

Merged
merged 2 commits into from
Apr 7, 2024
Merged

Upgrade to ash 0.38 #2510

merged 2 commits into from
Apr 7, 2024

Conversation

MarijnS95
Copy link
Contributor

@MarijnS95 MarijnS95 commented Apr 2, 2024 •
edited
Loading

https://github.com/ash-rs/ash/releases/tag/0.38.0

Code compiles, but there's still quite some more cleanup to do across autogenerated files. A future change should more rigorously use the new builder pattern directly on Vulkan structs, so that the borrow checker can accurately track any lifetime and mutable-reference issues on all PerXXX structs that are now marked 'static.

TODO

  • Clean up unnecessary duplication in autogen scripts.
  • Tidy up lifetime changes now that they're easy to spot in the diff; or do we postpone them to a separate PR?
  • Update documentation to reflect any user-facing changes - in this repository.
  • Make sure that the changes are covered by unit-tests.
  • Run cargo clippy on the changes.
  • Run cargo +nightly fmt on the changes.
  • Please put changelog entries in the description of this Pull Request
    if knowledge of this change could be valuable to users. No need to put the
    entries to the changelog directly, they will be transferred to the changelog
    file by maintainers right after the Pull Request merge.
    Please remove any items from the template below that are not applicable.
  • Describe in common words what is the purpose of this change, related
    Github Issues, and highlight important implementation aspects.

Changelog:

### Public dependency updates
- [ash](https://crates.io/crates/ash) 0.38.0

### Breaking changes
Changes to `Surface`:
- Changed `from_win32()` argument types to take `HWND` and `HINSTANCE` as `isize`.

Renamed `PipelineStages::SUBPASS_SHADING` to PipelineStages::SUBPASS_SHADER`, following upstream Vulkan changes.

MarijnS95
MarijnS95 commented Apr 2, 2024
View reviewed changes
vulkano/src/sync/pipeline.rs Outdated
SUBPASS_SHADING, SubpassShading = SUBPASS_SHADING_HUAWEI
// TODO: SUBPASS_SHADER?
SUBPASS_SHADING, SubpassShading = SUBPASS_SHADER_HUAWEI
Copy link
Contributor Author

Choose a reason for hiding this comment

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

                <enum bitpos="39" extends="VkPipelineStageFlagBits2"        name="VK_PIPELINE_STAGE_2_SUBPASS_SHADER_BIT_HUAWEI"/>
                <enum             extends="VkPipelineStageFlagBits2"        name="VK_PIPELINE_STAGE_2_SUBPASS_SHADING_BIT_HUAWEI" alias="VK_PIPELINE_STAGE_2_SUBPASS_SHADER_BIT_HUAWEI" deprecated="aliased"/>

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah this should be SUBPASS_SHADER now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah the naming was fixed for pipeline stages, but nowhere else (probably because it makes less sense). Still unsure what to do about subpass shading shader 😬

Copy link
Contributor

Choose a reason for hiding this comment

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

Apparently that's actually what Vulkan calls it!
https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VkPipelineStageFlagBits2.html#_description

Copy link
Contributor

Choose a reason for hiding this comment

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

I raised the question with the docs repo: KhronosGroup/Vulkan-Docs#2343

@0xcaff 0xcaff mentioned this pull request Apr 3, 2024
Merged
@Rua
Copy link
Contributor

Rua commented Apr 3, 2024

What still needs to be done for this PR?

@MarijnS95
Copy link
Contributor Author

MarijnS95 commented Apr 3, 2024 •
edited
Loading

I'm fairly certain to have written a TODO list via gh pr create when submitting this, but only the standard template entries remain (still need to be done anyway). Edited them into the PR description.

@Rua
Copy link
Contributor

Rua commented Apr 3, 2024

Regarding the change of *mut c_void to HANDLE, they don't seem to be the same type. In C it's a typedef to void*, which is equivalent to *mut c_void in Rust. Changing it to anything else would be a breaking change.

@0xcaff
Copy link
Contributor

0xcaff commented Apr 4, 2024

Regarding the change of *mut c_void to HANDLE, they don't seem to be the same type. In C it's a typedef to void*, which is equivalent to *mut c_void in Rust. Changing it to anything else would be a breaking change.

For what it is worth, raw-window-handle treated this as a breaking change. rust-windowing/raw-window-handle@69d9eac#diff-ee9da70e8a36920d0c024abb7c25308cb435c3dcba46a3566abca2a11fd1cfffL36-R34

I think this is the right thing to do to maintain semantic versioning correctness.

@Rua
Copy link
Contributor

Rua commented Apr 4, 2024

Sounds good.

@0xcaff 0xcaff mentioned this pull request Apr 5, 2024
Merged
@MarijnS95
Copy link
Contributor Author

Yes, we followed suit on the isize change.

MarijnS95
MarijnS95 commented Apr 6, 2024
View reviewed changes
vulkano/autogen/mod.rs
Comment on lines +446 to +460
// TODO: resolve aliases into CommandDefinition immediately?
if let RegistryChild::Commands(commands) = child {
return Some(commands.children.iter().map(|c| {
(
match c {
Command::Alias { name, .. } => name.as_str(),
Command::Definition(d) => d.proto.name.as_str(),
_ => todo!(),
},
c,
)
}));
}
None
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thoughts? This would save on the extra code in autogen/fns.rs.

Copy link
Contributor

Choose a reason for hiding this comment

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

I plan to refactor a lot of the autogen stuff, so I will have a look at that as well then.

@MarijnS95 MarijnS95 requested a review from Rua April 6, 2024 17:51
@MarijnS95 MarijnS95 marked this pull request as ready for review April 6, 2024 17:54
@Rua Rua merged commit 77abaff into vulkano-rs:master Apr 7, 2024
5 checks passed
Rua added a commit that referenced this pull request Apr 7, 2024
@Rua
#2510 changelog
b42fc43
@0xcaff 0xcaff mentioned this pull request Apr 7, 2024
Merged
@MarijnS95 MarijnS95 deleted the ash-0.38 branch April 7, 2024 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rua Rua Awaiting requested review from Rua

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MarijnS95 @Rua @0xcaff