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

"soft" remove EntityRef::get_unchecked_mut #5610

Closed

Conversation

BoxyUwU
Copy link
Member

@BoxyUwU BoxyUwU commented Aug 7, 2022

Objective

EntityRef::get_unchecked_mut existing is sort of "in conflict" with the idea that the &World in EntityRef is "truly" shared readonly rather than doing interior mutable shenanigans like query (see #5588). So remove it 🤔

I am a bit apprehensive about 'just' removing this method since I'm not convinced we have enough interior mutability/world splitting primitives in bevy rn to cover all of the uses that this method might have. Particularly I think you could use this method to accomplish a wildly unsafe unchecked "entity granular borrowck" vs the way Query works which is "archetype component id granular" and we don't have an API to do that in bevy atm.

Unfortunately (and somewhat ironically) Query::get_component_unchecked_mut uses this fn so instead of outright removing, it's just been made pub(crate). I am not sure if it is better to instead call the "internal" component getter methods in entity_ref.rs instead of this.

need to think about this all a bit more before this is merged I think


Changelog

  • EntityRef::get_unchecked_mut has been removed

Migration Guide

Try rearchitecting your code to not require this ( I know not very helpful ), if you have a good use case please share what it is so that it can help inform designing a good API to replace EntityRef::get_unchecked_mut :)

@BoxyUwU BoxyUwU force-pushed the entity_ref_rs_lifetime_cleanup branch from 937d45a to e6d1905 Compare August 7, 2022 21:02
@BoxyUwU BoxyUwU changed the title lifetime related clean up of entity_ref.rs "soft" remove 'EntityRef::get_unchecked_mut Aug 7, 2022
@BoxyUwU BoxyUwU changed the title "soft" remove 'EntityRef::get_unchecked_mut "soft" remove EntityRef::get_unchecked_mut Aug 7, 2022
@BoxyUwU BoxyUwU added the A-ECS Entities, components, systems, and events label Aug 7, 2022
@BoxyUwU BoxyUwU marked this pull request as draft August 7, 2022 22:36
@alice-i-cecile alice-i-cecile added the C-Code-Quality A section of code that is hard to understand or change label Aug 8, 2022
@BoxyUwU BoxyUwU added the P-Unsound A bug that results in undefined compiler behavior label Jan 10, 2023
@BoxyUwU
Copy link
Member Author

BoxyUwU commented Jan 15, 2023

Closing this in favor of #6404 which introduces an InteriorMutableWorld type that can be used by callers of this method in a more principled way, allowing us to fully remove this method instead of privating

@BoxyUwU BoxyUwU closed this Jan 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change P-Unsound A bug that results in undefined compiler behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants