Standardize clone depth of drawable configurations

[Cyclone1337]

Closes #987

Summary

This PR reviews and standardizes the clone depth of drawable configurations.

The contained configurations of DrawableConfig are now cloned independently instead of being retained by reference. Mutable nested state in the individual drawable configurations is cloned where appropriate, while intentional sharing is preserved and documented.

Changes

• Deep clone the configurations contained in DrawableConfig.
• Deep clone nested configurations and collections in MindMapNodeConf, including BranchLineConf.
• Preserve the intentional sharing of GameObject references contained in MindMapNodeConf.Children.
• Preserve null ImageConf.FileData values while independently copying existing file data.
• Validate required image data before creating AddImageNetAction.
• Add CloneWithoutDrawableTypes() to avoid unnecessary deep cloning in surface network actions.
• Document the clone depth of all relevant drawable configuration types.
• Add regression tests for mutable collections, nested configurations, null values, and clone independence.

Testing

• Added and executed regression tests for all relevant drawable configuration clone methods.
• Executed the complete EditMode test suite successfully.
• Verified the clone behavior at runtime in SEE, including line caps, image file data, mind map branch configurations, DrawableConfig.Clone(), and CloneWithoutDrawableTypes().

[Copilot]

🟡 Changes recommended

Image validation accepts empty data that cannot produce a valid non-web image.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Standardizes drawable configuration cloning so mutable nested state is independent while intentional GameObject sharing remains intact.

Changes:

• Deep-clones drawable configurations, collections, and mutable nested data.
• Adds lightweight surface-only cloning and image-data validation.
• Adds regression tests and clone-semantics documentation.

File summaries

File	Description
Assets/SEETests/Net/Actions/Drawable/AddImageNetActionTests.cs.meta	Registers the test asset.
Assets/SEETests/Net/Actions/Drawable/AddImageNetActionTests.cs	Tests image validation.
Assets/SEETests/Net/Actions/Drawable.meta	Registers the test folder.
Assets/SEETests/Net/Actions.meta	Registers the test folder.
Assets/SEETests/Net.meta	Registers the test folder.
Assets/SEETests/Game/Drawable/Configurations/TextConfCloneTests.cs.meta	Registers the test asset.
Assets/SEETests/Game/Drawable/Configurations/TextConfCloneTests.cs	Tests text cloning.
Assets/SEETests/Game/Drawable/Configurations/MindMapNodeConfCloneTests.cs.meta	Registers the test asset.
Assets/SEETests/Game/Drawable/Configurations/MindMapNodeConfCloneTests.cs	Tests nested and collection cloning.
Assets/SEETests/Game/Drawable/Configurations/LineConfCloneTests.cs.meta	Registers the test asset.
Assets/SEETests/Game/Drawable/Configurations/LineConfCloneTests.cs	Tests line cloning.
Assets/SEETests/Game/Drawable/Configurations/LineCapConfCloneTests.cs.meta	Registers the test asset.
Assets/SEETests/Game/Drawable/Configurations/LineCapConfCloneTests.cs	Tests line-cap cloning.
Assets/SEETests/Game/Drawable/Configurations/ImageConfCloneTests.cs.meta	Registers the test asset.
Assets/SEETests/Game/Drawable/Configurations/ImageConfCloneTests.cs	Tests file-data cloning.
Assets/SEETests/Game/Drawable/Configurations/DrawableConfigCloneTests.cs.meta	Registers the test asset.
Assets/SEETests/Game/Drawable/Configurations/DrawableConfigCloneTests.cs	Tests aggregate and surface-only clones.
Assets/SEETests/Game/Drawable/Configurations.meta	Registers the test folder.
Assets/SEETests/Game/Drawable.meta	Registers the test folder.
Assets/SEETests/Game.meta	Registers the test folder.
Assets/SEE/Net/Actions/Drawable/SurfaceNetAction.cs	Uses surface-only cloning.
Assets/SEE/Net/Actions/Drawable/AddImageNetAction.cs	Validates image data before cloning.
Assets/SEE/Game/Drawable/Configurations/TextConf.cs	Documents clone depth.
Assets/SEE/Game/Drawable/Configurations/MindMapNodeConf.cs	Deep-clones nested configurations and collections.
Assets/SEE/Game/Drawable/Configurations/LineConf.cs	Documents line clone semantics.
Assets/SEE/Game/Drawable/Configurations/LineCapConf.cs	Documents line-cap clone semantics.
Assets/SEE/Game/Drawable/Configurations/ImageConf.cs	Preserves null or copies file data.
Assets/SEE/Game/Drawable/Configurations/DrawableConfig.cs	Deep-clones contained configurations.

Review details

Files not reviewed (13)

