Skip to content

Conversation

@MiroBrodlovaUnityGit
Copy link
Contributor

Overview

This PR replaces all instances of BinaryFormatter with System.Runtime.Serialization.DataContractSerializer in this repo, and is part of Epic SCP-1555.

Previous Behavior

The package used deprecated BinaryFormatter which is deprecated in future .NET versions and may prevent assemblies from unloading or return incorrect results.

New & Expected Behavior

The package now uses DataContractSerializer for serialization/deserialization with [DataContract] and [DataMember] attributes added to all serialized classes.

Changes

  • KdTree.cs: Replaced BinaryFormatter with DataContractSerializer in SaveToFile and LoadFromFile methods
  • KdTreeNode.cs: Added [DataContract] and [DataMember] attributes to enable serialization
  • Added [DataContract] and [DataMember] attributes to all serialized classes to ensure proper serialization

Further Context

  • BinaryFormatter is deprecated in future .NET versions
  • Epic SCP-1555 tracks this migration work
  • DataContractSerializer was chosen as it does not require referencing additional assemblies

JIRA-ticket

Testing

Running the default job-suite. Please share if there are additional steps needed!

BinaryFormatter is deprecated in future .NET versions and may prevent
assemblies from unloading or return incorrect results.

Replaced with System.Runtime.Serialization.DataContractSerializer as
recommended for compatibility.

Changes:
- KdTree.cs: Replaced BinaryFormatter with DataContractSerializer in
  SaveToFile and LoadFromFile methods
- KdTreeNode.cs: Added DataContract and DataMember attributes
- Added [DataContract] and [DataMember] attributes to all serialized classes

Related to SCP-1555
@unity-cla-assistant
Copy link

unity-cla-assistant commented Nov 27, 2025

CLA assistant check
All committers have signed the CLA.

@codecov-github-com
Copy link

codecov-github-com bot commented Nov 27, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

@@           Coverage Diff           @@
##           master     #641   +/-   ##
=======================================
  Coverage   35.57%   35.57%           
=======================================
  Files         277      277           
  Lines       34897    34897           
=======================================
  Hits        12416    12416           
  Misses      22481    22481           
Flag Coverage Δ
mac_trunk 35.33% <ø> (-0.01%) ⬇️
win_trunk 35.23% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@MiroBrodlovaUnityGit
Copy link
Contributor Author

Marking this ready for review just for the purpose of triggering the automatic PR CI jobs (to get a gauge for how these changes fare)! Hope that's OK :)

@MiroBrodlovaUnityGit MiroBrodlovaUnityGit marked this pull request as ready for review January 9, 2026 12:13
@u-pr-agent
Copy link

u-pr-agent bot commented Jan 9, 2026

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

SCP-1555 - Partially compliant

Compliant requirements:

  • Replace/avoid CoreCLR-incompatible (“forbidden”) APIs in packages (notably AppDomain-related and other analyzer-flagged APIs).

Non-compliant requirements:

  • Ensure no packages remain that have analyzer warnings/errors for forbidden API usage (Definition of Done).

Requires further human verification:

  • Verify changes via an appropriate test plan / CI as needed.
  • Provide/attach a technical design document if required by the epic, or state why not needed.
  • Ensure no packages remain that have analyzer warnings/errors for forbidden API usage (Definition of Done).
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

The diff is moderate-sized but changes serialization semantics and may require validation across multiple type combinations and runtime targets.
🏅 Score: 72

The migration removes `BinaryFormatter`, but the current `DataContractSerializer` usage on generic/internal types is likely to break without additional configuration and tests.
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Serialization Breakage

DataContractSerializer for KdTree<TKey, TValue> may fail at runtime because generic parameters and member types (e.g., ITypeMath<TKey>, TKey[], TValue) are not guaranteed to be known/serializable; consider providing known types, a resolver, or redesigning persistence to serialize only the data needed (points/values) rather than runtime helpers like typeMath.

[DataMember]
private int dimensions;

[DataMember]
private ITypeMath<TKey> typeMath = null;

[DataMember]
private KdTreeNode<TKey, TValue> root = null;

[DataMember]
public AddDuplicateBehavior AddDuplicateBehavior { get; private set; }

public bool Add(TKey[] point, TValue value)
{
	var nodeToAdd = new KdTreeNode<TKey, TValue>(point, value);

	if (root == null)
DataContract Visibility

Data contract serialization typically requires members/types to be serializable and often expects public accessibility or explicit attributes; the internal child links and generic node type may need [DataMember] settings (e.g., IsRequired, EmitDefaultValue) and/or making the type/public members compatible, otherwise save/load may throw.

[DataContract]
class KdTreeNode<TKey, TValue>
{
	public KdTreeNode()
	{
	}

	public KdTreeNode(TKey[] point, TValue value)
	{
		Point = point;
		Value = value;
	}

	[DataMember]
	public TKey[] Point;
	[DataMember]
	public TValue Value = default(TValue);
	[DataMember]
	public List<TValue> Duplicates = null;

	[DataMember]
	internal KdTreeNode<TKey, TValue> LeftChild = null;
	[DataMember]
	internal KdTreeNode<TKey, TValue> RightChild = null;
Contract Consistency

Adding [DataMember] to Count and AddDuplicateBehavior risks persisting derived/transient state; validate that Count matches the reconstructed tree and that AddDuplicateBehavior setter-only behavior is correctly restored on deserialize.

	[DataMember]
public int Count { get; private set; }
  • Update review

🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr-agent

@u-pr-agent
Copy link

u-pr-agent bot commented Jan 9, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Prevent non-serializable strategy serialization

Avoid serializing typeMath because it is typically an interface-backed strategy
object and often not round-trippable with DataContractSerializer. Mark it as
non-serialized and rehydrate it after load (e.g., via constructor/factory) to
prevent runtime serialization failures.

External/KdTree/KdTreeLib/KdTree.cs [47-48]

-[DataMember]
+[IgnoreDataMember]
 private ITypeMath<TKey> typeMath = null;
Suggestion importance[1-10]: 8

__

Why: Serializing typeMath (an ITypeMath<TKey> interface strategy) is likely to fail with DataContractSerializer and also makes deserialized trees unusable without rehydrating the strategy; marking it with [IgnoreDataMember] is an important correctness fix.

Medium
  • More suggestions

🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr-agent

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.

3 participants