godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
91.53k stars 21.26k forks source link

Object metadata. What to do with it? #18591

Closed reduz closed 5 years ago

reduz commented 6 years ago

As you guys probably know, all objects in Godot can have metadata. This is accessed from code as:

obj.set_meta("key",value)
obj.has_meta("key")
obj.get_meta("key")

This metadata is even serialized, but currently it's usage by the community is rather unclear to us developers. In the editor itself, It's mostly unused. There remain a few cases where the 2D editor puts metadata into nodes (for locking and marking as bones) but this could be moved away.

Metadata can't be edited by users from the UI. This would be doable, but then editor should not store data in there anymore. This is also easy to do.

I'm also not sure if anyone uses these functions from code in games.

So, we were discussing with other devs, what should we do with it? I see three paths for this:

1) Move the remaining usages of it away from it and deprecate the feature 2) Move the remaining usages of it away from it, but keep the feature AND make metadata available for editing in inspector 3) Move the remaining usages away from it and do nothing.

Feedback very welcome on this!

ElfEars commented 6 years ago

2 definately. This is a really useful feature for distinguishing stuff and 2 makes it so much more viable.

ignaloidas commented 6 years ago
  1. It's a feature I would definitely use if it would be made more available, and I think it would be a waste to deprecate this feature
EliseoDiegoViola commented 6 years ago

3 For coherence 2 If you want to take an extra step.

The feature is extremely useful for plugins which import assets into the game.

Also, since we are talking about metadata , I would like to bring into the table the chance to define metadata in tiles.

ghost commented 6 years ago

I use it all the time when I want to quickly store data in nodes that don't have script attached (so I have no control over their member vars). Anything will do, just don't remove it. :P

adeluiz commented 6 years ago

If 1 makes Godot faster, then 1. Else 2

pchasco commented 6 years ago

I think in most cases where someone would use metadata, an export var would be more appropriate. That does require the additional step of creating a script and entering a line of code, so that is somewhat inconvenient.

One usage of meta that I can see is if another node wants to modify the metadata of a node. For example, to mark the node as having been seen in a stealth-type game. This obviously breaks all kinds of best practices for software design, but we’re just making games here. A finished game is the goal; the most elegant scripts are worth nothing if the game is never finished.

I say leave it in, but don’t use it for any internal Godot functions.

BeayemX commented 6 years ago

In the high level multiplayer tutorial it is used to store the network_peer

http://docs.godotengine.org/en/3.0/tutorials/networking/high_level_multiplayer.html

waldson commented 6 years ago

I use it to store Tiled's custom properties.

willnationsdev commented 6 years ago

In the custom type system, it is used to store the overriding _editor_icon that shows the custom type's icon in the Scene Dock / CreateDialog. In the stuff I'm working on in the editor, I'm currently using it to store the custom type's fully qualified typename (including namespace). This allows me to track editor-specific data without needing to modify core to add unnecessary, use-case-specific properties to the Object class.

