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

Add an Entry api to EntityWorldMut. #10650

Merged
merged 20 commits into from
Nov 21, 2023

Conversation

13ros27
Copy link
Contributor

@13ros27 13ros27 commented Nov 20, 2023

Objective

Adds .entry to EntityWorldMut with Entry, OccupiedEntry and VacantEntry for easier in-situ modification, based on HashMap.entry.

Fixes #10635

Solution

This adds the entry method to EntityWorldMut which returns an Entry. This is an enum of OccupiedEntry and VacantEntry and has the methods and_modify, insert_entry, or_insert, or_insert_with and or_default. The only difference between OccupiedEntry and VacantEntry is the type, they are both a mutable reference to the EntityWorldMut and a marker for the component type, HashMap also stores things to make it quicker to access the data in OccupiedEntry but I wasn't sure if we had anything it would be logical to store to make accessing/modifying the component faster? As such, the differences are that OccupiedEntry assumes the entity has the component (because nothing else can have an EntityWorldMut so it can't be changed outside the entry api) and has different methods.

All the methods are based very closely off hashbrown::HashMap (because its easier to read the source of) with a couple of quirks like OccupiedEntry.insert doesn't return the old value because we don't appear to have an api for mem::replacing components.


Changelog

  • Added a new function EntityWorldMut.entry which returns an Entry, allowing easier in-situ modification of a component.

@viridia
Copy link
Contributor

viridia commented Nov 20, 2023

Meets my needs. Minor nit on doc comment.

@DasLixou
Copy link
Contributor

Could we also get those for commands?

@viridia
Copy link
Contributor

viridia commented Nov 20, 2023

Could we also get those for commands?

Since commands are asynchronous, this would be more difficult: you can't insert a component and get back the same component instance in the same tick (I think).

@DasLixou
Copy link
Contributor

Could we also get those for commands?

Since commands are asynchronous, this would be more difficult: you can't insert a component and get back the same component instance in the same tick (I think).

Should be possible when we introduce a EntityEntryCommands thing like EntityCommands which you can call or_insert, etc. on but when executing the command we do the entry thing, right?

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Nov 20, 2023
Co-authored-by: Pascal Hertleif <[email protected]>
@alice-i-cecile
Copy link
Member

Should be possible when we introduce a EntityEntryCommands thing like EntityCommands

Agreed, but let's leave that for another PR :)

/// assert_eq!(world.query::<&Comp>().single(&world).0, 0);
/// ```
#[inline]
pub fn or_default(self) -> Mut<'a, T> {
Copy link
Member

Choose a reason for hiding this comment

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

Might be nice to make both a FromReflect and FromWorld equivalent, but those can wait.

@alice-i-cecile alice-i-cecile added the D-Complex Quite challenging from either a design or technical perspective. Ask for help! label Nov 20, 2023
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Very clean construction, and a useful bit of API.

IMO we should replace the unwrap_unchecked with simple unwrap. Unless there's compelling benchmarks, I would much rather have surprising panics than risk UB in a refactor or edge case.

…plaining how it must be occupied before being created (it has private fields so this shouldn't be a problem for user code).
…comments explaining why it should never panic.
@13ros27
Copy link
Contributor Author

13ros27 commented Nov 20, 2023

Replacing unwrap_unchecked makes sense to me, I do plan to make some (micro-)benchmarks for this to see whether it is faster to store the archetype or something but that will wait for a future PR.

@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 Nov 20, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Nov 21, 2023
Merged via the queue into bevyengine:main with commit 865041d Nov 21, 2023
22 checks passed
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
# Objective

Adds `.entry` to `EntityWorldMut` with `Entry`, `OccupiedEntry` and
`VacantEntry` for easier in-situ modification, based on `HashMap.entry`.

Fixes bevyengine#10635

## Solution

This adds the `entry` method to `EntityWorldMut` which returns an
`Entry`. This is an enum of `OccupiedEntry` and `VacantEntry` and has
the methods `and_modify`, `insert_entry`, `or_insert`, `or_insert_with`
and `or_default`. The only difference between `OccupiedEntry` and
`VacantEntry` is the type, they are both a mutable reference to the
`EntityWorldMut` and a marker for the component type, `HashMap` also
stores things to make it quicker to access the data in `OccupiedEntry`
but I wasn't sure if we had anything it would be logical to store to
make accessing/modifying the component faster? As such, the differences
are that `OccupiedEntry` assumes the entity has the component (because
nothing else can have an `EntityWorldMut` so it can't be changed outside
the entry api) and has different methods.

All the methods are based very closely off `hashbrown::HashMap` (because
its easier to read the source of) with a couple of quirks like
`OccupiedEntry.insert` doesn't return the old value because we don't
appear to have an api for mem::replacing components.

---

## Changelog

- Added a new function `EntityWorldMut.entry` which returns an `Entry`,
allowing easier in-situ modification of a component.

---------

Co-authored-by: Alice Cecile <[email protected]>
Co-authored-by: Pascal Hertleif <[email protected]>
@13ros27 13ros27 deleted the entity_world_mut_entry branch October 4, 2024 10:33
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-Usability A targeted quality-of-life change that makes Bevy easier to use D-Complex Quite challenging from either a design or technical perspective. Ask for help! 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
Development

Successfully merging this pull request may close these issues.

Provide a convenient method for lazy Component creation based on HashMap::entry
5 participants