[Merged by Bors] - keep track of type name in NodeState #1444
Conversation
alice-i-cecile
added
C-Code-Quality
| @@ -131,6 +132,7 @@ impl NodeState { | |||
| input_slots: ResourceSlots::from(node.input()), | |||
| output_slots: ResourceSlots::from(node.output()), | |||
| node: Box::new(node), | |||
| type_name: Some(std::any::type_name::<T>()), | |||
There was a problem hiding this comment.
Does it always have a value? Is the Option needed?
There was a problem hiding this comment.
In bevy all uses of NodeState supply the type name, so the option isn’t needed. However I wasn’t sure whether there are use cases where the type name is not available, and the size of Option<&str> is the same as &str anyway.
I don’t really care which one it is.
There was a problem hiding this comment.
its less about size and more about adding another branch to unwrap the option (which has performance and code complexity implications). I think I'd rather opt for non-options first, then add them if/when we hit a case where they're needed
There was a problem hiding this comment.
I changed it now.
alice-i-cecile
added
the
S-Ready-For-Final-Review
jakobhellermann
force-pushed
the
node-state-type-name
branch
from
d6183f3
to
5f22ea7
Compare
|
bors r+ |
Adds the original type_name to `NodeState`, enabling plugins like [this](https://github.com/jakobhellermann/bevy_mod_debugdump). This does increase the `NodeState` type by 16 bytes, but it is already 176 so it's not that big of an increase.
|
Pull request successfully merged into main. Build succeeded: |
bors
bot
changed the title
Adds the original type_name to
NodeState, enabling plugins like this.This does increase the
NodeStatetype by 16 bytes, but it is already 176 so it's not that big of an increase.