Skip to content

feat: Instantiation payload support for INetworkPrefabInstanceHandler #3430

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

Open
wants to merge 51 commits into
base: develop-2.0.0
Choose a base branch
from

Conversation

Extrys
Copy link

@Extrys Extrys commented Apr 27, 2025

Solves Issue #3421

Related to the discussions in Pull Request #3419 and Issue #3421 (follow-up proposal based on new approach).

This PR introduces support for sending custom instantiation payloads through INetworkPrefabInstanceHandlerWithData to receive metadata before calling Instantiate().

The feature is fully optional, backward-compatible, and requires no changes to existing user workflows.

Changelog

  • Added INetworkPrefabInstanceHandlerWithData, a variant of INetworkPrefabInstanceHandler that supports synchronizing custom data prior to the Instantiate() method call.

Testing and Documentation

  • New tests have been added.
  • Inline code documentation provided; INetworkPrefabInstanceHandler.Instantiate() summary updated to mention INetworkPrefabInstanceHandlerWithData
  • Further external documentation is recommended for INetworkPrefabInstanceHandlerWithData.

Deprecated API

  • None.

Backport

  • Can be backported to v1.x. with a cherry pick if merged

Implementation Example

public class MyPrefabInstanceHandler : INetworkPrefabInstanceHandlerWithData
{
    GameObject[] prefabs;
    public int customSpawnID = 0;

	//NEW
    void INetworkPrefabInstanceHandlerWithData.OnSynchronizeInstantiationData<T>(ref BufferSerializer<T> serializer)
        => serializer.SerializeValue(ref customSpawnID);

    public NetworkObject Instantiate(ulong clientId, Vector3 position, Quaternion rotation)
        => Object.Instantiate(prefabs[customSpawnID], position, rotation).GetComponent<NetworkObject>();

    public void Destroy(NetworkObject networkObject)
        => Object.Destroy(networkObject.gameObject);
}

Spawning flow:

MyPrefabInstanceHandler prefabInstanceHandler = new MyPrefabInstanceHandler();
GameObject basePrefab;

void RegisterHandler() => NetworkManager.Singleton.PrefabHandler.AddHandler(basePrefab, prefabInstanceHandler);
void UnregisterHandler() => NetworkManager.Singleton.PrefabHandler.RemoveHandler(basePrefab);

public void Spawn(int id)
{
    prefabInstanceHandler.customSpawnID = id; //Plot twist: simply modify the handler's data
    NetworkManager.Singleton.SpawnManager.InstantiateAndSpawn(basePrefab.GetComponent<NetworkObject>());
}

Important

When spawning, you must update the handler's data before calling Spawn() or InstantiateAndSpawn().
The data set in the handler will be serialized automatically prior the instantiation process.

Highlights

  • Optional and non-breaking
  • Intuitive to configure and resilient to errors
  • Fully aligns with NGO patterns (Serialize/Deserialize symmetry)
  • Late-join and scene object support
  • No public API modifications

@Extrys Extrys requested a review from a team as a code owner April 27, 2025 02:26
@Extrys Extrys changed the title feat: Network Object Instantiation Payload feat: Instantiation payload support for INetworkPrefabInstanceHandler Apr 27, 2025
@Extrys
Copy link
Author

Extrys commented Apr 27, 2025

I just posted a video demonstrating how the system works:

  • * is a symbol set on objects spawned through PrefabHandlers.
  • & is a symbol added to indicate the deterministic ID of an object.

In the video, I spawn A1 and A2, which are instances of the same prefab, displaying the letter A.
Each button spawns the prefab with a different number, and this number is sent via the instantiation payload.
No RPCs or NetworkVariables are needed.

The B object is registered with a regular PrefabHandler (no custom interface implemented here, just for basic testing).

D, E, and F are instantiated directly via Unity's regular Instantiate method.
Each of these sets its deterministic ID manually and registers itself into a local dictionary, indexed by that ID.

2025-04-27.21-45-44.mp4

By implementing a custom INetworkPrefabInstanceHandler together with INetworkInstantiationPayloadSynchronizer,
I simply retrieve the ID from the payload and use it to link the correct instance deterministically.

Here is the core implementation:

public class TestHandlerDeterministicLink : INetworkPrefabInstanceHandler, INetworkInstantiationPayloadSynchronizer
{
	public Dictionary<int, DeterministicIDHolder> deterministicSpawns = new Dictionary<int, DeterministicIDHolder>();

