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

Bump the latest allowed glam version to 0.21 #888

Merged
merged 4 commits into from
Aug 9, 2022

Conversation

expenses
Copy link
Contributor

@expenses expenses commented Aug 2, 2022

No description provided.

@expenses expenses requested a review from eddyb as a code owner August 2, 2022 15:50
@expenses expenses requested review from fu5ha and VZout as code owners August 2, 2022 16:02
Copy link
Contributor

@repi repi left a comment

Choose a reason for hiding this comment

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

thanks!

@eddyb eddyb enabled auto-merge (rebase) August 4, 2022 00:04
@eddyb
Copy link
Contributor

eddyb commented Aug 4, 2022

  Error: Android NDK is not found. Please set the path to the Android NDK with $ANDROID_NDK_ROOT environment variable.

Did some Android-related change break our CI? (clearly not specific to this PR)

@repi
Copy link
Contributor

repi commented Aug 4, 2022

filed a separate issue about the Android CI failing on main in #890.

So think we can just merge this as is

Comment on lines 27 to 31
const MIE_K_COEFFICIENT: Vec3 = Vec3::from_array([0.686, 0.678, 0.666]);
const MIE_V: f32 = 4.0;
const MIE_ZENITH_LENGTH: f32 = 1.25e3;
const NUM_MOLECULES: f32 = 2.542e25f32;
const PRIMARIES: Vec3 = const_vec3!([6.8e-7f32, 5.5e-7f32, 4.5e-7f32]);
const PRIMARIES: Vec3 = Vec3::from_array([6.8e-7f32, 5.5e-7f32, 4.5e-7f32]);
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW glam simply made all these functions (from_array()/new()/splat()) const fn so you can also call a clean constructor directly nowadays:

Suggested change
const MIE_K_COEFFICIENT: Vec3 = Vec3::from_array([0.686, 0.678, 0.666]);
const MIE_V: f32 = 4.0;
const MIE_ZENITH_LENGTH: f32 = 1.25e3;
const NUM_MOLECULES: f32 = 2.542e25f32;
const PRIMARIES: Vec3 = const_vec3!([6.8e-7f32, 5.5e-7f32, 4.5e-7f32]);
const PRIMARIES: Vec3 = Vec3::from_array([6.8e-7f32, 5.5e-7f32, 4.5e-7f32]);
const MIE_K_COEFFICIENT: Vec3 = Vec3::new(0.686, 0.678, 0.666);
const MIE_V: f32 = 4.0;
const MIE_ZENITH_LENGTH: f32 = 1.25e3;
const NUM_MOLECULES: f32 = 2.542e25f32;
const PRIMARIES: Vec3 = const_vec3!([6.8e-7f32, 5.5e-7f32, 4.5e-7f32]);
const PRIMARIES: Vec3 = Vec3::new(6.8e-7f32, 5.5e-7f32, 4.5e-7f32);

(from_array() just calls Self::new(a[0], a[1], a[2]) under the hood)

Copy link
Contributor

Choose a reason for hiding this comment

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

But is this still backwards-compatible with >=0.17?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh true, I don't think it is now. I'll change that.

Copy link
Contributor

Choose a reason for hiding this comment

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

But is this still backwards-compatible with >=0.17?

I didn't say anything about that after figuring out myself, but basically it doesn't matter AFAICT, since this is an example shader, and it's going to use whatever we have in Cargo.lock.

Only spirv-std has to be careful to remain compatible with all the supported versions.

Copy link
Contributor

Choose a reason for hiding this comment

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

@eddyb the more reason to change this into a single version and not bother with a version range? I guess the examples and spirv-std would have to be updated in parallel anyway?

Copy link
Contributor

Choose a reason for hiding this comment

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

upgrading glam is a bit of work for us so we prefer to support a range so it can be a bit decoupled. but can support 0.20 & 0.21 in spirv-std.

this specific code change in the repo was in the examples and as Cargo.lock with this uses 0.21 that is fine to require 0.21. but spirv-std does need to support all the range, which we do not have CI verification on but generally has been working quite well before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh true, I don't think it is now. I'll change that.

I'll leave it as-is then :)

Copy link
Contributor

Choose a reason for hiding this comment

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

this specific code change in the repo was in the examples and as Cargo.lock with this uses 0.21 that is fine to require 0.21. but spirv-std does need to support all the range, which we do not have CI verification on but generally has been working quite well before.

Yeah so I think the examples should then reflect the minimal supported version, which automatically trickles down into Cargo.lock correctly without requiring to manually set/check/cargo update this Cargo.lock. That helps when someone tries -Zminimal-versions with the examples even if that's an unlikely use-case.

After all you can still leave the main spirv-std crate to represent a proper range, and test for backwards compatibility!

Copy link
Member

Choose a reason for hiding this comment

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

Agree with Marijn here I think

Comment on lines 27 to 31
const MIE_K_COEFFICIENT: Vec3 = Vec3::from_array([0.686, 0.678, 0.666]);
const MIE_V: f32 = 4.0;
const MIE_ZENITH_LENGTH: f32 = 1.25e3;
const NUM_MOLECULES: f32 = 2.542e25f32;
const PRIMARIES: Vec3 = const_vec3!([6.8e-7f32, 5.5e-7f32, 4.5e-7f32]);
const PRIMARIES: Vec3 = Vec3::from_array([6.8e-7f32, 5.5e-7f32, 4.5e-7f32]);
Copy link
Member

Choose a reason for hiding this comment

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

Agree with Marijn here I think

auto-merge was automatically disabled August 8, 2022 09:05

Head branch was pushed to by a user without write access

@expenses
Copy link
Contributor Author

expenses commented Aug 8, 2022

Okay, I reverted the changes so that the only one is the glam version range change.

After all you can still leave the main spirv-std crate to represent a proper range, and test for backwards compatibility!

Ideally, the glam version that's used in the examples would be the earliest allowed one (0.17.0) instead of the one in Cargo.lock (0.20.2). But this can be done later.

@eddyb eddyb merged commit a9ae4d2 into EmbarkStudios:main Aug 9, 2022
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.

5 participants