Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Automatic event tracking #1776

Closed
wants to merge 10 commits into from
Closed

Automatic event tracking #1776

wants to merge 10 commits into from

Conversation

jihiggins
Copy link
Contributor

@jihiggins jihiggins commented Mar 27, 2021 •
edited
Loading

An implementation of the Automatic Event Tracking RFC

Works, but if Commands are fixed to work correctly when embedded in a SystemParam, it would probably be better to remove the RwLocks in favor of using that. (Should be an easy change.)

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events help wanted C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Mar 27, 2021
@alice-i-cecile
Copy link
Member

Ooh, our first proto-RFC! For those digging into it, this work relates closely to that done for reliable change detection (#68, #1471).

@jihiggins jihiggins changed the title Rough draft for automatic event subscriptions Automatic event subscriptions Mar 28, 2021
@jihiggins
Changed EventReader to only have read access to EventSubscriptions
a481fcd 
* SubscriberId SystemParam added to handle initializing the id correctly when system state is initialized, instead of on first use of an EventReader

* Changed to use a command to update the subscriber last read counts to avoid write access to EventSubscriptions
@alice-i-cecile
Copy link
Member

We still want to add the defer_event functionality to this right?

@jihiggins
Copy link
Contributor Author

jihiggins commented Mar 28, 2021 •
edited
Loading

Some ideas from discord:

WrongShoe:
could you reuse the counter used for change detection to help with tracking events?

alice:
For when your system receives an event and isn't ready to handle it yet: you want a way to put it aside without blocking all of the other later events that did get handled properly
It feels like you'd cache a clone of the extra events in the Local state, and then always check both?

(EventReader::defer_event?)

edit: Not sure change detection would be very clean, and the indirection it'd probably require would be slightly less performant

@JeanMertz
Copy link
Contributor

Two drawbacks/questions that seem worth discussing/noting:

  1. Is there a performance implication to this change? Should both the old and new way of doing things be available if the new one is safer in the defined use-cases, but comes at a performance penalty?
  2. What happens to the buffer if I have a reader that never reads (user error) or reads extremely infrequently?

@jihiggins
Copy link
Contributor Author

jihiggins commented Mar 28, 2021 •
edited
Loading

The current setup with the RwLock will cause a decrease in performance. It also currently uses a write lock with each EventReader usage. I think both of those issues can be fixed, though.

For infrequent reads, it'd empty the events less often. It'd be relatively easy to fix, though (eg the user could just split up the events by how often theyre used.) We could add a debug assert / warning for events that have been held onto for more than x frames, if it's something we're worried about.

@jihiggins
Remove UpdateSubscriberCount command
ede6153
@jihiggins
Merge EventSubscriptions into Events
fb20d4e
@jihiggins
ManualEventReaders are now obtained from the Events
b69b64a 
* Each ManualEventReader tracks its subscription through the use of a string id

* These will lock resources the same way that EventReaders do

* If a ManualEventReader is made after events have been read by other readers, they will not see those events.
@jihiggins
Change ManualEventReader to match EventReader implementation
401edd5 
* EventReader can now just use ManualEventReader for its own implementation
@jihiggins jihiggins changed the title Automatic event subscriptions Automatic event tracking Mar 28, 2021
@alice-i-cecile
Copy link
Member

This is compelling, but deserves a cohesive design review. Once #1662 is concluded, we should open an RFC for more reliable events.

@jihiggins jihiggins mentioned this pull request Apr 23, 2021
Open
@alice-i-cecile alice-i-cecile mentioned this pull request May 17, 2021
Closed
@cart cart added the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 23, 2021
@DJMcNab DJMcNab removed the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Aug 7, 2021
@alice-i-cecile alice-i-cecile added S-Needs-Design-Doc This issue or PR is particularly complex, and needs an approved design doc before it can be merged and removed E-Help-Wanted labels Sep 22, 2021
@mockersf
Copy link
Member

closing this PR for now as it has drifted, please reopen once the RFC is getting ready

@mockersf mockersf closed this Dec 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-Design-Doc This issue or PR is particularly complex, and needs an approved design doc before it can be merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jihiggins @alice-i-cecile @JeanMertz @mockersf @cart @DJMcNab