	public int customSpawnID = 0;

	void INetworkInstantiationPayloadSynchronizer.OnSynchronize<T>(ref BufferSerializer<T> serializer) => serializer.SerializeValue(ref customSpawnID);

	public NetworkObject Instantiate(ulong clientId, Vector3 position, Quaternion rotation)
	{
		var obj = deterministicSpawns[customSpawnID];
		TMP_Text text = obj.GetComponent<TMP_Text>();
		text.SetText(text.text + "*");
		return obj.GetComponent<NetworkObject>();
	}

	public void Destroy(NetworkObject networkObject) => GameObject.Destroy(networkObject.gameObject);

	int nextDeterministicId = 0;

	public void InstantiateLocally(GameObject linkablePrefab)
	{
		var spawned = GameObject.Instantiate(linkablePrefab);
		spawned.transform.position = UnityEngine.Random.insideUnitCircle * 0.01f;
		var text = spawned.GetComponent<TMP_Text>();
		text.SetText(nextDeterministicId.ToString() + "&" + text.text);
		var deterministicIdHolder = spawned.GetComponent<DeterministicIDHolder>();
		deterministicSpawns[nextDeterministicId] = deterministicIdHolder;
		deterministicIdHolder.SetID(nextDeterministicId);
		nextDeterministicId++;
	}
}

Warning

While this system enables advanced workflows,
it is important to note that developers are responsible for ensuring that the linked instances are compatible.
This flexibility is intentional to support a variety of custom deterministic linking strategies.

@victorLanga17
Copy link

This would actually save us a lot of trouble.

Right now when after we spawn stuff we have to run like two or three RPCs just to finish setting up objects properly.
If we could send a bit of info during the spawn itself I think we could solve a couple of problems easier.

I wish you luck on get it merged on Unity 6.1, it would be super useful for our current project.

@Extrys
Copy link
Author

Extrys commented Apr 28, 2025

This would actually save us a lot of trouble.

Right now when after we spawn stuff we have to run like two or three RPCs just to finish setting up objects properly. If we could send a bit of info during the spawn itself I think we could solve a couple of problems easier.

I wish you luck on get it merged on Unity 6.1, it would be super useful for our current project.

Sure! I'm just waiting for reviewers to be assigned to this PR.
In the worst case, you can always use my fork, which I will keep updated for my use cases only, so sometimes it might not be fully up to date.

@Extrys Extrys marked this pull request as draft April 29, 2025 04:51
@Extrys
Copy link
Author

Extrys commented Apr 29, 2025

I just got more feedback on the Issue #3421

I'm thinking about a way to have the prefab handler "write" the payload right before the spawn happens.
The idea is that this write just stores the data locally in the instance that directly calls CreateObjectMessage, and then the actual message would consume that cached external variable right before sending.

In this approach, I would try to move most of the logic into CreateObjectMessage, removing it from the object data.
Although I feel there would still need to be a way to link the payload to the generated instances to make things work correctly for late joiners and similar cases.

This would avoid all the newly added generics and any potential object boxing.

I'm converting this PR into a draft to keep modifying the implementation and will get back to comment once it's ready for feedback again.

@Extrys
Copy link
Author

Extrys commented Apr 29, 2025

[Sorry bad writting i might edit this text later]

I did requested changes by @EmandM into the PR, currently im reusing the same buffer serializer from the object serialization.
Now the changeset is much smaller and easier to check and review, i would like you to give me feedback on that

Also i could make the new interface to not have the OnSynchronize, and having instead Serialize and Deserialize methods, but that would make the usage not as comfortable.

Copy link
Collaborator

@EmandM EmandM left a comment

Choose a reason for hiding this comment

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

This is a fantastic next step! The video was super helpful to understand what the intention was. Is there any chance you have a more minimalistic example of this feature that doesn't require linking the items together later. The added complexity of having separate objects that are linked implies that this feature is doing the linking. My understanding is simply that this feature enables changing the object that is instantiated as part of the instantiation flow.

A few notes on the code:

Out of interest, is there a reason you chose to implement the custom data passing only on scene objects? We'd prefer a solution that works everywhere where the prefab handler can work. Again, the symmetry in the approach is important. If you can do something with prefab handlers in one area, that feature should work in all areas.

It would also be fantastic if you can add a test showing how this feature is used.

@Extrys
Copy link
Author

Extrys commented Apr 29, 2025

