AppExit should be an observer event rather than a BufferedEvent #20408 #20442
Conversation
…ngine#20408 # Objective Performance: - Replace logic that run every frame, by using the observer pattern. - Less event buffering so smaller memory footprint at runtime. ## Solution - A new Resource is added which stores the first error event seen. - Now using command.trigger to generate Events. - Examples updated. This is low level plumbing, no many public interfaces have been adjusted. ## Testing - Existing tests have been modified, No additional tests.
|
Welcome, new contributor! Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨ |
|
In terms of reivew, should I be using a Resource to store the first error observed? I will rapidly reponse with changes to PR is there is a more natural alternative |
|
I don't have time to review / debate this now, but this needs careful consideration as it has implications for large-scale patterns in Bevy's code. The other reason I think this is controversial is that the lack of observer ordering is felt particularly acutely with AppExit, and needs to be carefully considered. |
Better documentation Co-authored-by: Kristoffer Søholm <k.soeholm@gmail.com>
As suggested in a review by kristoff3r Co-authored-by: <k.soeholm@gmail.com>
- app.add_observer(despawn_windows) + app.add_observer(on_app_exit) Co-authored-by: <k.soeholm@gmail.com>
|
I still need to debug this... I am new to the project, so I fully expect my reasoning needs correcting We are swapping A for B where A ) Buffered Events for where the order of events is preseved for B) Commands pushed onto the command queue, which is determinsitc. The function in question is to latch the first error observed. Is there a race condiction here .. where Resources are maybe delayed in their update such that a stale resource is read twice leading to the second or third error being latched? As I say I am not telling, I am learning? |
…stry is no longer created.
| .insert_resource(DisplayHandleWrapper(event_loop.owned_display_handle())) | ||
| .insert_resource(DisplayHandleWrapper(event_loop.owned_display_handle())); | ||
|
|
||
| app.add_observer(on_app_exit) | ||
| .add_event::<RawWinitWindowEvent>() |
There was a problem hiding this comment.
Why split up this chain? Also .add_event::<RawWinitWindowEvent>() belongs close to .insert_resource(DisplayHandleWrapper(event_loop.owned_display_handle())), they serve a complimentary purpose.
| .cloned() | ||
| .unwrap_or(AppExit::Success), | ||
| ); | ||
| let stored_event = &self.world().get_resource::<AppExitRes>()?.state; |
There was a problem hiding this comment.
I think something is going wrong here, if I understand correctly you will only return a Some if any non AppExit::Success has been triggered (as a result of register_app_exit_error). However in the old code, a Some(AppExit::Success would still be emitted if there is at least 1 event and none of them are errors.
In other words, I believe your version would simply not exit upon AppExit::Success
There was a problem hiding this comment.
Thanks for your first contribution! I took a cursory look, I am unfortunately not able to test this at the moment.
I do have to agree with @alice-i-cecile that this change is somewhat controversial in the sense that it might have unforeseen interactions with existing third party plugins.
The original motivation of the issue was that
Multiple AppExit events in a buffer doesn't make sense
Which I think the runner_returns_correct_exit_code clearly shows otherwise: when their are multiple events, we want the first non-graceful exit to be used.
As well as:
This would be more performant for plugins to react to app exits, rather than having to schedule a system that runs every frame.
Which I would consider a very premature optimization, I highly doubt that the difference is measurable in any context.
This makes me believe that this change, while arguably sensible, is not entirely worth the risk. I'm interested to hear your opinions :)
Objective
Performance:
AppExitshould be an observer event rather than aBufferedEvent#20408Solution
This is low level plumbing, no many public interfaces have been adjusted.
Testing