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 conversions between Extent2D, Extent3D and Rect2D #557

Merged
merged 1 commit into from
Jan 22, 2022

Conversation

swooster
Copy link
Contributor

When using ash, I frequently find myself wishing that converting between 2D/3D extents was less verbose, since it's so common. Likewise, you need to convert from an Extent2D to a Rect2D any time you want to start a render pass that affects an entire framebuffer.

@swooster
Copy link
Contributor Author

This is just a first-pass PR since there are some outstanding questions:

  • Should conversion from Extent3D to Extent2D require the depth to be 1? (should I be using TryFrom instead of From?)
  • What's the proper way to add this? (I just made a best guess about how to tweak the generator)
  • When I tried to regenerate everything, this caused additional changes to native.rs (e.g. changing StdVideoH264PocType from c_uint to c_int) that I've avoided committing because I don't believe my changes should have affected that. Any idea what's up with that?

Anyway, let me know if this seems sane, or how I can improve things.

@MarijnS95
Copy link
Collaborator

MarijnS95 commented Jan 18, 2022

  • Should conversion from Extent3D to Extent2D require the depth to be 1? (should I be using TryFrom instead of From?)

I'd do it the way vector libraries such as glam do: a fn extend(depth: u32) so that it is up to the user to decide, instead of a seemingly unsuspecting from/into.

EDIT: For the truncation, not really sure what makes most sense, we try to not be too opinionated, and as a result not provide too many such helpers...

  • What's the proper way to add this? (I just made a best guess about how to tweak the generator)

We're moving away from having static code in the generator. Feel free to add this in ash/src/vk/prelude.rs.

Over time we will work towards a cleaner split between handwritten files, and dump everything autogenerated in an auto/ subfolder. For now there's not much that's hand-written, so the prelude is fine.

  • When I tried to regenerate everything, this caused additional changes to native.rs (e.g. changing StdVideoH264PocType from c_uint to c_int) that I've avoided committing because I don't believe my changes should have affected that. Any idea what's up with that?

That is odd, are submodules or your local Cargo.lock (ie. bindgen) out of date? The CI uses the most recent version just as I use locally, and that change doesn't show. I don't recall those types changing there historically, though.

@MarijnS95
Copy link
Collaborator

As an aside, you may showcase your new helpers in the examples :)

@Ralith
Copy link
Collaborator

Ralith commented Jan 18, 2022

For 2D -> 3D, I think a From impl that bakes in depth = 1 makes sense both logically, because that's the natural embedding of a 2D slice in a 3D space, and practically, because it is super common to e.g. construct an Extent3D for a 2D image in ImageCreateInfo, which requires depth = 1 exactly. FRU syntax can be used if something else is required; we could also offer extend but that seems like overkill to me.

Is there a specific use case for the 3D -> 2D case?

@swooster
Copy link
Contributor Author

swooster commented Jan 18, 2022

I'd do it the way vector libraries such as glam do: a fn extend(depth: u32) so that it is up to the user to decide, instead of a seemingly unsuspecting from/into.

Unless you feel strongly about this, my reasoning is the same as in @Ralith's comment; my own use-cases (and the ash examples) have only ever had a need for a depth of 1.

EDIT: For the truncation, not really sure what makes most sense, we try to not be too opinionated, and as a result not provide too many such helpers...

I'll skip the 3D-to-2D conversion for now since that's less clear cut..? While I've encountered situations where I've wanted that, maybe it'll turn out to be easy enough to work around using 2D-to-3D conversions.

We're moving away from having static code in the generator. Feel free to add this in ash/src/vk/prelude.rs.

Ok, I've redone this to use the prelude instead of modifying the generator.

That is odd, are submodules or your local Cargo.lock (ie. bindgen) out of date? The CI uses the most recent version just as I use locally, and that change doesn't show. I don't recall those types changing there historically, though.

