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

Expose "meta" to the Inspector #22642

Merged
merged 1 commit into from
Jul 19, 2019

Conversation

YeldhamDev
Copy link
Member

@YeldhamDev YeldhamDev commented Oct 2, 2018

And by "WIP" I mean "I really could use some help on this".

The metadata dictionary is part of the Object class, and exposing stuff from there to the inspector wasn't as easy as I thought it would be.

Initially, it looks fine, it's accessible just below the "Script" subcategory, except that unlike "Script", it also appears in literally all inspectors across the editor.

(It will, hopefully) Closes #5433.

EDIT: Done, see below.

@YeldhamDev YeldhamDev requested a review from reduz as a code owner October 2, 2018 19:46
@Piet-G
Copy link
Contributor

Piet-G commented Oct 2, 2018

Wasn't the plan to eventually deprecate metadata? I can't remember.

@YeldhamDev
Copy link
Member Author

@dualmatrix Sort of, but making it more accessible was also favorable.

@eon-s
Copy link
Contributor

eon-s commented Oct 2, 2018

@dualmatrix may refer to #18591, many users were in favor to keep and make it visible.

@akien-mga akien-mga modified the milestones: 3.0, 3.1 Oct 3, 2018
@YeldhamDev YeldhamDev changed the title [WIP] Expose "meta" to the Inspector Expose "meta" to the Inspector Oct 13, 2018
@YeldhamDev
Copy link
Member Author

@akien-mga Good to go!

Turned the set_hide_script() function into set_hide_object_properties(), so it affects both script and __meta__.

@YeldhamDev
Copy link
Member Author

Conflicts solved.

@groud
Copy link
Member

groud commented Nov 2, 2018

I'm not sure this should be merged before we get rid of all editor usage of metadata.

Else people might erase some values and we will end up with complains about the editor not working.

@YeldhamDev
Copy link
Member Author

@groud I don't see how could this make it possible to break the editor. The only thing this commit does is expose the meta value of nodes to the inspector.

@groud
Copy link
Member

groud commented Nov 2, 2018

@groud I don't see how could this make it possible to break the editor.

The editor uses metadata for internal stuff. For example, the nodes locking or grouping (in the 2D editor) use an hardcoded metadata of "edit_lock" to check whether or not the node is locked.
If we expose that to the inspector, users might mess up with this data, which could break the editor.

@YeldhamDev
Copy link
Member Author

Oh! I didn't even noticed that. Yep, that might be a problem.

@akien-mga
Copy link
Member

Moving to 3.2 milestone as we're about to enter the beta stage and release freeze for Godot 3.1. Only bug fixes against issues of the 3.1 milestone (or enhancements to core features of the 3.1 roadmap) will be considered until 3.1-stable is released.

Note: If it's merged after 3.1-stable is released, this change might be considered for cherry-picking into a later 3.1.x release.

@akien-mga
Copy link
Member

We finally got to review this, sorry for the delay.

The exposure of editor-only metadata is still a problem, but it shouldn't be blocking for this PR, we can handle it afterwards. There would be two options:

  • Hide metadata entries prefixed with _, and make sure all editor-specific metadata use this prefix.
  • Move editor-specific metadata to their own dictionary (out of Object meta). That would be cleaner but also requires adding code for compatibility to convert from meta to editor_meta or similar.

@akien-mga akien-mga merged commit 2ca3e47 into godotengine:master Jul 19, 2019
@akien-mga
Copy link
Member

Thanks!

@groud
Copy link
Member

groud commented Jul 19, 2019

Move editor-specific metadata to their own dictionary (out of Object meta). That would be cleaner but also requires adding code for compatibility to convert from meta to editor_meta or similar.

We don't have to add a new dictionary if we have a way to hide a property from both:

  • the inspector,
  • the documentation (could be kept there if we can specify that the property is editor-only),
  • the API outside of tool scripts.

@akien-mga
Copy link
Member

@groud True, but IMO (ab)using a common dictionary for all editor metadata might be more convenient than manually registering internal properties... though it's true that using properties is cleaner than relying on strings to get metadata from a dict.

@eon-s
Copy link
Contributor

eon-s commented Jul 19, 2019

separate editor metadata may be safer for plugins and complex custom nodes too, in particular with users that depend on regular metadata.

@YeldhamDev YeldhamDev deleted the inspector_metadata branch July 19, 2019 14:58
@YeldhamDev
Copy link
Member Author

@akien-mga Doing some tests seem that everything works mostly fine, even after all this time.

One minor problem (besides the editor metas), is that the metadata properties seem to start with a non-default value, even if they're completely empty:
Screenshot_20190719_135320

This makes so that every node has a "__meta__" key in the scene files, again, even if empty.

This behaviour didn't happen back at 2018, and I don't have a clue in how to fix this.

@Xrayez
Copy link
Contributor

Xrayez commented Jul 19, 2019

This makes so that every node has a "meta" key in the scene files, again, even if empty.

Perhaps there should be a way to write this metadata to all scenes in the project so that it doesn't create noise in version control? At least I'd like to make this manually if it can't be fixed easily.

@aaronfranke
Copy link
Member

The empty __meta__ stored in scene files is pointlessly increasing the average size of tscn files in bytes by about 5%. Glad to see there's already a PR open to fix this regression.

@akien-mga
Copy link
Member

Another issue from this PR, the __meta__ property is exposed in the ProjectSettings docs for some reason:

diff --git a/doc/classes/ProjectSettings.xml b/doc/classes/ProjectSettings.xml
index 7ab29e67a..edfdae0b9 100644
--- a/doc/classes/ProjectSettings.xml
+++ b/doc/classes/ProjectSettings.xml
@@ -161,6 +161,8 @@
                </method>
        </methods>
        <members>
+               <member name="__meta__" type="Dictionary" setter="" getter="" default="{}">
+               </member>
                <member name="android/modules" type="String" setter="" getter="" default="&quot;&quot;">
                        Comma-separated list of custom Android modules (which must have been built in the Android export templates) using their Java package path, e.g. [code]org/godotengine/org/GodotPaymentV3,org/godotengine/godot/MyCustomSingleton"[/code].
                </member>

Given the issues outlined above and the fact that the proposed solution in #30715 is a workaround, and the better solution from #29222 is not ready to merge yet, I'll revert this for now.

This is still a wanted feature though, so I'd encourage @YeldhamDev to reopen a PR with this code which we could test further and merge once all outstanding issues are handled.

Another thing to consider is whether the property should stay named __meta__ if it's now being exposed, as the __ prefix and suffix are meant to identify an internal, editor-only property. Maybe we should actually go for the better solution to have separate data structures for editor-only and user-exposed metadata.

@ballerburg9005
Copy link

For great justice: https://github.com/ballerburg9005/godot-metadata-inspector

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

editing Node's metadata from inside the Inspector
10 participants