implement get_mut_or_default

[JaniM]

Fixes #561

Checklist

• I've added tests for all code changes and additions (where applicable)
• I've added a demonstration of the new feature to one or more examples (didn't find any relevant examples)
• I've updated the book to reflect my changes
• Usage of new public items is shown in the API docs

API changes

New function: GenericWriteStorage::get_mut_or_default for Default component types. Not breaking.

---

This change is 

[Moxinilian]

I don't think an unwrap should be best here. Maybe at least an expect.
The one under does not need an except, but maybe an explanation in the comment explaining why it will never be triggered.

[JaniM]

Is there any situation where the expect could be hit?

[Moxinilian]

I believe the specs does not guarantee it would never happen. It's not very expensive to make sure it doesn't go wrong.

[JaniM]

Is the only possible error case that the component has been added between the check and the insertion? In that case we could ignore the result entirely.

[Moxinilian]

No, this would not even be an error, insert overwrites the previous component.
@torkleyy What do you think?

[azriel91]

+1 to expect() instead of unwrap().

Is the only possible error case that the component has been added between the check and the insertion?
it's a mutable borrow of self, so nothing can insert it between

by the way, WriteStorage is a Storage, and has a contains method, so perhaps it expresses the intent better.

if !self.contains(entity) {
    self.insert(entity, Default::default()).expect("..");
}
self.get_mut(entity).expect("..")

The alternate implementation feels a bit verbose

if let Some(component) = self.get_mut(entity) {
    component
} else {
    self.insert(entity, Default::default()).expect("..");
    self.get_mut(entity).expect("..")
}

Same comment for the below implementation

[JaniM]

Switched to expect.

[azriel91]

yo, this is useful!

have some recommendations, but then realized there's also existing tech debt

[azriel91]

+1 to expect() instead of unwrap().

Is the only possible error case that the component has been added between the check and the insertion?
it's a mutable borrow of self, so nothing can insert it between

by the way, WriteStorage is a Storage, and has a contains method, so perhaps it expresses the intent better.

if !self.contains(entity) {
    self.insert(entity, Default::default()).expect("..");
}
self.get_mut(entity).expect("..")

The alternate implementation feels a bit verbose

if let Some(component) = self.get_mut(entity) {
    component
} else {
    self.insert(entity, Default::default()).expect("..");
    self.get_mut(entity).expect("..")
}

Same comment for the below implementation

[azriel91]

hm, what are these loops and magic numbers? am finding I have to think quite a bit to understand the logic, and then think again to figure out what its intent is.

if the loops are necessary, it's worth writing the rationale in a comment

• is it to get the entity index high enough?
• can we use a smaller backing storage to achieve the same effect?

not obvious to understand what the test is doing 🤔:

1. insert 500 entities with n + 1290 as its value.
2. mutate 1000 entities with get_mut_or_default() to be n + n + 710
3. assert the first 500 are 2n + 2000
4. assert the last 500 are n + 710

ah 💡! while it is logically correct and does test the effect, it's considerable mental effort to turn the code back into understanding. could you try to make it easier, otherwise my head is like this: 🤕, haha

---

edit: just expanded the diff, and saw the existing code is like that. oh man 😂

[JaniM]

I removed the magic numbers & commented it.

[JaniM]

Squashed & pushed with relevant modifications.

[azriel91]

🚀

[torkleyy]

Thanks, looks useful. I'm a little unsure how to handle dead entities here.

[torkleyy]

This will panic for dead entities.

[JaniM]

Oh boy. I didn't think of that.

[torkleyy]

Same here

[JaniM]

Marked that it will panic if the entity is dead. Another option is to return an Option, which'd make the usage slightly less ergonomic, but in most cases still nice enough.

[JaniM]

^ Opened a separate PR for that. Pick one.