This is a fantastic next step! The video was super helpful to understand what the intention was. Is there any chance you have a more minimalistic example of this feature that doesn't require linking the items together later. The added complexity of having separate objects that are linked implies that this feature is doing the linking. My understanding is simply that this feature enables changing the object that is instantiated as part of the instantiation flow.

A few notes on the code:

Out of interest, is there a reason you chose to implement the custom data passing only on scene objects? We'd prefer a solution that works everywhere where the prefab handler can work. Again, the symmetry in the approach is important. If you can do something with prefab handlers in one area, that feature should work in all areas.

It would also be fantastic if you can add a test showing how this feature is used.

Regarding this, I've tested it, and in the video only one object is an in-scene placed object. The rest are dynamically instantiated through the prefab instance handler, not just scene objects. Maybe I misunderstood what you meant?

I’ll work on a simpler example, though to be honest, the linking case is the most valuable use case I’ve found so far, its actually what motivated this feature in the first place.

Right now I dont have many alternative examples because most of the benefit comes from enabling that exact flow: having objects pre-created and deterministically selected or connected based on payload metadata. Of course, it also opens the door to more advanced use cases, like sending special setup/configuration data before instantiation (For example in the video that sets up A to be configured with a text that says the id to the next), but those are things I imagine users will find creative uses for once the mechanism is available.

In a way, I don’t yet fully know the limits of the feature, I just know it unlocks workflows that weren’t previously possible.

About the other changes, I will answer these and also come with some changes you might like tomorrow.

@Extrys
Copy link
Author

Extrys commented Apr 30, 2025

Although the struct is named SceneObject, it’s already used across the codebase to represent any instance of NetworkObject, not just those previously placed in the scene.
Even dynamically instantiated objects are serialized using this structure.
So this change applies universally to all uses of INetworkPrefabInstanceHandler, not only in-scene-object cases.


With that in mind, I’ve prepared a visual comparison to clarify why the deserialization buffer for the instantiation payload currently lives inside SceneObject.Deserialize
image

As shown above, the HasInstantiationPayload flag is handled symmetrically in both Serialize() and Deserialize(). Keeping the payload deserialization inline within SceneObject.Deserialize ensures that all object-related synchronization logic remains co-located, which helps maintain readability and traceability when following the execution flow.

Moving the deserialization logic to AddSceneObject, while possible, would separate serialization and deserialization across different layers.

That said, since AddSceneObject already deals with other responsibilities like calling SynchronizeNetworkBehaviours, moving the deserialization logic there wouldn’t be unreasonable either, especially if buffer reuse is preferred.

Here's a visual representation of how that could look if moved into AddSceneObject:
image

I’m happy to move the deserialization logic if preferred, just wanted to show why it currently sits in SceneObject.Deserialize() to keep things cohesive and close to where the payload is originally serialized.

Let me know what direction you'd like to take, and I’ll gladly adapt the implementation accordingly.


About the naming
I'm open to renaming the interface and method to better align with NGO conventions. Below are a five of options I’ve considered:

/// Option A  - Short concise and easy to undesrtand
INetworkPrefabInstanceDataHandler
   HandleInstanceData()

// Option B   
INetworkPrefabPreInstanceDataHandler
   HandlePreInstanceData()
   
// Option C   - This is also pretty descriptive
INetworkPrefabInstanceSynchronizer
   SynchronizeInstanceData()
   
// Option D
INetworkPrefabInstanceDataSerializer 
   SerializeInstanceData()

// Option E - For me, this is the most descriptive
INetworkPrefabInstantiancePayloadHandler
   HandleInstancePayload()

Happy to hear your thoughts or preferences here!

@EmandM
Copy link
Collaborator

EmandM commented Apr 30, 2025

I had another pass today. Definitely agree with what you've said about SceneObject! Unfortunately the reason SceneObject.Serialize and SceneObject.Deserialize are not perfect mirrors is due to the deferred messaging system for distributed authority, so we do need to follow the SynchronizeNetworkBehaviours flow. That does mean deserializing inside the AddSceneObject function, just before the call to CreateLocalNetworkObject.

I also took a bit more time with the example. I absolutely see what you're doing there. Thank you for the detailed explanations.

It'll be best if the function naming we go with follows the On<event> naming convention. Also how do you feel about the idea of having the new interface extend from INetworkPrefabInstanceHandler?

Mixing and matching from your naming options, what do you think of something like these two options?