• Assets/SEETests/Game.meta: Generated file
• Assets/SEETests/Game/Drawable.meta: Generated file
• Assets/SEETests/Game/Drawable/Configurations.meta: Generated file
• Assets/SEETests/Game/Drawable/Configurations/DrawableConfigCloneTests.cs.meta: Generated file
• Assets/SEETests/Game/Drawable/Configurations/ImageConfCloneTests.cs.meta: Generated file
• Assets/SEETests/Game/Drawable/Configurations/LineCapConfCloneTests.cs.meta: Generated file
• Assets/SEETests/Game/Drawable/Configurations/LineConfCloneTests.cs.meta: Generated file
• Assets/SEETests/Game/Drawable/Configurations/MindMapNodeConfCloneTests.cs.meta: Generated file
• Assets/SEETests/Game/Drawable/Configurations/TextConfCloneTests.cs.meta: Generated file
• Assets/SEETests/Net.meta: Generated file
• Assets/SEETests/Net/Actions.meta: Generated file
• Assets/SEETests/Net/Actions/Drawable.meta: Generated file
• Assets/SEETests/Net/Actions/Drawable/AddImageNetActionTests.cs.meta: Generated file

• Files reviewed: 15/28 changed files
• Comments generated: 1
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[koschke]

There were only very minor things, which I fixed myself.
I renamed the new test classes so that they start with the prefix Test for consistency with our other test classes.

I will ask Claude for a second opinion tomorrow.

[koschke]

Here is Claude's opinion.

Review: improvement/987-standardize-drawable-clone-depth

18 commits, 28 files, +739/−50 vs master. Eight production files (six drawable config classes, two net actions); the rest is new tests plus .meta files. The change replaces three TODO (#987) markers with actual clone semantics.

Findings

• [DrawableConfig.cs:183](Assets/SEE/Game/Drawable/Configurations/DrawableConfig.cs:183) — Clone() is the only clone method in this PR left without null handling for its collections, while the same class guards those lists in SaveAttributes.

That is the only issue I'd raise, and it is a consistency gap rather than a regression — the previous ToList() threw on null too.

What I checked and found correct

Several things in this diff look risky and turn out to be fine; worth recording so they don't get re-litigated:

• SurfaceNetAction behavior is preserved exactly. The old code cloned then called .Clear() on four lists; the new CloneWithoutDrawableTypes() omits them entirely. That is equivalent only because all four fields carry = new() initializers, so the omitted properties come back as empty lists rather than null. I verified the initializers rather than assuming them — had they been absent, this would have silently turned empty lists into nulls across every surface net action.
• The new AddImageNetAction validation does not break the URL-image path, which was my main suspicion, since the constructor demands non-empty FileData and then deliberately empties it for web images. Every producer supplies real data: ImageConf.GetImageConf reads from ImageValueHolder, AddImageAction captures its memento straight after PlaceImage, and on the receiving client both web branches refill FileData before RePlaceImage. The one path that yields null — ImageConf.Restore when the file is missing locally — already threw NullReferenceException inside Clone() before this change, so the new ArgumentException is strictly more informative.
• MindMapNodeConf now cloning BranchLineConf is a fix, not a behavior break. Previously the field was absent from the object initializer, so clones silently defaulted it to null. The consumer at GameMindMap.cs:969 is null-guarded, and the practical beneficiary is DrawableConfigManager.GetSinglePageConfig, which previously lost branch-line styling when extracting a page.
• The tests can see the internal members they call (ValidateImageConfiguration, CloneWithoutDrawableTypes). SEETests is a separate assembly, but Assets/SEE/SEE.cs declares [assembly: InternalsVisibleTo("SEETests")].
• The branch compiles. Roslyn against Library/ScriptAssemblies gives the same 1644-error baseline as master, with no errors in any of the eight changed production files.

Two notes outside the diff

ImageConf.Restore reads the URL from pathLabel instead of urlLabel ([ImageConf.cs:157](Assets/SEE/Game/Drawable/Configurations/ImageConf.cs:157)), so every restored image gets URL == Path. That matters here because it makes !string.IsNullOrEmpty(Conf.URL) true for restored local images, sending them down AddImageNetAction's web branch, where the receiver will try to download a filesystem path. Pre-existing and untouched by this PR, but it sits directly under the code being changed.

The childrenNames != null guard added to MindMapNodeConf.Clone() is unreachable — that field has an initializer and is never assigned null anywhere in the class. Harmless, just not load-bearing like the Children guard beside it.

Finally, TestAddImageNetAction covers the empty and non-empty FileData cases but not the documented ArgumentNullException for a null imageConf. Worth adding, and note NUnit's Assert.Throws<T> matches the exact type, so Assert.Throws<ArgumentException> would not accept an ArgumentNullException if the validation were ever reshuffled.

[Cyclone1337]

Thanks for getting Claude's second opinion. I addressed all of the points raised in the review.

DrawableConfig.Clone() now preserves null configuration lists and has a corresponding regression test. I also added the missing test for the documented ArgumentNullException in AddImageNetAction.

I additionally fixed the pre-existing ImageConf.Restore() issue found by Claude, so the URL is now restored from urlLabel instead of pathLabel, and added a regression test for it. Finally, I simplified the redundant null handling for childrenNames in MindMapNodeConf.Clone().

[koschke]

So, should I pull the merge trigger?

[Cyclone1337]

Yes, from my side everything is ready now. Thanks!