Babylon.js icon indicating copy to clipboard operation
Babylon.js copied to clipboard

Inspector: METADATA pop-up window feature

Open j-te opened this issue 2 years ago • 12 comments

I have re-approached the code as per this discussion: https://forum.babylonjs.com/t/read-write-metadata-using-the-inspector/34208

Thus a first-attempt at the metadata editor. It's not perfect but in a good state.

Notes:

  • the metadata property can be set to null and undefined just by typing the literal string
  • the metadata property can also be set as a string or JSON object
  • there is some basic validation but aim was focused on UX
  • there is a known glitch where quoted values "a string" gets considered as a JSON in validation (not really an issue)
  • simple prettifying but not full beautifier with colour - considered re-using monaco-editor however it looked over bloaty/unnecessary
  • there is a bit of duplicate css however wanted to avoid conflicts and not affect backwards compatibility
  • minimal impact on performance by using an observer
  • could probably improve the picking trigger/observer but OK for first-release

New METADATA tab: meta-inspector

New METADATA pop-up window: meta-window

meta-window-error

meta-window-undefined

supersedes https://github.com/BabylonJS/Babylon.js/pull/13643

Closes #13027

j-te avatar Mar 24 '23 03:03 j-te

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). To prevent this PR from going to the changelog marked it with the "skip changelog" label.

bjsplat avatar Mar 24 '23 03:03 bjsplat

Snapshot stored with reference name: refs/pull/13671/merge

Test environment: https://babylonsnapshots.z22.web.core.windows.net/refs/pull/13671/merge/index.html

To test a playground add it to the URL, for example:

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/13671/merge/index.html#WGZLGJ#4600

Links to test babylon tools with this snapshot:

https://playground.babylonjs.com/?snapshot=refs/pull/13671/merge https://sandbox.babylonjs.com/?snapshot=refs/pull/13671/merge https://gui.babylonjs.com/?snapshot=refs/pull/13671/merge https://nme.babylonjs.com/?snapshot=refs/pull/13671/merge

To test the snapshot in the playground with a playground ID add it after the snapshot query string:

https://playground.babylonjs.com/?snapshot=refs/pull/13671/merge#BCU1XR#0

bjsplat avatar Mar 24 '23 03:03 bjsplat

Just had a thought after testing the implementation - shouldn't this be a part of the individual object tab? i.e. - open metadata for a specific object? i.e. here (all the way down): image

RaananW avatar Mar 27 '23 15:03 RaananW

Just had a thought after testing the implementation - shouldn't this be a part of the individual object tab? i.e. - open metadata for a specific object? i.e. here (all the way down)...)

I believe it's currently a tool because the properties are core babylon features, may get in the way a bit in the default tab. Once the metadata property becomes more discussed/developed I can imagine improved UI and potentially another pop-out approach rather than window. I can also see the inspector getting a face-lift in the future as more features become developed.

Happy to make a quick change to relocate the button (under DEBUG or METADATA category?) ...if you think this is right - I'll follow your instruction @RaananW.

j-te avatar Mar 27 '23 17:03 j-te

Just had a thought after testing the implementation - shouldn't this be a part of the individual object tab? i.e. - open metadata for a specific object? i.e. here (all the way down)...)

I believe it's currently a tool because the properties are core babylon features, may get in the way a bit in the default tab. Once the metadata property becomes more discussed/developed I can imagine improved UI and potentially another pop-out approach rather than window. I can also see the inspector getting a face-lift in the future as more features become developed.

Metadata is a core feature of a node in Babylon. it is a part of the node's properties (if defined), so it does make sense to have it in properties. In this case it is important to make sure the data updates according to the selected node, and that the external window is being reused when opening the editor (and not open a window per node :-)).

RaananW avatar Mar 27 '23 17:03 RaananW

Note that metadata also exists in the material, scene, animation group, sound, texture, reflection probe, sprite manager, control (GUI). So I think it would make more sense to have something like an "Edit Metadata" button in the property page of the corresponding element, instead of an "Open Metadata Editor" button in the tool page. Also, like @Raanan, why have a popup for this, we could embed the edit in the element property page (in that case, no need for the button, just add a "Metadata" section, or something like that) ?

Popov72 avatar Mar 27 '23 18:03 Popov72

Note that metadata also exists in the material, scene, animation group, sound, texture, reflection probe, sprite manager, control (GUI). So I think it would make more sense to have something like an "Edit Metadata" button in the property page of the corresponding element, instead of an "Open Metadata Editor" button in the tool page. Also, like @raanan, why have a popup for this, we could embed the edit in the element property page (in that case, no need for the button, just add a "Metadata" section, or something like that) ?

in short: so there's more room for the noobs to edit JSON in-place :)

It looks like 2:1 favour for the embedded textarea. I have simulated this which will require a new base-component:

inspector-metadata-properties

The down-side would be onChange would overwrite any values too easily... alternatively we have an additional "Update" button to prevent loss of Object with prototypes (acts as confirmation)?

@RaananW @Popov72

j-te avatar Mar 27 '23 19:03 j-te

I like the UI in the pop up, those buttons just need to be a part of the embedded area instead. Prettify, refresh update. When you refer to onChange you mean when the object selected was changed? In that case the dev will be required to update the object before. The UI can have a notification saying the metadata is "dirty" and should be updated.

RaananW avatar Mar 28 '23 12:03 RaananW

@RaananW it has been re-written and ready for review.

It was necessary to change the components that had pane className to make the metadata paneContainer conditionally render. I think the changes are improvements and look to follow the other panes (debug stats etc). There were additional unnecessary divs removed.

I've added a basic check to prevent corrupting Objects if they contain non-stringifyable values.

The controls' enabled attribute changes based on dirty, protected and types. Hopefully this behaviour will explain to the user enough they need to know.

inspector-prev

inspector-protect-prev

j-te avatar Apr 01 '23 21:04 j-te

@carolhmj @Popov72, could you have a look ?

@j-te when you removed those did you check all the tools using it like GUI and NME ?

Let me move as draft until @RaananW is back from vacation in a couple weeks ?

sebavan avatar Apr 03 '23 09:04 sebavan

@sebavan I've done a reference lookup and seems that the components in ../dev/sharedUiComponents/src/tabs/propertyGrids/gui/ are only used by the inspector. It's definitely worth another pair of eyes to check though please @carolhmj

image

guiEditor have similar named components, but it looks duplicated. I speculate there may have been style conflict issues while trying to consolidate components, during GUI dev.

j-te avatar Apr 03 '23 18:04 j-te

I moved to draft as we are heading real close from the 6.0 release (next week) so let s wait after release to mark as ready again and merge.

sebavan avatar Apr 14 '23 10:04 sebavan

Merged and will follow up when deployed. Thanks for that wonderful PR!

RaananW avatar May 02 '23 13:05 RaananW