Skip to content

Adds support for Dictionary<K,V> for entity remapping.#1

Closed
Kleptine wants to merge 1 commit into
masterfrom
john/dictionary-remapping
Closed

Adds support for Dictionary<K,V> for entity remapping.#1
Kleptine wants to merge 1 commit into
masterfrom
john/dictionary-remapping

Conversation

@Kleptine
Copy link
Copy Markdown
Contributor

@Kleptine Kleptine commented Aug 8, 2023

Adds support for Dictionary<K,V> for entity remapping when copying / loading / instantiating entities.

By default, Unity.Properties treats Dictionaries as readonly when traversing them. This makes some sense, as modifying keys would violate an invariant of the data structure. However, values can be freely modified, so this is overly-conservative.

This change adds a custom handler for dictionaries in the entity remapper, and forces writable traversal of values within the dictionary. Now entity remapping should support Dictionary<K, Entity> or Dictionary<K, List> or Dictionary<K, V> where V is any type that contains an Entity reference.

…loading / instantiating entities.

By default, Unity.Properties treats Dictionaries as readonly when traversing them. This makes some sense, as modifying keys would violate an invariant of the data structure. However, values can be freely modified, so this is overly-conservative.

This change adds a custom handler for dictionaries in the entity remapper, and forces writable traversal of values within the dictionary. Now entity remapping should support Dictionary<K, Entity> or Dictionary<K, List<Entity>> or Dictionary<K, V> where V is any type that contains an Entity reference.
@Kleptine Kleptine requested a review from matteblair August 8, 2023 19:54
@Kleptine Kleptine assigned matteblair and unassigned matteblair Aug 8, 2023
@Kleptine
Copy link
Copy Markdown
Contributor Author

Kleptine commented Aug 8, 2023

I'm going to write up a slightly broader explanation in a second.

For now, at a high level, I'm dipping my toes in modifying the entities package, as it's quite a bit of a footgun that remapping doesn't support Dictionaries. It resulted in no error, just corrupted entity references, which was painful to track down.

For changing the entities package:

  • I'd like to figure out a way to run the Entities package unit tests, so we can feel reasonably confident any of our changes aren't causing issues.
  • We should try not to build too much on top of our changes. Ideally we can revert back to master, if want to see if a bug is a result of our own changes.

// Check if the visited dictionary has TValue of type Entity. This works because this *only* implements
// ITypedVisit<Entity>, so the only time this cast will succeed is when TValue==Entity.
// This is a clever way to do concrete type casting without needing a lot of boxing / allocation.
if (this is ITypedVisit<TValue> typed)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The nature of this casting really broke my brain. You can see how it's used later in this file, which is why I used it here. It's really weird, haha.

@Kleptine
Copy link
Copy Markdown
Contributor Author

Kleptine commented Aug 9, 2023

@matteblair I've found a better fix than this one. This only patches the entity remapper, but there are several other Visitors that would need patching. Instead, I've patched Unity.Properties directly, which seemlessly fixes everything, and without modifying the entities package.

@Kleptine Kleptine closed this Aug 9, 2023
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.

2 participants