Skip to content

Rework events #262

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

Merged
merged 7 commits into from
May 23, 2024
Merged

Rework events #262

merged 7 commits into from
May 23, 2024

Conversation

Shatur
Copy link
Contributor

@Shatur Shatur commented May 22, 2024

Superseded #259.

Events was implemented this way since the initial release of replicon (it was more then a year ago, time flies quickly!), but it have some flaws:

  • Public API for custom ser/de require user to define the whole sending and receiving functions which is not ergonomic. It's especially bad for server events where user need to use a few public helpers that we provided to reduce the boilerplate.
  • It creates a new set of systems for each replicon event. It's not very efficient and Bevy recently did a nice optimization which we also can do. It won't be that noticeable since user register much less amount of replicon events, but nice to have.

I think it's time to revisit the implementation and fix the mentioned flaws.

@Shatur Shatur requested a review from UkoeHB May 22, 2024 00:00
@Shatur Shatur linked an issue May 22, 2024 that may be closed by this pull request
Events was implemented this way since the initial release of replicon
(it was more then a year ago, time flies quickly!), but it have some
flaws:
- Public API for custom ser/de require user to define the whole
  sending and receiving functions which is not ergonomic. It's
  especially bad for server events where user need to use a few public
  helpers that we provided to reduce the boilerplate.
- It creates a new set of systems for each replicon event. It's not very
  efficient and Bevy recently did
  [a nice optimization](bevyengine/bevy#12936)
  which we also can do. It won't be that noticeable since user register
  much less amount of replicon events, but nice to have.

I think it's time to revisit the implementation and fix the mentioned
flaws.
Copy link

codecov bot commented May 22, 2024

Codecov Report

Attention: Patch coverage is 89.04429% with 47 lines in your changes are missing coverage. Please review.

Project coverage is 90.86%. Comparing base (fc189a7) to head (868fca4).
Report is 1 commits behind head on master.

Files Patch % Lines
src/server_event.rs 87.65% 30 Missing ⚠️
src/client_event.rs 90.11% 17 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #262      +/-   ##
==========================================
+ Coverage   90.79%   90.86%   +0.06%     
==========================================
  Files          37       36       -1     
  Lines        2076     2310     +234     
==========================================
+ Hits         1885     2099     +214     
- Misses        191      211      +20     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

@notmd notmd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

look good.

/// Type-erased functions and metadata for a registered client event.
///
/// Needed to process all events in a single system and call its functions without knowing the type.
struct ClientEventData {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it worth to move this and server structs into their own modules.
I will do it after the review in this PR since I marked it as ready for review and don't want to disrupt the process.

Copy link
Collaborator

@UkoeHB UkoeHB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation breaks my use-case, which injects custom send/receive systems. I don't think there is a way to continue supporting my use-case (which is focused on exposing a single synchronized event stream), so I will just reimplement the missing pieces in my project.

/// # Safety
///
/// The caller must ensure that `events` is [`Events<E>`] and `server_events` is [`Events<ToClients<E>>`].
unsafe fn resend_locally<E: Event>(events: PtrMut, client_events: PtrMut) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
unsafe fn resend_locally<E: Event>(events: PtrMut, client_events: PtrMut) {
unsafe fn resend_locally<E: Event>(client_events: PtrMut, events: PtrMut) {

Match the order of use.

@Shatur
Copy link
Contributor Author

Shatur commented May 23, 2024

Thanks!
Ah, I forgot that you have a special use case... But yes, I think it would be better to implement this on your project side.

@Shatur Shatur requested a review from UkoeHB May 23, 2024 09:07
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 89.04429% with 47 lines in your changes are missing coverage. Please review.

Project coverage is 90.86%. Comparing base (3146286) to head (868fca4).

Current head 868fca4 differs from pull request most recent head 9f77642

Please upload reports for the commit 9f77642 to get more accurate results.

Files Patch % Lines
src/server_event.rs 87.65% 30 Missing ⚠️
src/client_event.rs 90.11% 17 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #262      +/-   ##
==========================================
+ Coverage   90.83%   90.86%   +0.03%     
==========================================
  Files          37       36       -1     
  Lines        2084     2310     +226     
==========================================
+ Hits         1893     2099     +206     
- Misses        191      211      +20     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Shatur Shatur merged commit 7a2f98e into master May 23, 2024
4 checks passed
@Shatur Shatur deleted the events-rework branch May 23, 2024 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use a single system for events
4 participants