/// Option A  
INetworkPrefabInstanceWithDataHandler
   OnPreInstantiate()
   
// Option B
INetworkPrefabWithSynchronizeHandler
   OnPrefabSynchronize()

@Extrys
Copy link
Author

Extrys commented Apr 30, 2025

I had another pass today. Definitely agree with what you've said about SceneObject! Unfortunately the reason SceneObject.Serialize and SceneObject.Deserialize are not perfect mirrors is due to the deferred messaging system for distributed authority, so we do need to follow the SynchronizeNetworkBehaviours flow. That does mean deserializing inside the AddSceneObject function, just before the call to CreateLocalNetworkObject.

No problem, I’ll make that change in a few minutes!


Also how do you feel about the idea of having the new interface extend from INetworkPrefabInstanceHandler?

Its a perfect idea, making it easier to use for developers. The next commit will include that.


Mixing and matching from your naming options, what do you think of something like these two options?

Since it’s still an INetworkPrefabInstanceHandler, just extended with support for instantiation-time data, I think INetworkPrefabInstanceHandlerWithData makes the most sense.

In contrast, INetworkPrefabInstanceWithDataHandler might suggest that the "instance" has a data handler, which doesn’t quite match the intended semantics.

Would this option work for you?

public interface INetworkPrefabInstanceHandlerWithData : INetworkPrefabInstanceHandler
{
   void OnSynchronizeInstantiationData<T>(ref BufferSerializer<T> serializer) where T : IReadWrite
}

The method name OnSynchronizeInstantiationData clearly indicates that it’s used to synchronize data for the instantiation process, implying that this happens before the object is actually instantiated.

Let me know what you think. I’ll go ahead and push a commit with these changes in the meantime and await your feedback. 😄

Extrys added 2 commits May 1, 2025 01:32
1) Moved the payload deserialization into the AddSceneObject, for deferred instantiation compatibility
2) Changed the new interface to be a direct single extended implementation, instead a complement of the handler
3) Some renames to better match what the feature does for developers
@Extrys
Copy link
Author

Extrys commented Apr 30, 2025

All requested changes have been implemented.

  • Moved the payload deserialization into the AddSceneObject, for deferred instantiation compatibility
  • Changed the new interface to be a direct single extended implementation, instead a complement of the handler
  • Some renames to better match what the feature does for developers

This is the same example shown earlier, but simplified and updated to reflect the new interface and naming conventions:

public class TestHandlerDeterministicLink : INetworkPrefabInstanceHandlerWithData
{
    Dictionary<int, DeterministicIDHolder> deterministicSpawns = new Dictionary<int, DeterministicIDHolder>();

    int nextDeterministicId = 0;

    int customSpawnID = 0;

    public void OnSynchronizeInstantiationData<T>(ref BufferSerializer<T> serializer) where T : IReaderWriter
    {
        serializer.SerializeValue(ref customSpawnID);
    }

    public NetworkObject Instantiate(ulong clientId, Vector3 position, Quaternion rotation)
    {
        return deterministicSpawns[customSpawnID].GetComponent<NetworkObject>();
    }

    public void Destroy(NetworkObject networkObject) => GameObject.Destroy(networkObject.gameObject);

    public void DoSpawn(GameObject linkablePrefab)
    {
        var deterministicIdHolder = GameObject.Instantiate(linkablePrefab).GetComponent<DeterministicIDHolder>();
        deterministicSpawns[nextDeterministicId] = deterministicIdHolder;
        deterministicIdHolder.SetID(nextDeterministicId);
        nextDeterministicId++;
    }
}

Marking the pull request as ready for review ✅
Let me know if anything else is needed!

@michalChrobot
Copy link
Collaborator

michalChrobot commented May 15, 2025

