Skip to content

Remove upcasting methods + Cleanup interned label code #18984

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
Merged
alice-i-cecile merged 7 commits into from
May 26, 2025
Merged

Remove upcasting methods + Cleanup interned label code #18984

alice-i-cecile merged 7 commits into from
May 26, 2025

Conversation

@tim-blackbird
Copy link
Contributor

@tim-blackbird tim-blackbird commented Apr 29, 2025 •
edited
Loading

Hiya!

Objective

  • Remove upcasting methods that are no longer necessary since Rust 1.86.
  • Cleanup the interned label code.

Notes

  • I didn't try to remove the upcasting methods from bevy_reflect, as there appears to be some complexity related to remote type reflection.
  • There are likely some other upcasting methods floating around.

Testing

I ran the breakout example to check that the hashing/eq implementations of the labels are still correct.

@tim-blackbird tim-blackbird added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Apr 29, 2025
@tim-blackbird tim-blackbird added this to the 0.17 milestone Apr 29, 2025
@alice-i-cecile alice-i-cecile requested a review from MrGVSV April 29, 2025 16:13
@tim-blackbird
Copy link
Contributor Author

tim-blackbird commented Apr 29, 2025

Oops, I definitely did break something :P

@tim-blackbird tim-blackbird added the M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Apr 29, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Apr 29, 2025

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

Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes.

chescock
chescock approved these changes Apr 30, 2025
View reviewed changes
crates/bevy_ecs/src/schedule/set.rs
}

fn dyn_hash(&self, mut state: &mut dyn Hasher) {
TypeId::of::<Self>().hash(&mut state);
Copy link
Contributor

@chescock chescock Apr 30, 2025

Choose a reason for hiding this comment

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

Huh, SystemSet::dyn_hash hashed the type id followed by the value, while DynHash::dyn_hash hashes the value followed by the type id. It's good that this is only implemented once now!

Copy link
Contributor Author

@tim-blackbird tim-blackbird Apr 30, 2025

Choose a reason for hiding this comment

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

I had completely overlooked the different ordering! Different hash implementations could produce some really cursed bugs. Hopefully no one hits that in the meantime 🙏

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Apr 30, 2025
@urben1680 urben1680 mentioned this pull request May 12, 2025
Closed
NthTensor
NthTensor approved these changes May 18, 2025
View reviewed changes
Copy link
Contributor

@NthTensor NthTensor left a comment

Choose a reason for hiding this comment

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

Migration guide looks good.

@urben1680 urben1680 mentioned this pull request May 21, 2025
Merged
@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 26, 2025
Merged via the queue into bevyengine:main with commit 4924cf5 May 26, 2025
36 checks passed
@PixelDust22 PixelDust22 mentioned this pull request May 31, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

@MrGVSV MrGVSV Awaiting requested review from MrGVSV

+2 more reviewers

@chescock chescock chescock approved these changes

@NthTensor NthTensor NthTensor approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change D-Straightforward Simple bug fixes and API improvements, docs, test and examples M-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

0.17

Development

Successfully merging this pull request may close these issues.

4 participants

@tim-blackbird @alice-i-cecile @chescock @NthTensor