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

Textures! #276

Merged
merged 1 commit into from
Nov 26, 2020
Merged

Textures! #276

merged 1 commit into from
Nov 26, 2020

Conversation

khyperia
Copy link
Contributor

Partially does #204 (just the compiler parts and the minimal library part to make sure the compiler bits work - library bits can be done in a follow-up)

I have a local change to wgpu runner based off https://github.com/gfx-rs/wgpu-rs/blob/e59ea495a66ce9606c3a2bbfc2efe8c0c05d413c/examples/cube/main.rs that makes sure textures work, but, considering it changes the shader significantly (adds a texture/sampler parameter and hardcode-replaces the sky shader), I'm not sure what I should do with it.

@khyperia khyperia requested review from eddyb and VZout November 26, 2020 12:50
return None;
}
};
if args.len() != 6 && args.len() != 7 {
Copy link
Member

Choose a reason for hiding this comment

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

I think we can set a lot of defaults values. Having to set depth to 0 for 90% of textures seems a bit annoying.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't imagine this attribute being used out of spirv-std (hence me leaving it undocumented), so user convenience and defaulting and whatnot isn't necessary.

Copy link
Member

Choose a reason for hiding this comment

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

Oh okay nvm then :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we specify some of these with const generics instead of attributes?

@VZout
Copy link
Member

VZout commented Nov 26, 2020

@khyperia don't forget to add it to the guide :) (or poke the community team to write documentation for you 🙊)

Comment on lines +732 to +735
if ty.size != Size::from_bytes(4) {
cx.tcx.sess.err("#[spirv(image)] type must have size 4");
return None;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just leaving this as a note, my preference is encoding it as &Image where

extern {
    #[spirv(image)]
    type Image;
}

But I have no idea if this is workable, so we should leave it for future (if ever) experimentation.


With a hardcoded size, one fun way to stop rustc from letting you nest the layout in anything else (with any more data), is to make it a newtype of [u8; (1 << 47) - 1]. That's because you can't have a size of 247 (or larger), on a 64-bit target (and there's a similar limitation on 32-bit targets but it's closer to 232).

Copy link
Contributor

Choose a reason for hiding this comment

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

The other way to think about this is we could represent them as small as 1-byte structs, but no less, so that the offset bijectively maps to the field, and generally to avoid any ZST special-casing.

The only reason to use pointer-size is the analogy with WASM, which has e.g. "function pointers" (and more generally externref), that are abstract values which can be referred to by an usize index into a global "table", if need be.
(Is that a plausible concern/direction with SPIR-V? Could we actually have a global array of e.g. Images?)

We can also avoid any Abi special-casing by using [u8; 1], IIRC, since that should create Abi::Aggregate.
(Either way we should assert the .abi is what we expect, in case it does change in the future)

@mergify mergify bot merged commit a73f54a into main Nov 26, 2020
@mergify mergify bot deleted the textures branch November 26, 2020 13:16
This was referenced Nov 26, 2020
Copy link
Contributor

@eddyb eddyb left a comment

Choose a reason for hiding this comment

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

I guess I can't "approve" it since it was already merged, but LGTM!

@VZout
Copy link
Member

VZout commented Nov 27, 2020

Why was it merged..? It should have waited for your review. dumb bot.

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.

3 participants