I just want to bump a quick comment to this thread that I really like the cooperation here, how this is developing and while I don't have sufficient knowledge to help more with those topics I love the energy in this thread and I'm looking forward to how it develops 👀 (having in mind that it's always complicated process with many bumps on the way)

@Extrys
Copy link
Author

Extrys commented May 16, 2025

I've already implemented the required changes addressing all feedback:

  • The system is now fully stateless
  • I'm using FastBufferWriter to pass the data between deserialization and Instantiate, avoiding the need for generics in method changes

This version aligns closely with the current architecture and feedback. I’d appreciate any thoughts on whether this direction feels more in line with your expectations, especially considering clarity, maintainability, and minimal disruption to existing workflows

Additionally, I’ve prepared a branch extending this change with more ambitious improvements for future-proofing
I kept it separate to keep this PR clean and easy to compare, it’s a PR on my own fork targeting my local branch.
(Ready to be merged into this PR if you like it)

Key improvements in that extended branch: Please, check extended branch PR

  • New interfaces
    • public INetworkPrefabInstanceHandlerSource: Common abstraction for unifying all current and future handler types
    • internal INetworkPrefabInstanceHandlerAdapter: Centralizes the instantiation flow under a single contract, removing the need for branching based on handler type

While INetworkPrefabInstanceHandlerAdapter remains internal, its design allows safe exposure for custom user-defined adapters if needed

  • Adapter-based routing

    • All handler types (INetworkPrefabInstanceHandler, INetworkPrefabInstanceHandlerWithData<T>, future variants...) now expose an adapter via INetworkPrefabInstanceHandlerSource
    • Eliminates type checks and separate dictionaries by handler type
    • Unifies all instantiation logic: for example, AddHandler now operate via the unified interfaces
    • The adapter determines whether custom data is needed, removing branching and parallel data paths
    • The system operates entirely through the adapter interface, reducing complexity and maintenance overhead
  • Compatibility adapters
    LegacyHandlerAdapter and PrefabInstanceHandlerWithDataAdapter wrap existing implementations without changing their internal behavior, for backwards compatibility

@Extrys
Copy link
Author

Extrys commented May 21, 2025

Hello @EmandM Just checking in to confirm this is still on your radar. Let me know if you'd like me to adjust anything or clarify further.

Just to clarify: the last update message includes two proposals.

  • The base implementation (fully stateless and aligned with the current architecture, and already applied in this PR)
  • A more ambitious version, available as a PR on my fork.

The extended one builds on the base and introduces cleaner abstractions for long-term maintainability, but I kept it separate to avoid adding noise here.
Both versions have been tested in production, and no additional edge cases have been found. All included tests for this feature are still passing.
Switching between approaches does not require any changes to user code.

Copy link
Collaborator

@EmandM EmandM left a comment

Choose a reason for hiding this comment

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

This is looking really good!

There's still some tweaking of the approach needed. The HandleWrapper pattern adds a bit too much complexity. It makes other things seem simple but at the cost of a lot of complexity in one place. I'd rather not use that approach.

I'd also rather not re-allocate the FastBufferReader. I'll see if I get the time to look for some other approaches.

Thanks for your patience with reviews! It's been quite busy on our side.

@Extrys
Copy link
Author

Extrys commented May 23, 2025

Cool! I will be doing some changes this weekend.

@EmandM
Copy link
Collaborator

EmandM commented May 26, 2025

I've found myself with a bit of time today. I see us going round in circles a bit for the structure and flow of this new interface so I'm going to take a deeper dive to see if I find a way through all the contradicting requirements.

@Extrys
Copy link
Author

Extrys commented May 27, 2025

Hey @EmandM, thats nice!
Just checking in case the last review responses slipped through over the weekend
Switching the interface to an abstract class solved a bunch of issues and really cleaned up the code more than expected. :)
#3430 (comment)

From my side, I’m not seeing any remaining conflicting requirements, I think they’ve all been resolved cleanly, unless there are some new internal constraints I’m not aware of yet.

I think the only thing left is choosing between A or B so I can put together a commit that covers the remaining part of your last review.

Quote of the message in question

Okay, this is the last thing pending from this review.

This one took me a bit to solve, I had to dig deeper into FastBufferReader to fully understand. The main target was avoiding buffer copy allocations when extracting a slice of the reader for instantiation data. So my first option was this:

A

fastBufferReader.ReadValueSafe(out int dataSize);
int dataStartPos = fastBufferReader.Position;
unsafe
{
    byte* ptr = fastBufferReader.GetUnsafePtr() + dataStartPos;
    return new FastBufferReader(ptr, Allocator.None, dataSize);
}

This is efficient, avoids allocations as it's pointing to the original buffer, and is consistent with the rest of the (already unsafe) API. The only drawback is requiring unsafe{ } block in that part. That could be avoided by exposing a Slice() method from FastBufferReader.

Alternatively, I tried a safe version using a tracked slice:

B

