Skip to content
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

create unique IDs for from prefab instantiated entities and components #2068

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

laske185
Copy link

@laske185 laske185 commented Nov 26, 2023

PR Details

After instantiation of a prefab, all entities and components get a new ID.

Closes: #2067

Description

Fixes the crash of the Game Studio described in #2067 caused by ambiguous IDs. This could also fix some other consequential errors happening through the ambiguous IDs.

Related Issue

#2067

Motivation and Context

The Game Studio crashed each time I changed the hierarchy in my scene.

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My change requires a change to the documentation.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have built and run the editor to try this change out.

@@ -30,7 +31,33 @@ public sealed class Prefab
public List<Entity> Instantiate()
{
var newPrefab = EntityCloner.Clone(this);
CreateNewIds(newPrefab.Entities);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be something for EntityCloner to do? Seems more like a responsability for it, as its goal is to clone a hierarchy of Entitys and EntityComponents so they can be used afterwards.
Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

No, you don't want to duplicate everything. Otherwise, you are never going to be able to reuse stuff.

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't mean to duplicate it, but to move it to EntityCloner, so it can clone an entity tree with or without unique ids.

Copy link
Member

Choose a reason for hiding this comment

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

I wasn't talking about code but about cloning itself.

Copy link
Author

Choose a reason for hiding this comment

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

After a few debugging sessions through the engine, I think it should be fixed in the EntityCloner. See my comment below.

@Kryptos-FR
Copy link
Member

Kryptos-FR commented Nov 26, 2023

I'm surprised by this PR because I recall we should already have code that "fixup" IDs in case it is needed. Did you check for any regression elsewhere?

For instance, does the same issue happen outside of an Entity processor? If not, then it means it is only an issue related to entity processors and shouldn't be fixed in the Instantiate() method. This would otherwise impacts other parts of the engine, which have not been tested for regression here.

On the other hand, if it is a general issue with instantiating prefabs, then we should implement something similar to what is done in the AssetCloner class (see sources/assets/Stride.Core.Assets/AssetCloner.cs and sources/assets/Stride.Core.Assets/AssetClonerFlags.cs).

@laske185
Copy link
Author

laske185 commented Nov 29, 2023

There are at least two ways in which prefabs are instantiated, depending on whether it happens in the Game Studio when the hierarchy is changed or at runtime. The Game Studio uses the AssetCloner for instantiation, as the prefabs are held here as a
PrefabAsset.
When instantiating a prefab or cloning entities at runtime, however, the EntityCloner is used. The EntityCloner is only used within the engine for the public functions for instantiating prefabs and cloning entities. These themselves are not used within the engine.
Cloning is implemented through serialization. In contrast to the EntityCloner, the AssetCloner generates new IDs during deserialization. It is therefore independent of the EntityProcessor. The problem with the ambiguous IDs always occurs when a prefab is instantiated or an entity is cloned at runtime. The only question is whether or when subsequent errors occur.
From my standpoint, it would also make sense if the EntityCloner, like the AssetCloner, generates a new ID when deserializing an entity or component. I would then change the implementation analogous to the AssetCloner.

@Eideren
Copy link
Collaborator

Eideren commented Nov 30, 2023

Yep, sounds alright to me as well, tried it out on my end just to validate and instantiation does copy the Id from the source entity instead of replacing it with another one.

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.

Multiple instantiation of prefabs causes ambiguous IDs
4 participants