I don't think bindgen was out of date, as I set it up for the first time yesterday (I installed LLVM 13.0.0), but maybe I've installed it wrong? (I was just flailing around trying to get to a point where I could run the generator again)

As an aside, you may showcase your new helpers in the examples :)

Ok, examples updated.

@swooster swooster marked this pull request as ready for review January 18, 2022 23:56
@swooster
Copy link
Contributor Author

Huh... I just now noticed there are two preludes: ash::prelude and ash::vk::prelude. Which one should I have modified?

@MarijnS95
Copy link
Collaborator

Unless you feel strongly about this, my reasoning is the same as in @Ralith's comment; my own use-cases (and the ash examples) have only ever had a need for a depth of 1.

Gave his comment a thumbs-up because of that, no strong feelings either way.

I don't think bindgen was out of date, as I set it up for the first time yesterday (I installed LLVM 13.0.0), but maybe I've installed it wrong? (I was just flailing around trying to get to a point where I could run the generator again)

It might be the bindgen crate itself that the generator depends on, instead of the installable binary. Though even its library version should leverage the system clang and LLVM? Using LLVM 13 here too, no diff when running the generator. What host OS and architecture are you running on?

As an aside, you may showcase your new helpers in the examples :)

Ok, examples updated.

Shaves off a lot of code 😉

Huh... I just now noticed there are two preludes: ash::prelude and ash::vk::prelude. Which one should I have modified?

src/ash/vk/prelude.rs, as per my comment above. These helpers solely relate to the vk definitions and go there. src/ash/prelude.rs holds more general helpers that are used by the wrappers outside the vk module.

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.

D'oh I forgot to ask in my previous comment: please add an entry to ash/changelog.md :)

ash/src/vk/prelude.rs Show resolved Hide resolved
examples/src/bin/texture.rs Outdated Show resolved Hide resolved
examples/src/bin/texture.rs Outdated Show resolved Hide resolved
@swooster
Copy link
Contributor Author

It might be the bindgen crate itself that the generator depends on, instead of the installable binary. Though even its library version should leverage the system clang and LLVM? Using LLVM 13 here too, no diff when running the generator. What host OS and architecture are you running on?

I'm running Windows 10 on x86. The difference persists even after running cargo update.

@filnet
Copy link
Contributor

filnet commented Jan 19, 2022

I'm running Windows 10 on x86. The difference persists even after running cargo update.

Did you run cargo update in ash or in your project ? Got bitten by that in the past.

@MarijnS95
Copy link
Collaborator

MarijnS95 commented Jan 19, 2022

I'm running Windows 10 on x86. The difference persists even after running cargo update.

Yeah, that explains things. We had more Windows-vs-Linux bindgen issues with the Video extension in the past: #417 (comment).

For obvious reasons crates usually keep generated output separate per platform/OS, but the Video extensions are supposed to "have the same ABI" across all architectures.

EDIT: See also KhronosGroup/Vulkan-Docs#1571, won't waste too much time on it as the extension is still experimental and the type definitions are supposedly moved into xml to leverage existing generator functionality.

@swooster swooster force-pushed the add-conversions branch 2 times, most recently from d4975f5 to 03e7502 Compare January 19, 2022 10:42
@swooster
Copy link
Contributor Author

D'oh I forgot to ask in my previous comment: please add an entry to ash/changelog.md :)

I just now noticed this comment... added.

Changelog.md Outdated Show resolved Hide resolved
These two conversions occur all the time in Vulkan applications. For
example, Extent2D -> Extent3D occurs whenever you need to make an image
the same size as a surface and Extent2D -> Rect2D occurs whenever you
fill out a scissors or render area from a surface resolution.
ash/src/vk/prelude.rs Show resolved Hide resolved
@MarijnS95 MarijnS95 merged commit fbcc45f into ash-rs:master Jan 22, 2022
@swooster swooster deleted the add-conversions branch January 23, 2022 05:28
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.

4 participants