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

[Merged by Bors] - Dedupe move logic in remove_bundle and remove_bundle_intersection #2521

Closed
wants to merge 6 commits into from
Closed

[Merged by Bors] - Dedupe move logic in remove_bundle and remove_bundle_intersection #2521

wants to merge 6 commits into from

Conversation

BoxyUwU
Copy link
Member

@BoxyUwU BoxyUwU commented Jul 22, 2021

This logic was in both remove_bundle and remove_bundle_intersection but only differed by whether we call .._forget_missing_.. or .._drop_missing_..

@github-actions github-actions bot added S-Needs-Triage This issue needs to be labelled S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work labels Jul 22, 2021
@mockersf
Copy link
Member

while I like const generics, what's the benefit here over an additional parameter?

@BoxyUwU
Copy link
Member Author

BoxyUwU commented Jul 22, 2021

I was thinking that it would mean that the branch wouldn't be there 🤔 i would hope this gets inlined and the branch to get optimized out even if this was a parameter though

@OptimisticPeach
Copy link
Contributor

Yeah, the monomorphization would definitely help reduce the chance of branching.

Whether it's worth it or not should (imo) be based on how often this function would hypothetically be called.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change C-Performance A change motivated by improving speed, memory usage or compile times and removed S-Needs-Triage This issue needs to be labelled S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work labels Jul 22, 2021
@cart cart added the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 23, 2021
@mockersf mockersf removed the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 24, 2021
@alice-i-cecile alice-i-cecile 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 Jul 24, 2021
cart
cart reviewed Jul 27, 2021
View reviewed changes
Copy link
Member

@cart cart 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 to me! Just one nit.

crates/bevy_ecs/src/world/entity_ref.rs Outdated Show resolved Hide resolved
@cart
Copy link
Member

cart commented Jul 27, 2021

bors r+

bors bot pushed a commit that referenced this pull request Jul 27, 2021
@BoxyUwU
Dedupe move logic in remove_bundle and remove_bundle_intersection (#2521
c83a184 
)

This logic was in both `remove_bundle` and ` remove_bundle_intersection` but only differed by whether we call `.._forget_missing_..` or `.._drop_missing_..`
@bors
Copy link
Contributor

bors bot commented Jul 27, 2021

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Dedupe move logic in remove_bundle and remove_bundle_intersection [Merged by Bors] - Dedupe move logic in remove_bundle and remove_bundle_intersection Jul 27, 2021
@bors bors bot closed this Jul 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cart cart cart left review comments

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

@mockersf mockersf mockersf approved these changes

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 C-Performance A change motivated by improving speed, memory usage or compile times 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.

None yet
5 participants
@BoxyUwU @mockersf @OptimisticPeach @cart @alice-i-cecile