If you DO go with option 2, you might(?) wanna make it so that meta properties can be hidden from the Editor view that edits the values (perhaps only displaying values that don't have a leading underscore?).

I would recommend 2 or 3. I generally think that using metadata for general properties on a node is a bad idea though. My use case just works well because the alternative would be adding code-bloat to the Object class in core.

FabienDeshayes commented 6 years ago

I see the main usage being importers adding their own custom properties / data structure. For example the tiled-importer.

I'm in favor of 3, just because I don't see much value in making this editable: if the data is imported via an addon, I treat is as read-only and will change it via the source only. It would be nice to see it but I don't think of it as important.

Bauxitedev commented 6 years ago

I vote for 2 simply because the Tiled importer I used half a year ago for a game of mine put custom properties into the metadata. Without metadata that would've been a big headache to deal with.

Additionally, being able to edit metadata from the Inspector would be very useful for storing "material properties" inside of SpatialMaterials, comparable to $surfaceprop in the Source engine. Basically that means some materials have different friction/decals/sounds when you step on them or particles when you shoot them. Metadata is an ideal way to store this.

Norrox commented 6 years ago

2!

neikeq commented 6 years ago

One thing to keep in mind is that the metadata field makes the Object class a bit fatter, so there is a price for keeping it. An alternative to removing it could be improving it. I find the implementation being a Variant Dictionary to be unnecessarily slow. Why not something like an array of components? That would fit the needs of those using it the way the tiled-importer does, and improve performance.

reduz commented 6 years ago

I find the implementation being a Variant Dictionary to be unnecessarily slow. Why not something like an array of components?

Probably would be better as an actual hash map from StringName to Variant, keeping insertion order. This way it's only a pointer when unused.

CodeChomper commented 6 years ago

2 please that way I don't have to make a script to attach data to a scene.

willnationsdev commented 6 years ago

Is there any way to attach metadata to a scene itself? I wouldn't think so since it'd have to be saved into the scene file.

neikeq commented 6 years ago

@willnationsdev Either to the PackedScene, the SceneState or the root Node. There's nothing else closer to be considered a scene.

willnationsdev commented 6 years ago

@neikeq Right, but if you unloaded and then reloaded the PackedScene (and if you didn't want to instance the scene to store additional data about it), then that data would be lost, wouldn't it? Or would the PackedScene's metadata be stored inside the .tscn file too?

neikeq commented 6 years ago

@willnationsdev Not sure if I understand the cases you mention. But if the PackedScene resource has the metadata asigned at the time it's saved to disk, then it should be stored in the .tscn.

vinterskog commented 6 years ago

Didn't even know about this! Could this be used to store additional per-tile data in TileMap and GridMap? I guess probably not since all they store are simple IDs, right? Anyway, I think this is a useful feature and shouldn't be removed. One question though: It seems as if for every object in Godot, an additonal dictionary is stored for this. Does this mean that there is some memory overhead to objects because of this? Even if the dictionary isn't actually used?

willnationsdev commented 6 years ago

@vinterskog I suspect so based on reduz's reply earlier:

Probably would be better as an actual hash map from StringName to Variant, keeping insertion order. This way it's only a pointer when unused.

And the only way you'd be able to use the TileMap's metadata to store tile data would be to use the tile ID as a key to a sub-HashMap or something like that...if I'm not mistaken. I know there is another Issue about that topic already.

LikeLakers2 commented 6 years ago

2, most definitely. We already have many many ways to configure, sort, and interact with our nodes -- what's one more, especially since it's arbitrary data that the dev can parse to their needs?

Zylann commented 6 years ago

I never needed this, even when it comes to adding properties to objects without a script. It's more a last-resort quick-feature for cases where time lacks or every other design patterns fail to address, because it is more convenient in the long term to attach a script in order to declare them (be it a Node or a PackedScene). So even if they were editable in the inspector it would be potentially error-prone, especially when there are many properties. I'm fine with 1 or 2 if most people like the feature. Definitely the editor shouldn't pollute this unless absolutely necessary.

The only time I started needing it was in GDNative to store type tags when they weren't implemented, but that was an old workaround we got rid of.

eon-s commented 6 years ago

Is unused due to the lack of information about it, it can be useful if gets better access (like inspector) and can reduce some complexity in certain nodes, also used on scenes that do not need a script and for "data nodes".

so, 2 too.

ol-smaug commented 6 years ago

It's definitely a convenient feature and is nice to have available in certain scenarios. Either 2 or 3 would be fine so it doesn't go away. :)

CowThing commented 6 years ago

For a long time I didn't even know the metadata existed, and I'm still not completely sure what its use is. If it is kept there should probably be a page in the docs describing it and giving examples.

Ranoller commented 6 years ago

I don´t know if this concerns metadata of TreeItems and similar, but if it is, all my plugins use that (and there are some of they), and i don´t find other way to do that. In game i remember used .set_meta .get_meta in any project... but currently, only in plugins.

So 2. If it include controls And 1. If exposed metadata to gdscript- TreeItem and similar is respected.

willnationsdev commented 6 years ago

@Ranoller the metadata stored in TreeItems is something different I think. That is a Variant specific to TreeItem while we are talking about a Dictionary that every Object has. And the method in the TreeItem is set_metadata(index, value), not set_meta(name, value).

akanewf commented 6 years ago

In my personal opinion it doesn't matter whether it's useful or not, It shouldn't be there.

Be used like a hacker(put some tile data in metadata, used on scenes that do not need a script, etc.) is not a good design pattern.

Those usage should not be the original purpose of design.

Personally, I think it might be better to remove or clarify it.

pchasco commented 6 years ago

Generally I agree with akanewf in that it is not a good pattern, but again, we’re making games. I wouldn’t allow this in any business app of consequence. But sometimes you just have to finish the damn game!

nobuyukinyuu commented 6 years ago

having metadata is nice as a "catch-all" but maybe not necessary when it's so easy to extend a class with an export. It seems to be one of those things to save a little bit of boilerplate in-code without having to box and unbox vars. Although I haven't used the metadata property in Godot yet, it seems similar to how Control.Tag is used in winforms, and I used that a number of times to save some headache. It's "dirty" but it works.

I'd vote option 2 or 3. If the inspector can handle whatever gets thrown into metadata then it could be useful for tool scripts.

volzhs commented 6 years ago

i vote for 2. it's a nice feature to save something without creating and attaching a script on the node.

sprite-1 commented 6 years ago

I have to agree with the majority here. 2 would be the the best option and would probably get more usage out of people who didn't even realize it was a thing beforehand if they can see it in the editor UI as an editable property.

gorgo commented 6 years ago

2, and could be a good idea to support importing obj metadata from Blender etc. (guess FBX supports metadata)

beniwtv commented 6 years ago

I can see this being useful in a number of scenarios, so I agree with the majority here, 2 gets my vote.

Plopsiskopsis commented 6 years ago

2

StraToN commented 6 years ago

I wasn't aware of this feature, so I have absolutely no idea what one can do with it.

lucbf commented 6 years ago

Does this means a major version bump?

willnationsdev commented 6 years ago

@M4gma the master branch has already been bumped? And I don't see why that would be related to this Issue anyway. If they keep metadata and add an editor for it, that wouldn't warrant a major version bump anyway.

vnen commented 6 years ago

Metadata is a bit like groups, in which you can tag objects with some data. But groups are only for nodes, while metadata is for every object. It's a very useful thing for developing interfaces (no wonder Godot editor itself uses it). In particular for plugins, but can be useful in-game too for GUI heavy games.

As some people have mentioned, my plugin to import maps from Tiled use metadata to bring stuff that is available on Tiled but there's no respective property in Godot nodes.

About plugins as well, if the metadata is removed from the editor, the API to access them should be exposed to EditorPlugin in some way.

However, I don't see much use for editing them on the Inspector. Metadata is only useful from scripting. For me would be a 3 if I have to pick, otherwise just leave as it is. I'm not against moving to a StringName -> Variant hash map though.

mhilbrunner commented 6 years ago

I second vnen, its useful for some workarounds; string -> variant hashmap should be fine.

willnationsdev commented 6 years ago

In fact, wouldn't creating a generic StringName -> Variant HashMap editor for the Editor domain be generally applicable? Sounds like you could use that for a variety of things down the road.

lucbf commented 6 years ago

Hmm, I understood move away the usage from the object class, not from the editor. I vote for 2 or 3 then

toger5 commented 6 years ago

I don't like that ppl might be confused what it is good for and are unsure about where to store values (metadata or obj var...) Maybe also might get misused. And if there are elegant work arounds without that feature I think dropping it could be good.

eon-s commented 6 years ago

Is there any example of misuse of metadata in Godot or in general in game development?

vnen commented 6 years ago

I don't like that ppl might be confused what it is good for and are unsure about where to store values (metadata or obj var...)

I believe this can be solved with documentation. Many people don't read it, but still is a place to be pointing to if anyone ask or search for it. Everything can be misused, and it seems more likely that people wouldn't even know it exists (like many people replying to this).

The only workaround I see is applying scripts with exported variables. Not only this is cumbersome, especially when dealing with dynamic objects, but it probably affects performance as well.

legione commented 6 years ago

I would vote for 2 or 3 as many others already said. The main reason is that this feature is incredibly useful for UI elements, where you can store data on the items of the menus or in the itemlists. A compromise would be to remove the feature from all the objects and keep it only for the UI, but I guess that this is more work than just keeping the feature as it is.

Zylann commented 6 years ago

@legione ItemList, menus and other "item-based" controls have a way to associate userdata already. Object metadata wouldn't be useful for them because these items are not nodes. Or maybe you were talking about something else?

legione commented 6 years ago

@Zylann as far as I understood metadata is a property of all objects, not just nodes, and I thought the items' metadata were just inherited from the object class. If it's not the case, option 1 would be fine for me, but the majority seems in favor of keeping them anyway.

mwerezak commented 6 years ago

Has anyone here mentioned any usage of metadata that can't be accomplished with script vars?

It seems like most things that have been mentioned here use them as an awkward work-around for monkey patching.

Personally I'm for 1 if it streamlines the engine. I don't see a use for them (outside of serialization) that isn't something you probably shouldn't be doing anyways.

Otherwise, I think it should be kept only if we decide on a clear reason for it existing.

If we do keep it around for the purpose of monkey patching, it would be nice if there was some support in the gdscript syntax for it, as long as it's small.

(It might be worth noting that using a dictionary in gdscript can emulate monkey patching just fine in many cases)