godotengine
Show custom data names in the tile set editor
I noticed that the custom properties in the new tile editor have no name, just the index, even if a name is set.
I couldn't find another Issue or PR targeting this.
Which is very annoying if you have several custom properties.
This PR just changes that to show the names instead. Currently it fall-backs to the index.
Thanks for your review.
What you did there renamed the property only in the property list, but does not modify the _get and the _set functions setting those properties.
My intention was not to change the id, but to change the display name.
It actually felt wrong...
However I could not find another way to set the displayed name.
I think I have to dig deeper in the codebase to understand this properly.
@groud After some experimentation, I realized, that (if I don't miss something) the current PropertyInfo cannot hold information about an alternative display name. The only way I can think off would be to use a new PROPERTY_HINT_ with the hint_string, but that would prevent the usage of any other PROPERTY_HINT_ (which would be no problem in the current case as it's not used there, yet.). Also I am not sure if the hint was meant for something like this...
Do you have any other idea? Or do I miss something completely?
After some experimentation, I realized, that (if I don't miss something) the current PropertyInfo cannot hold information about an alternative display name.
Yeah, that's the main problem. I see two solutions:
- Modify PropertyInfo by adding an "alias" property that would be used to display properties with a different label. This is not complicated, but would mean modifying the core for something quite specific. But it may be useful in other areas, so it could be worth.
- Implement an EditorInspectorPlugin to override the way those properties are displayed. It may be a lot of code though.
Actually I was a bit surprised that the PropertyInfo does not allow for something like this. That's why I initially thought it cannot be that hard to display custom titles^^
I pushed 1. as POC. I think it is a good solution, as I don't think the alias is very specific to the current case but may be useful for other things too. However that's nothing I can decide as it must be discussed with the core team. Maybe there is a specific reason that an alias is missing, or maybe it just wasn't needed yet as there is no other that generic feature which allows setting the name...
Not sure if for something so basic like setting a display name, a new EditorInspectorPlugin, which introduces some new complexity, is the better way.
However I will try to look into 2., too. However I have not looked into how to build an EditorInspectorPlugin, yet. I am quite new to the codebase, so I am not sure if I will be successful :-)
This would actually make the tile set editor much more readable when using many custom data fields.
Using the name of the property as an internal index instead of using an actual internal index sounds like the real issue. Creating an alias is a solution but it's confusing. As a core feature, I think it should use more clear field naming. Something like id or index for the internal index and name for the display.
@groud Has there been decision whether this way of adding alias to PropertyInfo is the way to go? Or it needs to be done with EditorInspectorPlugin? I will be giving a go with the project using this PR and I will try to report here any possible issues the PropertyInfo change might have.
@groud Has there been decision whether this way of adding alias to PropertyInfo is the way to go? Or it needs to be done with EditorInspectorPlugin?
Not really. I'd say the PropertyInfo change is probably the easiest solution to implement but it's quite a core change. It might not be accepted, IDK. I'll ask maintainers.
Alright, I've just checked the code, I believe it might be easy enough to make it a plugin. Kobewi pointed me to EditorProperty *prop_ed = EditorInspector::instantiate_property_editor() which can generate a property editor, for which we can set a label using prop_ed->set_label(). So it should be doable easily without touching the core.
@groud Sounds promising! Are you planning on creating a plugin for this or is it currently open for anyone to give it a shot? (I could try, but completly newish to the whole source editing and only basic c++ knowledge). Also does it need to be a whole new plugin or can it be just an edit/addition to tile_set_atlas_source_editor.cpp?
@groud Sounds promising! Are you planning on creating a plugin for this or is it currently open for anyone to give it a shot? (I could try, but completly newish to the whole source editing and only basic c++ knowledge). Also does it need to be a whole new plugin or can it be just an edit/addition to tile_set_atlas_source_editor.cpp?
You can give it a try, I haven't planned to work on it now.
I guess the code could lie in tile_set_atlas_source_editor.cpp, but you need to check how other editors implement it. You can look for classes that extend EditorInspectorPlugin in the codebase.
