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

[Merged by Bors] - Update glam to 0.13.0. #1550

Closed
wants to merge 4 commits into from

Conversation

bitshifter
Copy link
Contributor

@Ratysz
Copy link
Contributor

Ratysz commented Mar 4, 2021

Don't forget to cargo fmt.

@Ratysz
Copy link
Contributor

Ratysz commented Mar 4, 2021

Looks like there were more deprecations in this update:

error: use of deprecated associated function `bevy_math::Mat4::identity`: use Mat4::IDENTITY instead
   --> crates/bevy_core/src/bytes.rs:234:25
    |
234 |         test_round_trip(Mat4::identity());
    |                         ^^^^^^^^^^^^^^
    |
    = note: `-D deprecated` implied by `-D warnings`

@mockersf
Copy link
Member

mockersf commented Mar 4, 2021

What do you think of replacing VecN::default() with VecN::ZERO to benefit there also from the new consts? As a plus I find it clearer when reading the code to know directly what the vec used is instead of remembering "default is zero for vec"

@bitshifter
Copy link
Contributor Author

@mockersf I agree, I think VecN::ZERO would be a bit clearer intent wise than VecN::default(). Other than that, it might remove a function call in unoptimised builds but I think it's probably much of a muchness otherwise.

@Ratysz
Copy link
Contributor

Ratysz commented Mar 4, 2021

Please resolve warnings locally first?

@bitshifter
Copy link
Contributor Author

@Ratysz I did build locally but I did not see these warnings. They appear to be from iOS examples, perhaps they aren't part of the standard build?

@Ratysz
Copy link
Contributor

Ratysz commented Mar 4, 2021

The one I linked runs cargo clippy --workspace --all-targets --all-features -- -D warnings -A clippy::type_complexity -A clippy::manual-strip on Linux, and the warning comes from an example.

@bitshifter
Copy link
Contributor Author

Does that require nightly? failed to build for me at all on Rust stable 1.50 on Windows:

error: use of a blacklisted/placeholder name `foo`
   --> crates\bevy_tasks\src\task_pool.rs:283:13
    |
283 |         let foo = Box::new(42);
    |             ^^^
    |
    = note: `-D clippy::blacklisted-name` implied by `-D warnings`

etc.

@bitshifter
Copy link
Contributor Author

The file that is failing now was added at some point after I made this PR, which is why I didn't see it. I need to rebase with main.

@Ratysz
Copy link
Contributor

Ratysz commented Mar 4, 2021

That's odd. I know this sounds silly, but it works on my machine 😄

@bitshifter
Copy link
Contributor Author

It worked after rebasing.

Copy link
Member

@alec-deason alec-deason left a comment

Choose a reason for hiding this comment

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

I'm excited to see this one land.

@cart
Copy link
Member

cart commented Mar 6, 2021

Thanks for keeping us up to date! Looks good to me.

@cart
Copy link
Member

cart commented Mar 6, 2021

We will need to update hexasphere before merging to avoid duplicate deps: https://github.com/OptimisticPeach/hexasphere
@OptimisticPeach down to do an update?

@OptimisticPeach
Copy link
Contributor

Of course! I'll have it ready in an hour or so.

@OptimisticPeach
Copy link
Contributor

Half an hour, actually, hexasphere 3.2.0 is up.

Related: how would you feel if I were to rewrite/refactor hexasphere to be generic over the vector used in a separate crate (and then use that in bevy), so updating glam is seamless?

@cart
Copy link
Member

cart commented Mar 6, 2021

That sounds good to me!

@cart
Copy link
Member

cart commented Mar 6, 2021

bors r+

bors bot pushed a commit that referenced this pull request Mar 6, 2021
See https://github.com/bitshifter/glam-rs/blob/master/CHANGELOG.md for details on changes.

Co-authored-by: Cameron Hart <c_hart@wargaming.net>
@bors
Copy link
Contributor

bors bot commented Mar 6, 2021

@bors bors bot changed the title Update glam to 0.13.0. [Merged by Bors] - Update glam to 0.13.0. Mar 6, 2021
@bors bors bot closed this Mar 6, 2021
@bitshifter bitshifter deleted the glam-0.13.0 branch March 7, 2021 07:47
bors bot pushed a commit that referenced this pull request Mar 13, 2021
…1645)

it's a followup of #1550 

I think calling explicit methods/values instead of default makes the code easier to read: "what is `Quat::default()`" vs "Oh, it's `Quat::IDENTITY`"

`Transform::identity()` and `GlobalTransform::identity()` can also be consts and I replaced the calls to their `default()` impl with `identity()`
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.

6 participants