internal readonly struct ReaderSlice
{
    public readonly FastBufferReader Reader;
    public readonly int Start;
    public readonly int Length;

    public ReaderSlice(FastBufferReader reader, int startPosition, int length)
    {
        Reader = reader;
        Start = startPosition;
        Length = length;
    }

    public void Read<T>(out T value) where T : struct, INetworkSerializable
    {
        var currentPos = Reader.Position;
        Reader.Seek(Start);
        Reader.ReadValueSafe(out value);
        Reader.Seek(currentPos);
    }
}

This avoids unsafe code and keeps the slice strictly tied to instantiation data. It limits misuse but adds friction, since most of the NGO API expects FastBufferReader, so needing a separate type just for one read feels a bit odd to me. Maybe it’s totally fine and it's me overthinking.

Both approaches work fine and still passing the tests. I lean slightly toward the unsafe one for API consistency and performance, but happy to align with your preference.

So I will just wait for your feedback on this :)

@Extrys
Copy link
Author

Extrys commented May 28, 2025

Just to have the code in a fully updated and reviewable state, I went ahead and pushed option A for now. It felt a bit unfinished leaving it undecided, so this way the last review point is addressed and everything is ready for a clean review pass.

If you prefer option B instead, let me know and I’ll switch it.

Copy link
Collaborator

@EmandM EmandM left a comment

Choose a reason for hiding this comment

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

This looks fantastic! I really like the abstract class approach. We're definitely on the final stretch here.

Next steps:

  1. The code needs to be formatted to match our internal format standards. You can run dotnet run --project dotnet-tools/netcode.standards -- --fix to auto fix what can be auto fixed.
  2. The tests are fantastic but don't entirely fit with our testing framework. That's fine, I absolutely don't expect you to fully onboard to that. I can either fix it directly on your branch if you give me access to your repo, or I can try fix things on my side and send you the updated file. The second approach might be a bit slower though.

@Extrys
Copy link
Author

Extrys commented May 28, 2025

Awesome! As it’s more convenient for you to work directly on the tests from my repo, I already gave you access.
I’ll be reviewing the changes from this latest review shortly.

@Extrys
Copy link
Author

Extrys commented May 28, 2025

Most of the feedback from the last round should be addressed in this commit:

  • Created NetworkObjects now get their InstantiationData properly assigned after instantiation.
  • Renamed INetworkPrefabInstanceHandlerWithData.cs to NetworkPrefabInstanceHandlerWithData.cs (dropped the I prefix).
  • Calling Dispose() for FastBufferReader after use (even if using Allocator.Temp for copy, let me know if it's strictly necessary).
  • Reverted the FastBufferReader slice logic back to where it wasn't using the unsafe block.
  • The abstract class remains unchanged: Instantiate and Destroy need to be overridable, and HandlesDataType is already a private, non-virtual interface implementation.
  • Inlined RegisterPrefabInstance() method and removed redundant null check.
  • Writing size to buffer remains unchanged: WriteBytesSafe(byte[]) does not write the length (just checked it), and it’s required explicitly for correct deserialization when slicing. I could use WriteUnmanaged(T[]), but that would require TryBeginWrite(), which I think would overcomplicate things further.

Let me know if anything else should be adjusted.

Regarding formatting: I couldn’t run the dotnet-tools/netcode.standards script locally, as it throws errors when executing Program.cs. Since you have access to the repo, would you be able to run it on your side?

@EmandM
Copy link
Collaborator

EmandM commented May 30, 2025

It makes sense to write the size separately. We shouldn't be snooping on the internals of the serialization anyway. The over-optimization bug apparently got me there.

I've pushed up the formatting fixes and updated the tests to use our test framework, however in doing so I seem to have broken the late joining client test. Do you mind looking into that?

@Extrys
Copy link
Author

Extrys commented May 30, 2025

Before syncing with your changes, the test was failing on my side too because I had forgotten to re-add the missing Seek after reverting the unsafe block approach. Once I added just that, it worked again. I saw you also added the missing Seek in your commit, so the current failure must be coming from somewhere else now. No worries, I’ll take a look and push a fix later today. 👍

@Extrys
Copy link
Author

Extrys commented May 30, 2025

It’s fixed now. The issue was just that the Seek call didn’t include startPosition when calculating the new position, so it ended up seeking too early. Easy miss.
If you feel we are close to the final review or for a merge, I will be setting this as ready for review already.

Let me know if there’s anything else I should take care of, like updating the main post body or anything similar. Not entirely sure what else would be needed at this point.

@Extrys Extrys marked this pull request as ready for review May 30, 2025 10:09
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.

8 participants