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

Fix for NetworkBehaviour.authority #3495

Closed
wants to merge 9 commits into from

Conversation

Pelax
Copy link

@Pelax Pelax commented Jun 2, 2023

Checking for both isClient and isServer to consider NetworkManagerMode.Host

Currently this flag is returning false if a NB is spawned without an ownerConnection (server-owned) in a host. It evaluates isClient as true and does the clients checks only. But if the NB syncs server-to-client, the host actually has authority.

@miwarnec
Copy link
Collaborator

miwarnec commented Jun 2, 2023

good find.
I'll add a unit test for this later before we merge.
unless you wanna :)

@Pelax
Copy link
Author

Pelax commented Jun 2, 2023

Yeah I could do that. Would 4 tests that check if it's true work? I'm thinking client only, server only, host as client, host as server.

Also, should it be included in Assets\Mirror\Tests\Editor\NetworkBehaviour\NetworkBehaviourTests.cs, or somewhere else?

@miwarnec
Copy link
Collaborator

miwarnec commented Jun 6, 2023

@Pelax any chance you could explain how to reproduce this with our tanks example?

@Pelax
Copy link
Author

Pelax commented Jun 7, 2023

@vis2k You can repro it by spawning any server-to-client object without ownerConnection while in host mode, and evaluating the authority.
For the tanks example, it can reproduced using the Projectile class, since they are spawned without an ownerConnection parameter. You could write something like this inside the Start method:

Debug.Log("syncDirection: " + syncDirection);
Debug.Log("authority: " + authority);

Then you start the host, and shoot, the console will indicate that the projectile is server-to-client and authority is false (but as we know authority is true)

@miwarnec
Copy link
Collaborator

thanks for explanation, gonna test in a few days!

@miwarnec
Copy link
Collaborator

miwarnec commented Jun 27, 2023

@miwarnec
Copy link
Collaborator

fixed slightly differently. nice find!
#3529

@miwarnec miwarnec closed this Jun 27, 2023
@Pelax
Copy link
Author

Pelax commented Jun 27, 2023

@vis2k looks like you did what @MrGadget1024 suggested a few weeks ago. I might be wrong, but as I mentioned in that conversation, I think this only reverses the problem. Wouldn't a client-owned with client-to-server sync direction entity return false in a host now?

@miwarnec miwarnec reopened this Jun 27, 2023
@miwarnec
Copy link
Collaborator

@vis2k looks like you did what @MrGadget1024 suggested a few weeks ago. I might be wrong, but as I mentioned in that conversation, I think this only reverses the problem. Wouldn't a client-owned with client-to-server sync direction entity return false in a host now?

I reopend the PR: #3531
gonna look into your suggestion asap, thanks

Copy link

@aletoivonen aletoivonen left a comment

Choose a reason for hiding this comment

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

I tested this and for me this works just as I expect it to work based on the documentation.

Copy link
Author

Choose a reason for hiding this comment

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

@vis2k I added these 8 tests with the variations that make sense to me, let me know what you think. If I run them in master branch, the one at line 174 fails (Host isOwned=false syncDirection=ServerToClient), basically the error that motivated this change suggestion.

@miwarnec
Copy link
Collaborator

sorry, totally forgot this.
fixed on master and I still use your tests

@miwarnec miwarnec closed this Oct 29, 2023
miwarnec pushed a commit that referenced this pull request Oct 29, 2023
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.

4 participants