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.

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

Use single event system for client events #259

Closed
wants to merge 14 commits into from

Conversation

notmd
Copy link

@notmd notmd commented May 14, 2024

Fix #245

@notmd notmd marked this pull request as ready for review May 15, 2024 11:54
@notmd notmd changed the title Use single event system. Use single event system for client events May 15, 2024
src/lib.rs Outdated Show resolved Hide resolved
src/network_event/client_event.rs Outdated Show resolved Hide resolved
src/network_event/client_event.rs Outdated Show resolved Hide resolved
src/network_event/client_event.rs Outdated Show resolved Hide resolved
src/network_event/client_event.rs Outdated Show resolved Hide resolved
src/network_event/client_event.rs Outdated Show resolved Hide resolved
@Shatur
Copy link
Contributor

Shatur commented May 15, 2024

I think we probably should make the API nicer by using two functions. First function will read the resource from the registry by ID (like you tried to do before, like Bevy does for updates), iterates over all events and passes each event into a second function with the correct type. And let user to override only the second function.
The same for both sending and receiving.

I suspect that ClientEventFns will be the same as the upcoming ServerEventFns. So I would move it upper to the network_event module and rename into NetworkEventFns.

Also instead of passing the world, we probably should pass a context struct like we do for components. This context will contain the current tick and TypeRegistry.

@notmd
Copy link
Author

notmd commented May 16, 2024

I think we probably should make the API nicer by using two functions. First function will read the resource from the registry by ID (like you tried to do before, like Bevy does for updates), iterates over all events and passes each event into a second function with the correct type. And let user to override only the second function. The same for both sending and receiving.

I'm not sure I fully understand, but doing that we need to store the ComponentId (maybe a few times), which seems unnecessary for the internal API. I suppose app can have maximum few hundred events, does this matter?

I suspect that ClientEventFns will be the same as the upcoming ServerEventFns. So I would move it upper to the network_event module and rename into NetworkEventFns.

The ServerEventFns have an additional pop_from_queue fn, so this is not applicable.

Also instead of passing the world, we probably should pass a context struct like we do for components. This context will contain the current tick and TypeRegistry.

I'm not get this, can you elaborate more?

@Shatur
Copy link
Contributor

Shatur commented May 16, 2024

I'm not sure I fully understand, but doing that we need to store the ComponentId

I think that you can store it in the struct with functions. The idea behind it is to provide a nice API for users. Functions customization needed only to provide serialization/deserialization for events that doesn't have Serialize / Deserialize. For example, events with Box<dyn Reflect> inside.

The ServerEventFns have an additional pop_from_queue fn

Maybe we can unite pop_from_queue and receive? I think it will be possible with the mentioned two-functions approach.

I'm not get this, can you elaborate more?

Sure! Right now we pass &mut World into a single function. But let's consider separation into two functions for sending from server:

  1. Accepts &mut World, EventContext, current tick and channel ID. Obtains Events<T> from it, iterates over all, passes each T into second serialize function and sends the returned bytes over a channel ID to RepliconServer.
  2. Accepts T, EventContext, serializes, returns bytes.
    Similar for sending and client functions. Here EventContext is a struct with necessary context for serialization. I would store TypeRegistry and the current tick there.
    Take a look at functions for components API, it's similar. So the idea is to give user access only to necessary logic for serialization/deserialization instead of direct &mut World (maybe for sending we could use even only &World!). And instead of passing TypeRegistry and current tick directly, we also create a "context" struct with parameters. For consistency with components API and to avoid making breaking changes if we will need a new to pass more "context" data into functions.

@Shatur Shatur mentioned this pull request May 22, 2024
@Shatur
Copy link
Contributor

Shatur commented May 22, 2024

Superseded by #262.

@Shatur Shatur closed this May 22, 2024
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
2 participants