Skip to content

AppExit should be an observer event rather than a BufferedEvent #20408 #20442

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

martinfrances107
Copy link

@martinfrances107 martinfrances107 commented Aug 6, 2025

Objective

Performance:

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.

…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.
Copy link
Contributor

github-actions bot commented Aug 6, 2025

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@martinfrances107
Copy link
Author

martinfrances107 commented Aug 6, 2025

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

@alice-i-cecile alice-i-cecile added the A-Windowing Platform-agnostic interface layer to run your app in label Aug 6, 2025
@alice-i-cecile alice-i-cecile requested a review from ItsDoot August 6, 2025 17:34
@alice-i-cecile alice-i-cecile added A-App Bevy apps and plugins X-Controversial There is active debate or serious implications around merging this PR S-Needs-Review Needs reviewer attention (from anyone!) to move forward S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Aug 6, 2025
@alice-i-cecile
Copy link
Member

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.

martinfrances107 and others added 4 commits August 6, 2025 18:48
Better documentation

Co-authored-by: Kristoffer Søholm <[email protected]>
As suggested in a review by kristoff3r

Co-authored-by: <[email protected]>
- app.add_observer(despawn_windows)
+ app.add_observer(on_app_exit)

Co-authored-by: <[email protected]>
@martinfrances107
Copy link
Author

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.
( At least command 1 is executed before command 2 etc. )

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-App Bevy apps and plugins A-Windowing Platform-agnostic interface layer to run your app in S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AppExit should be an observer event rather than a BufferedEvent
3 participants