Render events - #18517
Render events#18517ecoskey wants to merge 8 commits into
Conversation
|
The generated |
ecoskey
added
A-Rendering
|
Quickly leaving my thoughts, I don't love the events naming. It has nothing to do with events about rendering, but is just an arbitrary channel for sending data back to the main world, right? Not sure what a better name is though, and again haven't looked at the PR to deeply yet. |
|
They are events from the render world. I think it's fair to call them render event. Presumably, users will need this to send events in the context of rendering. Otherwise they wouldn't need events sent from the render world. Maybe specify render world event just to remove any possible ambiguity, but I'm not sure what kind of ambiguity there could be. |
| let send_render_event = ( | ||
| ParamBuilder, | ||
| ParamBuilder, | ||
| ParamBuilder, | ||
| LocalBuilder(Timer::from_seconds(2.0, TimerMode::Repeating)), | ||
| ) | ||
| .build_state(render_app.world_mut()) | ||
| .build_system(send_render_event); |
There was a problem hiding this comment.
This feels a bit unnecessarily complex. I'd suggest using a resource instead just because that's a more common ECS api.
tychedelia
left a comment
tychedelia
left a comment
There was a problem hiding this comment.
Think formalizing this makes sense versus ad hoc. 👍
|
|
||
| use crate::RenderApp; | ||
|
|
||
| pub trait RenderEventApp { |
There was a problem hiding this comment.
Not sure what our convention is here -- should this be in the prelude so it doesn't have to be imported?
| /// | ||
| /// Internally, this struct writes to a channel, the contents of which are relayed | ||
| /// to the main world's [`Events`] stream during [`PreUpdate`]. Note that because | ||
| /// the render world is pipelined, the events may not arrive before the next frame begins. |
There was a problem hiding this comment.
They almost certainly will not, but they also could. I think we might want to be clearer about how this works. In practice, you'll receive events from rendering frame N-2 with respect the the main world, but that isn't guaranteed. Users should supply their own temporal correlation id if necessary.
| @@ -0,0 +1,90 @@ | |||
| //! Simple example demonstrating the use of [`App::init_render_event`] to send events from the | |||
There was a problem hiding this comment.
This should be expanded to say, in effect, "you probably don't want to do this."
tychedelia
added
S-Ready-For-Final-Review
alice-i-cecile
left a comment
alice-i-cecile
left a comment
There was a problem hiding this comment.
Broadly on-board with this pattern and it existing, but I think we need to clean this up before merging.
Key things:
- the system builder in the example is too complicated and distracts from the point of the example
- the example description needs to be expanded to give more context
- this needs a non "Event" naming scheming to avoid confusion. Maybe something with "channel" in it?
Long-term, this should be baked into a multi-world ECS design, and not need channels, but formalizing this pattern in some form is a good way to gather requirements and ease refactoring.
ecoskey
added
S-Waiting-on-Author
|
Cut from the milestone: this would be nice to have but shouldn't block Bevy 0.17's release. |
|
closing, stale. |
5 participants
Objective
fix #18491 (comment)
Sometimes users want to send events from the render world to the main world. While not often advised, some advanced use-cases like readback use a similar pattern, which often requires setting up a channel and relaying events manually.
Solution
Add a simple abstraction (
App::init_render_event) to set up the channel and relay events to the main world, retaining caller information.Note: this pr makes
Events::send_with_callerpublic, but does not add a similar public method onEventWriterTesting
Ran example