Add TileStorage::drain and return removed entities in remove
#586
Conversation
Co-authored-by: Rob Parrett <[email protected]>
| /// | ||
| /// Panics if the given `tile_pos` doesn't lie within the extents of the underlying tile map. | ||
| pub fn remove(&mut self, tile_pos: &TilePos) { | ||
| self.tiles[tile_pos.to_index(&self.size)].take(); | ||
| pub fn remove(&mut self, tile_pos: &TilePos) -> Option<Entity> { |
There was a problem hiding this comment.
should we get rid of this entirely and make checked_remove the new remove while we're adding a return value? 0.16 is going to come with fallible systems too, and I can't actually think of a use case for panicking here. Its not chainable, so .unwrap() won't add length to chains of calls if panicing is preferred.
upstream preference seems to be for returned errors, especially in cases of lesser-used apis.
There was a problem hiding this comment.
That makes sense.
The whole set of methods could really use a closer look though. Maybe I should revert the remove changes and we can discuss in a separate issue?
There was a problem hiding this comment.
I don't think we need to revert, but happy to discuss more generally in a separate issue. Even if we do that sort of thing the function signatures here will match and the only difference will be "no panic".
Fixes #579
I was tempted to call this
remove_allinstead to remove a potential source of confusion about what is happening to the underlying storage. Still on the fence about this, so let me know what you think.