Custom state#33
Conversation
CorvusPrudens
left a comment
CorvusPrudens
left a comment
There was a problem hiding this comment.
I thought I'd make a couple comments on the API, but they're just some small suggestions.
I've integrated this PR in bevy_seedling. Overall, I'm maybe the slightest touch worried that it's a little cumbersome to get the state, but I'm very happy that this encourages nodes to be POD. That takes the APIs I've written for the ECS from mostly correct to necessarily correct.
There are certainly ways to make sharing a node's state easy with this API if someone needs that, so I'd be content if it were merged as-is.
| channel_config: ChannelConfig, | ||
| uses_events: bool, | ||
| call_update_method: bool, | ||
| custom_state: Option<Box<dyn Any>>, |
There was a problem hiding this comment.
This is super nitpicky, so take it for what it's worth, but could we possibly separate out the state from the info struct?
The reason I ask is that technically, a user could provide the state in both AudioNode::info and AudioNode::processor, although it seems like it's supposed to be constructed in the latter (correct me if I'm wrong).
There was a problem hiding this comment.
The same Option<Box<dyn Any>> object that is created in the AudioNodeInfo struct is what is passed to AudioNode::processor and AudioNode::update. The node is free to mutate these at any point, but generally nodes should construct it in AudioNodeInfo so that the state is immediately available to the user in FirewheelCtx::node_state.
|
Looks good to me -- my examples work, all tests pass, and I like the constructor argument consolidation. |
2 participants
No description provided.