playcanvas / editor

Issue tracker for the PlayCanvas Editor
https://playcanvas.com/
161 stars 28 forks source link

Show guid of Entity in the Inspector #616

Open marcusx2 opened 3 years ago

marcusx2 commented 3 years ago

It would be useful to show the guid in the editor, to be able to use methods like findByGuid

Maksims commented 3 years ago

It would be useful to show the guid in the editor, to be able to use methods like findByGuid

What would be the benefit of using GUID instead of: Script Attribute for entity type, Name or Tags?

marcusx2 commented 3 years ago

@Maksims Other than shorter code because declaring a script attribute takes 3 lines:

Main.attributes.add('StartButton', {
    type: 'entity'
});

and findByGuid takes one(+ 1 line to have a variable store the guid). Although not really an argument since I could declare a script attribute in just one line.

But other than that, nothing. Simply the fact of having another way of fetching an entity. And the fact that we already have findByGuid but no means to use it lol.

Another argument I guess is that using the script attribute takes extra steps. You need to declare it, then you need to drag the entity to the script component in the inspector. I'd argue that using the guid is more straightforward.

In the end it really boils down to allowing the user to do something different. It's not like you'll have to do much anyways, the method already exists. It just needs to be shown in the inspector.

yaustar commented 3 years ago

I think it could be used as way to identify a particular entity with code that can't reference it by entity attribute (eg a global manager) and would be immune by name or hierarchy changes.

Ita also used with the internal class Entity Reference which looks like it could be useful to expose publicly in the long term? ie A way to reference an entity that doesn't exist in the scene at runtime yet and is automatically updated when it does.

Maksims commented 3 years ago

Using ID's should be treated with care, same as ID's for assets. While cloning a project, asset ID changes, entity GUID persists, but, not sure regarding copying entities between projects/branches, etc. So it's worth investigating cases where GUIDs change, and if there are many, then using GUID can be as an anti-pattern, as using asset ID's directly in code.

Also, the script attribute allows each instance have its own ID, instead of one GUID for all instances if hard-coded. And working with artists, definitely not good to use ID's anywhere really.

marcusx2 commented 3 years ago

Using ID's should be treated with care, same as ID's for assets. While cloning a project, asset ID changes, entity GUID persists, but, not sure regarding copying entities between projects/branches, etc. So it's worth investigating cases where GUIDs change, and if there are many, then using GUID can be as an anti-pattern, as using asset ID's directly in code.

Also, the script attribute allows each instance have its own ID, instead of one GUID for all instances if hard-coded. And working with artists, definitely not good to use ID's anywhere really.

I think script attribute is a great way to make your code more accessible for other people who might use it, as it's very easy to link entities in a visual manner. And also as a generic coding pattern, because the script will work with any entity the user attaches on the inspector.

However if you don't want to be able to use any entity, it's a specific one that you want, and you're not really trying to make your code accessible for other people, I think script attributes is not the best solution. In this case referencing a specific entity is better. And findbyguid is better than findbyname because the latter relies on a magic string.

Hopefully GUIDS always stay the same on every occasion.

yaustar commented 3 years ago

And findbyguid is better than findbyname because the latter relies on a magic string.

So do guids as they are a string. The important distinction is more about if/when/do they change.

AFAIK, once an entity has been created, the GUID does not change.

Max is right about forking/creating copies of projects, templates and scenes though. It's not necessarily a pattern we want users to follow without clear warnings.

marcusx2 commented 3 years ago

So do guids as they are a string. The important distinction is more about if/when/do they change.

From the user's perspective, it isn't a magic string. Or rather, it's a magic string without side effects. Because the user can only read these strings and they (hopefully) never change. The same can't be said about findbyname.

Max is right about forking/creating copies of projects, templates and scenes though. It's not necessarily a pattern we want users to follow without clear warnings.

So the guids can change on these cases?

Maksims commented 3 years ago

Here are known to me cases where GUID changes:

  1. Copy entity between projects.
  2. Copy entity between scenes.
  3. Any copy/paste really.
  4. Duplicating scene - all entity GUIDs get changed.
marcusx2 commented 3 years ago

Here are known to me cases where GUID changes:

  1. Copy entity between projects.
  2. Copy entity between scenes.
  3. Any copy/paste really.
  4. Duplicating scene - all entity GUIDs get changed.

I don't quite understand 1 to 3. If you are copying an entity, the entity will have its own guid, different from the original. That's expected.

Duplicating a scene changes the GUIDs? Hm...not sure if this is a problem in practice.

Does the GUID change if you fork a project? This would be a problem.

yaustar commented 3 years ago

For reference, to get the guid of a selected entity in the the Editor, you can get the via the API like so: image

editor.selection.item.get('resource_id')
yaustar commented 3 years ago

Does the GUID change if you fork a project? This would be a problem.

It doesn't change at the moment but that's not a guarantee.

  • Copy entity between projects.
  • Copy entity between scenes.
  • Any copy/paste really.
  • Duplicating scene - all entity GUIDs get changed.

I don't see these issues being an issue because the id of using a guid is to target one specific instance of an entity in the whole app. It's a pretty niche use case but given we can and there is a use case and the API is already public, I don't see the harm of exposing it to the user in some way in the Editor.

ie using it to reference a child entity, that would a bad use case for referencing via GUID. At the same time, so would be be trying to find it via app.root.findByName instead of enitity.findByName which is a common pattern.

We should clearing explain this in the docs and the API docs too.

yaustar commented 3 years ago

Just to re-iterate and to also be on the other side of the argument here, I can't think of a use case where I've ever needed findByGuid except for debugging purposes.

Maybe that's enough of a reason to expose it in the Editor?

Maksims commented 3 years ago

When working with multiple branches/scenes, it would be intuitive to copy an entity from one scene/branch to another. And GUID will be changed, so in this workflow, it will break.

Additionally, there are no docs and rules on where and how GUID is preserved or modified. Encouraging its use has to come with clear information on its persistence and lack of in certain cases.

Also, it creates another way of doing a task, that there are tools for accomplishing already, which is not great for learning APIs.

marcusx2 commented 3 years ago

When working with multiple branches/scenes, it would be intuitive to copy an entity from one scene/branch to another. And GUID will be changed, so in this workflow, it will break.

Even so, I'd still like to use guids. I think a simple warning that guids can change when copying entities is enough.

Additionally, there are no docs and rules on where and how GUID is preserved or modified. Encouraging its use has to come with clear information on its persistence and lack of in certain cases.

I agree.

Also, it creates another way of doing a task, that there are tools for accomplishing already, which is not great for learning APIs.

Disagree on this one. There is currently no good way of retrieving a specific entity. findbyname isn't a good because it relies on a magic string. And I think entity attributes are valid when you want a generic entity input, but if you want to retrieve a specific entity, you should do it with an id.

yaustar commented 3 years ago

but if you want to retrieve a specific entity, you should do it with an id.

I disagree on this aspect as entity attributes does do this and should be the go to solution. It allows the developer to retrieve a specific Entity that is set in the Editor.

It just can't be done across scenes or be done via none pc.script code (e.g a global manager) which is where the use case for the GUID is valid to a degree.

As mentioned before, I've not seen a use case where a findByGuid is needed over the entity reference script attribute or findByTags/findByName beyond debugging purposes.

yaustar commented 3 years ago

Also, it creates another way of doing a task, that there are tools for accomplishing already, which is not great for learning APIs.

We already have the API in the engine as public. This is not a purposely for new API but for showing the data needed for the existing API.

Even if the only real use case is for debugging, I think that is okay to show the GUID in the Editor but not promote the use of it in tutorials, documentation etc