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.

Sign up for GitHub

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

Jump to bottom

update syn, encase, glam and hexasphere #8573

Merged
merged 14 commits into from
May 16, 2023
Merged

update syn, encase, glam and hexasphere #8573

merged 14 commits into from
May 16, 2023

Conversation

mockersf
Copy link
Member

@mockersf mockersf commented May 8, 2023 •
edited
Loading

Objective

Blocked on teoxoy/encase#42 and on OptimisticPeach/hexasphere#17

@alice-i-cecile alice-i-cecile added C-Dependencies A change to the crates that Bevy depends on A-Math Fundamental domain-agnostic mathematical operations labels May 8, 2023
@james7132 james7132 added the S-Blocked This cannot move forward until something else changes label May 8, 2023
@mockersf mockersf mentioned this pull request May 10, 2023
Merged
nicopap
nicopap suggested changes May 10, 2023
View reviewed changes
Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

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

Looks good. I think it could benefit from a couple cleanups.

crates/bevy_reflect/bevy_reflect_derive/src/type_uuid.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/bevy_reflect_derive/src/field_attributes.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/bevy_reflect_derive/src/derive_data.rs Show resolved Hide resolved
crates/bevy_ecs/macros/src/lib.rs Show resolved Hide resolved
crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs Show resolved Hide resolved
JoJoJet
JoJoJet approved these changes May 10, 2023
View reviewed changes
Copy link
Member

@JoJoJet JoJoJet left a comment

Choose a reason for hiding this comment

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

This looks right, except for a couple of minor things.

crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/macros/src/component.rs Show resolved Hide resolved
crates/bevy_render/macros/src/as_bind_group.rs Outdated Show resolved Hide resolved
crates/bevy_render/macros/src/as_bind_group.rs Outdated Show resolved Hide resolved
@OptimisticPeach
Copy link
Contributor

Just pushed hexasphere 9.0.0. I made some important changes this time around:

  • I moved some features behind feature gates so this should reduce compilation time.
  • I moved constant computation to a compile-time constant floating point library (constgebra) which might increase compilation time, but reduce the need for our once_cell dep and remove an extra check when generating an icosphere.

If you notice anything out of the ordinary with these changes (disproportionately long compiletimes that I somehow missed on my local machine, etc.), let me know and I can revert them -- give me a ping on Discord for the fastest reply.

@mockersf mockersf changed the title update syn, encase and glam update syn, encase, glam and hexasphere May 11, 2023
@mockersf mockersf removed the S-Blocked This cannot move forward until something else changes label May 11, 2023
MrGVSV
MrGVSV approved these changes May 15, 2023
View reviewed changes
crates/bevy_reflect/bevy_reflect_derive/src/derive_data.rs Outdated
Comment on lines 151 to 155
let metas = meta_list
.parse_args_with(Punctuated::<Meta, Token![,]>::parse_terminated)?
.into_iter()
.collect();
let new_traits = ReflectTraits::from_metas(metas)?;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Not blocking, but couldn't we also just have ReflectTraits::from_metas take a Punctuated::<Meta, Token![,]>? Seems like the three places that use it collect it into a Vec like this.

crates/bevy_reflect/src/lib.rs
@@ -1636,6 +1636,24 @@ bevy_reflect::tests::should_reflect_debug::Test {
assert_eq!("Foo".to_string(), format!("{foo:?}"));
}

#[test]
fn custom_debug_function() {
Copy link
Member

Choose a reason for hiding this comment

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

Sweet, thanks for adding this!

@mockersf mockersf added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label May 15, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 16, 2023
Merged via the queue into bevyengine:main with commit 0736195 May 16, 2023
@Selene-Amanita Selene-Amanita added A-ECS Entities, components, systems, and events M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Jul 10, 2023
@github-actions
Copy link
Contributor

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MrGVSV MrGVSV MrGVSV approved these changes

@nicopap nicopap nicopap approved these changes

@JoJoJet JoJoJet JoJoJet approved these changes

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events A-Math Fundamental domain-agnostic mathematical operations C-Dependencies A change to the crates that Bevy depends on M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Consider updating from syn@1 to syn@2
8 participants
@mockersf @OptimisticPeach @alice-i-cecile @JoJoJet @nicopap @MrGVSV @james7132 @Selene-Amanita