Decouple glam from spirv-std, add const-generics feature flag. #476

XAMPPRocky · 2021-03-08T12:01:25Z

This PR removes glam as a dependency from spirv-std. Since we still use it for our tests, we currently have to patch it with a forked version of glam that contains the trait implementations we need until glam can depend on these changes directly. It also exports derivative::derive_impl as a hidden export so that glam can use it in its implementation.

This PR also introduces a new const-generics feature flag, and reintroduces some of the const generics in order to be able enforce the restrictions on these types through the Vector trait. Having these under a feature flag, will allow us to iterate and add condt generic code without affecting Ark until they are released on stable at the end of the month, and then we can remove it for primitives.

glam branch diff

crates/spirv-std/src/arch.rs

examples/shaders/shared/Cargo.toml

crates/spirv-std/src/derivative.rs

crates/spirv-std/src/textures.rs

XAMPPRocky · 2021-03-12T10:22:10Z

This should be ready for review. The main change is that I've added a new proc-macro called vectorized where it automatically creates a vector version of the function based on the arguments of the original function.

crates/spirv-std/src/vector.rs

crates/spirv-std/src/float.rs

eddyb

LGTM, modulo the concerns about whether the traits should be unsafe trait.

XAMPPRocky · 2021-03-15T08:19:03Z

@khyperia This should be ready for a final review.

khyperia

LGTM pending some outdated stuff

crates/spirv-builder/src/test/basic.rs

Already approved.

XAMPPRocky · 2021-03-16T08:51:49Z

I need one of you to give me the green checkmark to merge. 😄

khyperia

approved pending two more things, sorry!

khyperia · 2021-03-16T08:54:22Z

crates/spirv-builder/src/test/basic.rs

    let element = unsafe { spirv_std::arch::vector_extract_dynamic(vector, 1) };
    assert!(2.0 == element);
+    let uvector = glam::UVec2::new(1, 2);
+    let element: u32 = unsafe { spirv_std::arch::vector_extract_dynamic(uvector, 1) };


sorry, meant to include this in my comment too but didn't want to spam too many comments

Suggested change

let element: u32 = unsafe { spirv_std::arch::vector_extract_dynamic(uvector, 1) };

let element = unsafe { spirv_std::arch::vector_extract_dynamic(uvector, 1) };

Spam me with review comments 😄

khyperia · 2021-03-16T08:54:52Z

crates/spirv-builder/src/test/basic.rs

 #[spirv(fragment)]
 pub fn main(image: UniformConstant<StorageImage2d>, mut output: Output<glam::Vec2>) {
-    let coords = image.read(glam::IVec2::new(0, 1));
+    let coords =  image.read(glam::IVec2::new(0, 1));


nit: formatting

Suggested change

let coords = image.read(glam::IVec2::new(0, 1));

let coords = image.read(glam::IVec2::new(0, 1));

khyperia · 2021-03-16T09:00:26Z

oh. silly me, didn't see the automerge enable, that's why I didn't approve in my previous comment :P

XAMPPRocky · 2021-03-16T09:02:48Z

I'll make a followup PR for those

XAMPPRocky requested review from eddyb and khyperia March 8, 2021 12:01

XAMPPRocky requested review from VZout and fu5ha as code owners March 8, 2021 12:01

XAMPPRocky force-pushed the decouple-glam branch 2 times, most recently from b85d4f6 to 008ccc6 Compare March 8, 2021 12:04

VZout removed their request for review March 8, 2021 12:09

XAMPPRocky force-pushed the decouple-glam branch 2 times, most recently from 0f48669 to e34f2ec Compare March 8, 2021 12:24

XAMPPRocky mentioned this pull request Mar 8, 2021

Add spirv module bitshifter/glam-rs#142

Merged

XAMPPRocky force-pushed the decouple-glam branch from e34f2ec to 0baa4d7 Compare March 8, 2021 12:35

fu5ha removed their request for review March 8, 2021 20:35

XAMPPRocky force-pushed the decouple-glam branch from 0baa4d7 to 5327419 Compare March 9, 2021 08:05

khyperia previously requested changes Mar 11, 2021

View reviewed changes

crates/spirv-std/src/arch.rs Outdated Show resolved Hide resolved

examples/shaders/shared/Cargo.toml Outdated Show resolved Hide resolved

crates/spirv-std/src/derivative.rs Show resolved Hide resolved

crates/spirv-std/src/textures.rs Outdated Show resolved Hide resolved

XAMPPRocky force-pushed the decouple-glam branch 9 times, most recently from a3be4c6 to f59221c Compare March 12, 2021 10:01

XAMPPRocky requested a review from khyperia March 12, 2021 10:15

XAMPPRocky mentioned this pull request Mar 12, 2021

Add arch functions for all non-Matrix arithmetic instructions #446

Merged

eddyb reviewed Mar 12, 2021

View reviewed changes

crates/spirv-std/src/vector.rs Outdated Show resolved Hide resolved

eddyb reviewed Mar 12, 2021

View reviewed changes

crates/spirv-std/src/float.rs Outdated Show resolved Hide resolved

eddyb reviewed Mar 12, 2021

View reviewed changes

XAMPPRocky force-pushed the decouple-glam branch from f59221c to d2e9d36 Compare March 15, 2021 07:37

khyperia reviewed Mar 16, 2021

View reviewed changes

crates/spirv-builder/src/test/basic.rs Outdated Show resolved Hide resolved

crates/spirv-builder/src/test/basic.rs Outdated Show resolved Hide resolved

Decouple glam from spirv-std

1d0ca42

XAMPPRocky force-pushed the decouple-glam branch from d2e9d36 to 1d0ca42 Compare March 16, 2021 08:20

XAMPPRocky enabled auto-merge (squash) March 16, 2021 08:27

XAMPPRocky requested review from eddyb and khyperia March 16, 2021 08:51

khyperia approved these changes Mar 16, 2021

View reviewed changes

XAMPPRocky merged commit 6e8453f into main Mar 16, 2021

XAMPPRocky deleted the decouple-glam branch March 16, 2021 08:59

msiglreith mentioned this pull request Apr 20, 2021

Generic types for image operations #456

Closed

	let element: u32 = unsafe { spirv_std::arch::vector_extract_dynamic(uvector, 1) };
	let element = unsafe { spirv_std::arch::vector_extract_dynamic(uvector, 1) };

	let coords = image.read(glam::IVec2::new(0, 1));
	let coords = image.read(glam::IVec2::new(0, 1));

Decouple glam from spirv-std, add const-generics feature flag. #476

Decouple glam from spirv-std, add const-generics feature flag. #476

Uh oh!

Conversation

XAMPPRocky commented Mar 8, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

XAMPPRocky commented Mar 12, 2021

Uh oh!

Uh oh!

Uh oh!

eddyb left a comment

Choose a reason for hiding this comment

Uh oh!

XAMPPRocky commented Mar 15, 2021

Uh oh!

khyperia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

XAMPPRocky commented Mar 16, 2021

Uh oh!

khyperia left a comment

Choose a reason for hiding this comment

Uh oh!

khyperia Mar 16, 2021

Choose a reason for hiding this comment

Uh oh!

XAMPPRocky Mar 16, 2021

Choose a reason for hiding this comment

Uh oh!

khyperia Mar 16, 2021

Choose a reason for hiding this comment

Uh oh!

khyperia commented Mar 16, 2021

Uh oh!

XAMPPRocky commented Mar 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants