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

Removed return type inference from Image API #990

Merged
merged 3 commits into from
Feb 13, 2023
Merged

Removed return type inference from Image API #990

merged 3 commits into from
Feb 13, 2023

Conversation

oisyn
Copy link
Contributor

@oisyn oisyn commented Feb 10, 2023

Return types are now always glam vectors. This also means that glam is 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 Vector trait, but that ripples down to some of the API functions like any() so I left it in place.

@oisyn oisyn requested a review from eddyb as a code owner February 10, 2023 12:57
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.
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.

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.

crates/spirv-std/src/lib.rs Outdated Show resolved Hide resolved
crates/spirv-std/src/image/params.rs Outdated Show resolved Hide resolved
## [Unreleased]

### Changed 🛠
- [PR#990](https://github.com/EmbarkStudios/rust-gpu/pull/990) Removed return type inference from `Image` API and made `glam` usage mandatory.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we mark this explicitly as backwards-incompatible?
Also, should we remove the optional = true from here?

glam = { version = "0.22", default-features = false, features = ["libm"], optional = true }

Copy link
Contributor Author

@oisyn oisyn Feb 13, 2023

Choose a reason for hiding this comment

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

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?

@oisyn
Copy link
Contributor Author

oisyn commented Feb 13, 2023

but I'm not sure what the decision process should be like.

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 glam obviously 😉.

Also includes an insignificant naming change
@oisyn oisyn requested a review from eddyb February 13, 2023 11:59
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.

The explanations make sense, just wanted to be sure, thanks!

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.

2 participants