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 Const Generic Image Type #359

Merged
merged 6 commits into from
Apr 28, 2021
Merged

Add Const Generic Image Type #359

merged 6 commits into from
Apr 28, 2021

Conversation

XAMPPRocky
Copy link
Member

@XAMPPRocky XAMPPRocky commented Dec 30, 2020

This PR refactors Image in the backend to not use attributes to determine things like dimensionality and depth, and to instead accept them as const generics as parameters. This has the advantage of only needing a single Image type as opposed previously to requiring a new type for each combination, and also allows us to have enums to represent all of the possibilities rather than using u32s directly.

Additions

  • macros::Image a new procedural macro for defining Image types.
  • image::SampleType a trait that allows us to be generic of the underlying sampled type of the image.
  • image::ImageCoordinate a trait built on top of Vector for parameterising image coordinate vectors based on the sampled type, dimensionality, and arrayed-ness of the Image.
  • A new spirv_types crate for holding types that are useful in both spirv-std and spirv-std-macros.

Re-works

  • crate/spirv-std-macros has been moved into the spirv-std folder as macros beside the shared crate, this keeps the folder structure cleaner.

@expenses

This comment has been minimized.

@XAMPPRocky

This comment has been minimized.

examples/shaders/sky-shader/src/lib.rs Outdated Show resolved Hide resolved
rust-toolchain Outdated Show resolved Hide resolved
@XAMPPRocky XAMPPRocky force-pushed the image-const-generics branch 8 times, most recently from afd1ab8 to 93c683b Compare January 7, 2021 13:20
@XAMPPRocky

This comment has been minimized.

@XAMPPRocky XAMPPRocky marked this pull request as ready for review January 14, 2021 09:50
@khyperia

This comment has been minimized.

@khyperia khyperia requested a review from VZout January 14, 2021 09:54
@VZout

This comment has been minimized.

@XAMPPRocky

This comment has been minimized.

@khyperia

This comment has been minimized.

@XAMPPRocky

This comment has been minimized.

@XAMPPRocky

This comment has been minimized.

@XAMPPRocky XAMPPRocky force-pushed the image-const-generics branch 2 times, most recently from ed67d48 to 8ccad08 Compare January 22, 2021 08:44
@XAMPPRocky XAMPPRocky force-pushed the image-const-generics branch 2 times, most recently from 25a5bb9 to bb6ae0a Compare April 22, 2021 11:24
@XAMPPRocky XAMPPRocky force-pushed the image-const-generics branch 2 times, most recently from e775214 to 724cee2 Compare April 26, 2021 06:34
@XAMPPRocky
Copy link
Member Author

XAMPPRocky commented Apr 26, 2021

Alright, I've implemented all of the feedback.

I think I might prefer nuking the old image system at the same time, having duplication for even a short amount of time is a big maintenance burden and causes confusion.

Well if we nuke the old system, we can't upgrade rust-gpu in Ark until C-like enums are stabilised or we change the parts of Ark to not use reflection for entry points. So I do think it's worth keeping them both for a short time, I've also added a deprecated message if you have the const generics feature enabled, so if you can use it, it warns to move to the new type.

Copy link
Contributor

@khyperia khyperia left a comment

Choose a reason for hiding this comment

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

LGTM pending nits.

Just realized, do we want to export common type aliases? Say, for all the types we already have explicitly defined, e.g. pub type Image2D = Image!(2d, type=f32, sampled) - might be nice for both examples for users of how to use the macro, plus saves users having to figure out e.g. what the heck sampled means and if they want it or not.

crates/rustc_codegen_spirv/src/symbols.rs Outdated Show resolved Hide resolved
crates/spirv-builder/src/lib.rs Outdated Show resolved Hide resolved
tests/ui/image/sample_depth_reference/sample.rs Outdated Show resolved Hide resolved
@khyperia
Copy link
Contributor

khyperia commented Apr 27, 2021

Oh yeah, also adding docs to the rust-gpu book on the macro syntax would be good, but that can be done in a follow-up if you'd like (file an issue if so).

@XAMPPRocky
Copy link
Member Author

Oh yeah, also adding docs to the rust-gpu book on the macro syntax would be good, but that can be done in a follow-up if you'd like (file an issue if so).

The syntax is documented on the macro itself so when you visit the Image! API docs you'll see it there.

@khyperia
Copy link
Contributor

khyperia commented Apr 27, 2021

The syntax is documented on the macro itself so when you visit the Image! API docs you'll see it there.

People look at the book instead of the API docs when trying to figure out language constructs (where language = DSL in this case), e.g. u32 is documented both here https://doc.rust-lang.org/stable/std/primitive.u32.html and here https://doc.rust-lang.org/book/ch03-02-data-types.html . I think it'd make rust-gpu more accessible and easier to use if documentation was available in both places - something as core to graphics programming as images should be documented in the rust-gpu book, especially if it's not "normal" rust type syntax.

@XAMPPRocky XAMPPRocky merged commit f88ae5b into main Apr 28, 2021
@XAMPPRocky XAMPPRocky deleted the image-const-generics branch April 28, 2021 07:47
XAMPPRocky added a commit that referenced this pull request May 3, 2021
* Add parameterized Image type

* nits

* Update crates/spirv-std/src/lib.rs

* Update crates/rustc_codegen_spirv/src/symbols.rs

* Update crates/rustc_codegen_spirv/src/symbols.rs

* Update symbols.rs
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.

6 participants