Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Refactor AllocationCreateInfo #2174

Merged
merged 2 commits into from
Apr 2, 2023

Conversation

marc0246
Copy link
Contributor

@marc0246 marc0246 commented Apr 1, 2023

This refactors AllocationCreateInfo such that it can be used together with BufferCreateInfo and, in the future, ImageCreateInfo, to construct buffers and images, removing BufferAllocateInfo. Doing this before users start using this API in 0.33.

What is still unresolved is whether ignoring BufferCreateInfo::size when in these constructor functions is the right way.

Changelog:
Add:

### Breaking changes
- `AllocationCreateInfo::{requirements, allocation_type, dedicated_allocation}` fields were removed.
- Added the parameters `requirements`, `allocation_type`, `dedicated_allocation` to `MemoryAllocator::allocate[_unchecked]`.

@Rua
Copy link
Contributor

Rua commented Apr 2, 2023

I see two other options for size:

  • Require it to be always zero.
  • Pass it on to the buffer creation when it's not zero, allowing the user to override the size if they want.

@marc0246
Copy link
Contributor Author

marc0246 commented Apr 2, 2023

If the user can override the size, then that would cause issues if it's smaller than required for T, or [T] given some len. And in the case where the size is larger, that would result in the user not being able to access the rest of the bytes in those cases. Therefore I think that the only viable option is nr 1, at least for now.

@Rua
Copy link
Contributor

Rua commented Apr 2, 2023

Let's go with that then. I think requiring a fixed value, especially one that is otherwise disallowed, is better than just ignoring the value. It allows us to change our mind later.

@marc0246
Copy link
Contributor Author

marc0246 commented Apr 2, 2023

Yes, I very much agree. That's what is currenly in the code, except that the requirement for it to be zero is only a debug_assert. Do you want me to make it stronger, and/or anything else?

@Rua
Copy link
Contributor

Rua commented Apr 2, 2023

Yeah I think a full assert would be better.

@marc0246
Copy link
Contributor Author

marc0246 commented Apr 2, 2023

Alrighty done.

@Rua Rua merged commit 59be815 into vulkano-rs:master Apr 2, 2023
@marc0246 marc0246 deleted the refactor-allocation-create-info branch April 2, 2023 13:07
Rua added a commit that referenced this pull request Apr 2, 2023
hakolao pushed a commit to hakolao/vulkano that referenced this pull request Feb 20, 2024
* Refactor `AllocationCreateInfo`

* `debug_assert` -> `assert`
hakolao pushed a commit to hakolao/vulkano that referenced this pull request Feb 20, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants