Conversation
Return types are now always `glam` vectors. This also means that `glam` is required. The "glam" feature toggle is made mandatory, we may want to support other specific vector libraries in the future.
eddyb
left a comment
There was a problem hiding this comment.
The bulk of the changes LGTM, only having some questions. Also, are we sure we're doing this? It makes a lot of sense, from a technical standpoint, but I'm not sure what the decision process should be like.
| ## [Unreleased] | ||
|
|
||
| ### Changed 🛠 | ||
| - [PR#990](https://github.com/EmbarkStudios/rust-gpu/pull/990) Removed return type inference from `Image` API and made `glam` usage mandatory. |
There was a problem hiding this comment.
Should we mark this explicitly as backwards-incompatible?
Also, should we remove the optional = true from here?
rust-gpu/crates/spirv-std/Cargo.toml
Line 15 in fd4da16
There was a problem hiding this comment.
Should we mark this explicitly as backwards-incompatible?
Not sure what you mean?
Also, should we remove the optional = true from here?
Yeah I thought about that. The reason I kept it as non-default is that I don't want to silently force glam onto people that are currently NOT using glam. Especially if we want to add support for some other vector library, you'd need to consciously make the choice which one you want. Do you agree with this reasoning?
In what way, exactly? I've openly discussed making glam mandatory in #rust-gpu, everybody seemed to agree. I don't think the technical change ended up being that big after all. I did play around with more drastic proof of concepts but kept scratching those as I wasn't yet familiar with all the intrinsic details (if I did wanted to go through with one of those I would've definitely ran it by you and the others first before committing to a full implementation). It's fully backwards compatible. Well, only if you're using |
Also includes an insignificant naming change
eddyb
left a comment
There was a problem hiding this comment.
The explanations make sense, just wanted to be sure, thanks!
Return types are now always
glamvectors. This also means thatglamis required; the "glam" feature toggle is made mandatory rather thn removed as we may want to support other specific vector libraries in the future, and this way people get a clear error when they use something else.I considered removing the
Vectortrait, but that ripples down to some of the API functions likeany()so I left it in place.