search

FirstGearGames / FishNet

FishNet: Unity Networking Evolved.
Other
1.39k stars 150 forks source link

SyncVar .Dirty() does not invoke OnChange for caller #674

Closed Jazz23 closed 6 months ago

Jazz23 commented 6 months ago

General Unity version: 2022.3.23f1 Fish-Networking version: Commit 9533cae73a61fd5924602d5011eb1bb8744fa643 (I just pulled from github, but this happens with any version) Discord link: https://discord.com/channels/424284635074134018/1034477094731784302/1241098646146121819

Description Calling .Dirty() on a SyncVar<T> does not invoke the OnChange event for whoever invoked Dirty (server or client).

Steps to reproduce the behavior: Just subscribe to OnChange (client or server), call .Dirty() on a normal SyncVar, and watch it not invoke your method. I made a quick demo video (including the reason why fishnet has this bug in the first place): https://www.youtube.com/watch?v=VkmgAGn86gw Test script used in video: https://gist.github.com/Jazz23/69f962045a5c9cbf72d6ba4e58d43d35

Expected behavior The OnChange event gets invoked after calling .Dirty(). FishNet does this for every other sync type.

Notes: I would have done a PR but fixing this requires adding a new function to SyncVar since it's .Dirty() is directly from SyncBase (and it's not a virtual method). I didn't wanna mess around with something like public new void Dirty()...

Thank you!

FirstGearGames commented 6 months ago

I confirmed this issue. Calling Dirty might work for SyncVars because there is only one value, so we know what to dirty. But for something like a SyncList there are already ways to dirty specific items, and calling Dirty would have to dirty the entire collection to conform API.

Plus the behavior of it as of right now is to basically indicate the SyncType has something dirty, rather than specify what is dirtied as just mentioned. So I suppose what I am saying is that it shouldn't even be public to begin with, but protected.

The inheriting class should be the one calling it, and dirty said synctype another way. I see collection types have DirtyAll() so perhaps I'll implement that in syncvar.

FirstGearGames commented 6 months ago

Resolved in 4.3.2. You will be using DirtyAll. I also found a couple other callback bugs in relation.

Jazz23 commented 6 months ago

FGG the goat!