-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Preserve spawned RelationshipTarget order and other improvements #17858
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
Conversation
|
It would be great to see a test that verifies that order is preserved. I attempted to write one that spawns a scene containing a hierarchy into a world with a different set of archetypes, but it seems that I don't understand the issue well enough to produce a test that fails on main despite the thorough explanation of the issue. |
|
Done! I added a test that forces the archetype init order to mismatch the child order during spawn. This fails on main and passes on this branch. |
|
Feedback on the minor changes. Grumble grumble about them being rolled into one PR though!
Great!
Always nice.
Good.
This is much nicer.
Good, I think this is clearer.
Sure :) |
villor
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
This also seems useful for micro-optimized ”risky” surgical relationship changes from user-code.
Co-authored-by: Viktor Gustavsson <[email protected]>
Fixes #17720
Objective
Spawning RelationshipTargets from scenes currently fails to preserve RelationshipTarget ordering (ex:
Childrenhas an arbitrary order). This is because it uses the normal hook flow to set up the collection, which means we are pushing onto the collection in spawn order (which is currently in archetype order, which will often produce mismatched orderings).We need to preserve the ordering in the original RelationshipTarget collection. Ideally without expensive checking / fixups.
Solution
One solution would be to spawn in hierarchy-order. However this gets complicated as there can be multiple hierarchies, and it also means we can't spawn in more cache-friendly orders (ex: the current per-archetype spawning, or future even-smarter per-table spawning). Additionally, same-world cloning has slightly more nuanced needs (ex: recursively clone linked relationships, while maintaining original relationships outside of the tree via normal hooks).
The preferred approach is to directly spawn the remapped RelationshipTarget collection, as this trivially preserves the ordering. Unfortunately we can't just do that, as when we spawn the children with their Relationships (ex:
ChildOf), that will insert a duplicate.We could "fixup" the collection retroactively by just removing the back half of duplicates, but this requires another pass / more lookups / allocating twice as much space. Additionally, it becomes complicated because observers could insert additional children, making it harder (aka more expensive) to determine which children are dupes and which are not.
The path I chose is to support "opting out" of the relationship target hook in the contexts that need that, as this allows us to just cheaply clone the mapped collection. The relationship hook can look for this configuration when it runs and skip its logic when that happens. A "simple" / small-amount-of-code way to do this would be to add a "skip relationship spawn" flag to World. Sadly, any hook / observer that runs as the result of an insert would also read this flag. We really need a way to scope this setting to a specific insert.
Therefore I opted to add a new
RelationshipInsertHookModeenum and anentity.insert_with_relationship_insert_hook_modevariant. Obviously this is verbose and ugly. And nobody wants more insert variants. But sadly this was the best I could come up with from a performance and capability perspective. If you have alternatives let me know!There are three variants:
RelationshipInsertHookMode::Run: always run relationship insert hooks (this is the default)RelationshipInsertHookMode::Skip: do not run any relationship insert hooks for this insert (this is used by spawner code)RelationshipInsertHookMode::RunIfNotLinked: only run hooks for unlinked relationships (this is used in same-world recursive entity cloning to preserve relationships outside of the deep-cloned tree)Note that I have intentionally only added "insert with relationship hook mode" variants to the cases we absolutely need (everything else uses the default
Runmode), just to keep the code size in check. I do not think we should add more without real very necessary use cases.I also made some other minor tweaks:
SourceComponentfromComponentCloneCtx. Reading the source component no longer needlessly blocks mutable access toComponentCloneCtx.RefCellwrapper over the cloned component queue.write_target_component_ptrto simplify the API / make it compatible with the splitSourceComponentapproach.EntityCloner::recursivetoEntityCloner::linked_cloningto connect that feature more directly withRelationshipTarget::LINKED_SPAWNEntityCloneBehavior::RelationshipTarget. This was always intended to be temporary, and this new behavior removes the need for it.