Split custom R/W gen & register process

[pepoipod]

Mirror's custom Reader Writer currently has the following problem.

For example, suppose you have a case like this:
Assembly A → references Assembly B's NetworkMessage → references Assembly C's custom Reader Writer

When Assembly A is processed, the ReaderWriterProcessor processes the related assemblies in sequence. If Assembly B happens to be processed first, Assembly C's custom Reader Writer has not yet been registered at that point.

In this situation, where it should be referencing Assembly C's custom Reader Writer, it instead generates and registers a default Reader Writer inside Assembly B. Since Mirror does not permit registering a differently-named custom Reader Writer afterward, this causes a problem: in cases where an assembly is referenced multiple times, the default Reader Writer ends up winning.

To solve this problem, we separated the Reader Writer Processor into the following phases:

1. Registering custom Reader Writers into the cache
2. Generating the Message R/W

[SoftwareGuy]

What's the likelihood of this breaking projects? Does it stop and scream if something goes wrong?

[pepoipod]

The answer is a bit difficult, but this will occur with 100% certainty if there is such an assembly dependency. To avoid it, the user could control the registration order of the asmdefs, but that would be far too user-unfriendly.

Whether a crash — or rather, unintended behavior — occurs depends on the data structure. If a discrepancy arises between this default Reader Writer and the custom Reader Writer, you will definitely be disconnected from the server.

For example, in my project the classes are designed like this:

public struct TestMessage : NetworkMessage {
    public TestObject testObject;
}

public struct TestObject {
    public int A { get; set; }
}

public static class TestObjectReaderWriter {
    public static void WriteTestObject(this NetworkWriter writer, TestObject obj)
    {
        writer.Write(obj.A);
    }
     
    public static TestObject ReadTestObject(this NetworkReader reader)
    {
        return new()
        {
            A  = reader.Read<int>()
        };
    }
}

In Mirror, getters/setters are not automatically collected as properties. If your custom Reader Writer was configured to use the getter, a discrepancy arises with the default Reader/Writer, and you get disconnected from the server when the message is sent.

[SoftwareGuy]

Right, so you're saying that if the developer uses the default configuration then it shouldn't break, but if they implement their own then the discrepancy will arise and it's up to them to fix it?

[pepoipod]

You're exactly right. It is possible to work around this by imposing constraints such as not creating custom serializers, not crossing assembly boundaries, and not using getters/setters for the properties you want to use in classes targeted for synchronization — in other words, by having the developer stick to a default-style implementation.

However, getters/setters and asmdef are standard features of C#/Unity, and Mirror does provide a mechanism for custom serializers.

Since this touches on functionality at the very core of the Weaver, I understand there are certainly aspects that are difficult to judge, but if possible, I would be very grateful if you would consider merging this PR.

[MrGadget1024]

public struct TestObject {
    public int A { get; set; }
}

If you make that a field instead of a property (no get / set) does it still break?

[pepoipod]

Of course, this won't happen if you make it a field, because Mirror will collect it if it's a normal property.
However, this problem will occur if the user has defined their own CustomReader/Writer in an attempt to make this a field.

This isn't a question of whether fields should be enforced or not; it's a bug that can always occur when dealing with multiple assemblies related to custom Reader/Writer.

[MrGadget1024]

Of course, this won't happen if you make it a field

We only support fields in structs because the weaver generates properties with get / set, at least for SyncVars, so if making that struct just have public fields makes it work across assemblies as you expect, that's probably why and we can't change that.

[pepoipod]

This is clearly a bug in weaving, and it's a very unfortunate outcome as it will inconvenience users who use custom R/W, but I understand.