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

breaking: Removed [SyncEvent] because ClientRpc/TargetRpc does the same thing, and it's an enormous effort to support the Weaver code. #2178

Merged
merged 1 commit into from
Sep 3, 2020

Conversation

miwarnec
Copy link
Collaborator

@miwarnec miwarnec commented Aug 21, 2020

Nobody uses SyncEvent.
Nobody knows how it really works (does it broadcast to everyone, to observers, or only to owner?)

It's a redundant feature. ClientRpc/TargetRpc can be used instead, and it's more obvious who it's sent to.

It also removes a large chunk of weaver magic.
832 LOC for a useless feature.

Let's get rid of the baggage so we can move on into the future :)

@miwarnec miwarnec marked this pull request as ready for review August 21, 2020 10:30
@sonarcloud
Copy link

sonarcloud bot commented Aug 21, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@James-Frowen
Copy link
Contributor

I agree with removing them, SyncEvents are a confusing feature because they allow you to register handlers on server but it never invokes them on the server.

We should try to add/update docs on alternatives to SyncEvents before we merge this

@miwarnec miwarnec marked this pull request as draft August 21, 2020 14:58
@miwarnec miwarnec changed the title breaking: Removed [SyncEvent] because ClientRpc/TargetRpc does the same thing, and it's an enormous effort to support the Weaver code. [September] breaking: Removed [SyncEvent] because ClientRpc/TargetRpc does the same thing, and it's an enormous effort to support the Weaver code. Aug 21, 2020
@miwarnec miwarnec mentioned this pull request Aug 21, 2020
@James-Frowen
Copy link
Contributor

I will update the docs with some of the info below later, but for now I am just dumping it here.

Extra context on removing SyncEvents

  • Critical bug like only being allowed 1 sync var per NetworkBehaviour existed in UNET and mirror for several years without being removed. This implies that not many people use sync events.
  • Adding handlers on the server like a normal c# event, but they will never be invoked on the server. This seems is error prune it would be hard to debug why your event is not invoking a handler.
  • There are not many examples or tests around SyncEvents there is no clear idea how they should work.

Instead of working out how SyncEvent should ideally work it is simple to remove them as the same behaviour can be achieved with ClientRpc and will lead to more predicable behaviour

Alternative to SyncEvent

public event OnHarm onHarm;
[ClientRpc]
private void RpcOnHarm(float damage, GameObject attacker)
{
    this.onHarm?.Invoke(damage, attacker);
}

[Server]
private void Harm(float damage, GameObject attacker)
{
    // harm stuff here

    // invoke on server
    this.onHarm?.Invoke(damage, attacker); 
    // invoke on client
    RpcOnHarm(damage, attacker);
}

This will let people add handlers to event on server or client, then decide which ones need to be invoked at different times.

For example, the server might want to listen to an even to keep track score, and the client might want to listen to even to show a visual effect.

@miwarnec miwarnec marked this pull request as ready for review September 3, 2020 06:49
…me thing, and it's an enormous effort to support the Weaver code.
@miwarnec miwarnec changed the title [September] breaking: Removed [SyncEvent] because ClientRpc/TargetRpc does the same thing, and it's an enormous effort to support the Weaver code. breaking: Removed [SyncEvent] because ClientRpc/TargetRpc does the same thing, and it's an enormous effort to support the Weaver code. Sep 3, 2020
@miwarnec miwarnec merged commit fc1096d into master Sep 3, 2020
@miwarnec miwarnec deleted the remove_syncevent branch September 3, 2020 07:04
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.

2 participants