Improve "Tags" API

Feature requests
1

Hi,

the “Tags” concept from Babylon.js is a very central piece for our Combeenation configurators for marking and grouping meshes and materials.
However the API for tag handling leaves some room for improvement I would say.
Whilst we could easily create a wrapper around it to adapt it to our needs, I’ll pitch some of my thoughts here, as other projects could benefit as well.

Availability and Typescript support
Seems like there are 2 ways of using tags:

  • Tags.HasTags(obj)
  • obj.hasTags()

The second approach is definitely the more intuitive one.
However the _tags property and all the functions (e.g. hasTags) are not available as typing, since they are injected on runtime via the Tags.EnableFor function.

From an API user perspective I’d suggest to create an interface with all these functions and automatically enable tags for all Node types.
I like to compare tags with metadata, and metadata are available for all Node types as well.

image
image384×127 5.58 KB

One would lose the optimization strategy of lazy loading the tags content, but I’d say that the advantages outweigh.

Set / remove all tags
The API provides some neat functions for adding and removing tags to the existing tags object, but if I want to wipe or exchange the whole tags object, it gets a bit clunky, as I need to get the tags first before I can add them as parameter of the Tags.RemoveTags function.

Depending on the outcome of the topic above, one could just provide setter and getter for the _tags object itself, making a full overwrite possible very easily.

Format
It’s quite hard to understand if one is working with tags as string, object or array.
I’d say a representation as array would make the most sense though.

I haven’t figured out why the object format has been chosen, as the props are always true.
There is no API to set a tag to false and I’d say one would just use the Tags.RemoveTagFrom function in this case.
Getting tags as array is a bit tricky concerning compatibility though, but maybe the new tags getter could help out here.

Convenience functions for asset container
Scene function scene.getMeshesByTags or scene.getMaterialsByTags are really cool.
However we work a lot with a AssetContainer, as it’s the representation of an uploaded model on our platform.
So could we add these functions for asset containers as well?

4 Likes
2

Wow, Tags are being used! :slight_smile:

Tags was implemented as an independent way to tag/mark any object. Not just for nodes. it has no dependency on any other part of Babylon (apart from the and/or parser). Whatever can be added to the Tags class that won’t break backwards compatibility, I am all for it. Especially if you are using it and have good feedback.

1 Like
3

May I suggest to make this on-demand instead? If people do not need Tags (e.g. if an ECS in used), they do not need this extra code. Also, maybe fly weight pattern? If only a subset of meshes receive tags, no need to store a tag property on every mesh.

Wouldn’t a Set or Object be better for performance (afaik lookup is O(1))? Or dependeing on numbers of tags, even just stored as flags in an int?

1 Like
4

I can’t agree more. It was done even before we switched to TS :slight_smile:

5

@TheHOF wanna try a PR for it?

6

This is already the case. I guess an extra improvement would be to dynamically load the Tags module when it is needed, but:

  1. this will break backwards compatibility
  2. The amount of code it will save is redundant.

Code-wise, no tags or extra functions are added to any node, unless explicitly enabled by the Tags module.

That’s why an object was used. It is true that there is no way to change the actual value (i.e. - it is always true), but it is possible to remove a tag. Same same, but different :slight_smile:

7

Thx for the responses, I’d love to try a PR on this topic.

Only thing I am not sure about is the “on-demand” argument.
I want to use stuff like myMesh.addTags directly, without having to activate the Tags class for certain objects via Tags.EnableFor or Tags.AddTags (which calls EnableFor internally).

Also I want to have myMesh.addTags being available for the TS parser at any time.
But that would mean that the Tags class is always added to a subset of classes (e.g. for Node) automatically.

1 Like
8

It could be on demand as long as you import the Tag class and that class makes the change on the mesh (like extending the prototype. I already hear @RaananW hating me for saying that lol)

1 Like
9

Just so I can save this idea for later:

This is the first I’ve noticed the Tags class. I’m thinking this would be a great implementation for the GUI CSS implementation I’m working on. Specifically, using tags as the CSS “class” selector. Previously, I was thinking to ask for a CSSClass property added to the GUI Control class, but the Tags class seems a very apt substitute.

The most relevant description of CSS Class I’ve posted is:

10

I rely on tags in every my project as well. They’re so powerful and easy to use!

1 Like
11

@Deltakosh As the requirements are not completely clear and backwards compatibility and on-demand property handling are holding this back a bit, I am quite hesitant of contributing a PR.
Maybe one of the inner circle Devs want to have a look at it, now that you see that Tags are actually used quite a bit? :smile:
If not, I am also totally with fine it, since we can make a slim wrapper in our Combeenation code base, implementing some important functionalities for our use cases from scratch.

I’d still contribute the AssetContainer.get*ByTags functions, as described in my last point of the original post.
I was curious if we could just move these from the Scene class to AbstractAssetContainer and let Scene extend from AbstractAssetContainer? :thinking:
Otherwise there would surely be some code duplication, which is already the case for the getNodes function (in Scene and AbstractAssetContainer classes)

12

My spider-sense is tingling, that’s for sure :slight_smile:

Is it really needed? Tags don’t have any direct dependency with any other class, they can be dynamically imported and augment anything they want whenever the dev initialize them. No changes to mesh or anything else is needed. So - dynamic import, on demand, already possible! I think? Or am I missing something?

13

I think there are 2 ways of looking at “on-demand” import:

  • adding properties on objects like “_tags, hasTag, etc…” on demand: :white_check_mark: already works via Tags.EnableFor function
  • ES6 dynamic imports: :x: does not work, as the Tags module itself is still statically imported a lot (e.g scene.ts)
    image
    image518×252 15.7 KB

But that wasn’t really my point.
I was asking to remove the first “on-demand” optimization, so that things like mesh.hasTag works with TypeScript.

1 Like
14

got it, makes sense. That can be achieved with prototype extension like we do with other classes. The only “issue” is - at the moment it works with any simple JS object. If we only extend specific classes (i.e. Mesh or even Node) it won’t be available on all other type of objects. If we extend the Object prototype it will be available on every jS object in the typescript project. Might make sense to do that, but that’s manipulating a js native feature, which could be considered the same as global namespace pollution. Dunno, need to think about it a little more.

But I certainly like the fact that you want it improved, and i love the fact that it is being used :slight_smile:

15

FWIW, I also use Tags in my project, but I don’t use TypeScript (and never will), so please don’t break the JS implementation.
The only thing on my wish-list would be support for a simple glob search, like ‘myTag*’, but it hasn’t been important enough yet to implement it. I also never remove a tag, once set, so don’t really need an optimised unset.

I have to say, i was somewhat surprised by the implementation, but it works, so I have other, much larger fish to fry.