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

Constructors of custom class/struct do not use syncvar getter/setter for GO/NI/NB. #3797

Open
James-Frowen opened this issue Mar 28, 2024 · 0 comments
Assignees
Labels
bug Something isn't working Needs Research Weaver Bug caused by Weaver

Comments

@James-Frowen
Copy link
Contributor

Describe the bug
Normally when using a GO/NI/NB syncvar it will check the netid field first and get the object from the spawned dictionary, it will then set the real field and return the value. There seems to be a bug where if you use that field inside a class/struct before it has been set by either OnDeserialize or another method using it then it will stay null

This seems like a minor issue in Mirror since it will use the real field and not cause any errors. It will mostly cause timing issues where the GO/NI/NB field will stay null for longer than it needs to

[IMPORTANT] How can we reproduce the issue, step by step:
something like this should replicate it:

  • NB1 has field targeting NI2
  • Spawn NI1 and NI2 on server
  • Send NI1 to client
    • NB1 will store netid of NI2, but field will be null
  • Send NI2 to client
  • expected: using NB1 field will get the NI2 object from spawned dictionary
  • bug: using NB1 field will stay null when called inside class/struct constructor

Additional context
This should be the code in Mirror:

static void ProcessSetInstruction(SyncVarAccessLists syncVarAccessLists, MethodDefinition md, Instruction i, FieldDefinition opField)
{
// don't replace property call sites in constructors
if (md.Name == ".ctor")
return;
// does it set a field that we replaced?
if (syncVarAccessLists.replacementSetterProperties.TryGetValue(opField, out MethodDefinition replacement))
{
//replace with property
//Log.Warning($" replacing {md.Name}:{i}", opField);
i.OpCode = OpCodes.Call;
i.Operand = replacement;
//Log.Warning($" replaced {md.Name}:{i}", opField);
}
}
// replaces syncvar read access with the NetworkXYZ.get property calls
static void ProcessGetInstruction(SyncVarAccessLists syncVarAccessLists, MethodDefinition md, Instruction i, FieldDefinition opField)
{
// don't replace property call sites in constructors
if (md.Name == ".ctor")
return;
// does it set a field that we replaced?
if (syncVarAccessLists.replacementGetterProperties.TryGetValue(opField, out MethodDefinition replacement))
{
//replace with property
//Log.Warning($" replacing {md.Name}:{i}");
i.OpCode = OpCodes.Call;
i.Operand = replacement;
//Log.Warning($" replaced {md.Name}:{i}");
}
}

Tracking the code back, it seems like the ctor check is from UNET with no explanation.

After some testing it seems that setting default values for syncvar will cause errors when using the setter in a NetworkBehaviour constructor. So it seems like the check if only there to avoid it for NetworkBehaviour (and maybe other unity types).

This fix would be something like this, where you only skip the constructor for unity types:
MirageNet/Mirage@2f5db3d

@MrGadget1024 MrGadget1024 added Weaver Bug caused by Weaver Needs Research bug Something isn't working labels Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Needs Research Weaver Bug caused by Weaver
Projects
None yet
Development

No branches or pull requests

4 participants