Update init_resource to not overwrite #1349
Conversation
|
I like this change, although it would be nice to change See #1186 for more discussion on this topic. Edit: I've made my own PR to solve this: #1352. |
| @@ -235,8 +235,10 @@ impl AppBuilder { | |||
| where | |||
| R: FromResources + Send + Sync + 'static, | |||
| { | |||
| let resource = R::from_resources(&self.app.resources); | |||
| self.app.resources.insert(resource); | |||
| if !self.resources().contains::<R>() { | |||
There was a problem hiding this comment.
This does "double hash" the type id. Resources don't currently have an "entry" api. And I don't think adding one is in scope for this pr. But can we add a // PERF: remove this double hash comment so it is discoverable?
There was a problem hiding this comment.
That would be difficult in this case, because we need to both be modifying self.resources() for the Entry, and reading it for the from_resources call - so without some gnarly unsafe code, I'm not sure this is feasible.
It's also worth noting that this is using the bevy_utils::HashMap, so the hashing is rather cheap.
Also this only happens at app startup time, so the cost doesn't matter too much. I've added a comment explaining this though
There was a problem hiding this comment.
makes sense. i added a "PERF" label here. I use those like TODO labels, but for places where we are knowingly leaving perf on the table.
|
Well in that case, I think we'll all ready to merge. Message to future readers: Don't try and solve that perf thing, you're in for a world of unsafe pain. |
# Objective - The perf comments, added (by me) in bevyengine#1349, became outdated once the initialisation call started to take an exclusive reference, (presumably in bevyengine#1525). - They have been naïvely transferred along ever since ## Solution - Remove them
# Objective - The perf comments, added (by me) in bevyengine#1349, became outdated once the initialisation call started to take an exclusive reference, (presumably in bevyengine#1525). - They have been naïvely transferred along ever since ## Solution - Remove them
The name init does not suggest it would overwrite an already created version.
We discussed this on discord and @cart has at one point been on board.