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

Rework mapped memory, add BufferContents trait with bytemuck #1853

Merged
merged 2 commits into from Mar 6, 2022

Conversation

Rua
Copy link
Contributor

@Rua Rua commented Mar 2, 2022

Changelog:

- **Breaking** `MappedDeviceMemory` is now constructed separately with `new`, the `_and_map` variants of `DeviceMemory` are removed.
- **Breaking** Changed how `MappedDeviceMemory` handles CPU access; added `invalidate_range` and `flush_range` methods and `read` and `write` methods to get (im)mutable byte slices to the memory.
- **Breaking** Bytemuck dependencies:
  - All buffer types now require their data type parameter to implement `BufferContents`, which has `bytemuck::Pod` as a supertrait.
  - `Vertex` also has `Pod` as a supertrait.
  - Consequently, you must derive the `Zeroable` and `Pod` traits from bytemuck on any custom structures that are used as buffer contents.
  - To do this for structures generated by Vulkano-shaders, use the `types_meta` parameter as seen in the teapot example.

Fixes #1809. I decided against making all buffers be exposed to the user only as byte slices, since the typing of buffers does offer quite some convenience. This change can always be made in the future if decided. In the meantime, since buffers all contain Pod now, it's trivial to convert to and from bytes.

@Eliah-Lakhin
Copy link
Contributor

Review is in progress. One small comment so far. I noticed that you have reformatted imports. I like this change. Maybe we should add rustfmt config to the repository?

@Eliah-Lakhin
Copy link
Contributor

Eliah-Lakhin commented Mar 6, 2022

@Rua I finished testing. It works fine on my end.

Regarding the conversion to Pod. Do you think it would be safe to auto-implement Pod and Zeroable for all shader generated types? In this case I think we should advance vulkano-shaders macro as well.

At this moment if the user didn't specify types_meta section in the macro invocation, the macro auto-implements Clone and Copy traits only: https://github.com/vulkano-rs/vulkano/blob/master/vulkano-shaders/src/lib.rs#L268 Otherwise the user has to specify traits to implement explicitly, but there is currently no way to specify unsafe impl. The extension could be done somewhere here: https://github.com/vulkano-rs/vulkano/blob/master/vulkano-shaders/src/lib.rs#L705

@Eliah-Lakhin Eliah-Lakhin added PR: Approved. Additional fixes required. Maintainer approved the PR in general. Additional Actions required from the Author to continue. and removed PR: Approved. Additional fixes required. Maintainer approved the PR in general. Additional Actions required from the Author to continue. labels Mar 6, 2022
@Eliah-Lakhin Eliah-Lakhin merged commit c70fcc5 into vulkano-rs:master Mar 6, 2022
@Eliah-Lakhin
Copy link
Contributor

@Rua I merged it. The PR looks good, and it covers established goals. As of Vulkano shaders macro, I will do an addendum PR later. I think just having unsafe impl in the types_meta section should be enough.

Eliah-Lakhin added a commit that referenced this pull request Mar 6, 2022
@Rua
Copy link
Contributor Author

Rua commented Mar 6, 2022

I don't think an unsafe impl is needed. Look at the teapot example for how it could be done.

I do agree though that it's preferable if Vulkano-shaders derives these types automatically. But because bytemuck is an external crate, I think it should be re-exported from Vulkano if this is done. Then Vulkano-shaders can import Zeroable and Pod from vulkano::bytemuck instead and the user doesn't need to add it as a separate dependency.

@Rua Rua deleted the buffertype branch May 31, 2022 18:36
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.

CpuAccessibleBuffer methods read and write are unsound
2 participants