Inspector v2 bugs and feedback

[ryantrem]

This issue is capturing bugs and feedback from the Inspector v2 forum announcement.

@georginahalpern is marking the items specific to shared controls / UX with 🎨🎛️ (instead of having a separate github issue to track them)

Bugs:

• Errors with React 19
• Vertically resizing panes can get into a broken state (logged an upstream bug here, let's see if we get traction: [Bug](@fluentui-contrib/react-resize-handle): Resizing sometimes fails with a 🚫 cursor microsoft/fluentui-contrib#523)
• Scene Explorer node indentation resets after 10 total levels of depth
• Not all disposable entities have a Dispose button

Feature Gaps (known or forum-identified)

• 🎨🎛️ Copy button for properties
  • https://forum.babylonjs.com/t/introducing-inspector-v2/60937/24
• Close panes (might work differently in v2)
  • https://forum.babylonjs.com/t/introducing-inspector-v2/60937/65

Requests:

• (Investigating) Scene explorer: click on entity text to expand/collapse; expand the click zone beyond the arrow
  • https://forum.babylonjs.com/t/introducing-inspector-v2/60937/14
  • https://forum.babylonjs.com/t/introducing-inspector-v2/60937/49
• 🎨🎛️Properties pane: improve information density (possibly in scene explorer too) (e.g. compact mode)
  • https://forum.babylonjs.com/t/introducing-inspector-v2/60937/24
  • https://forum.babylonjs.com/t/introducing-inspector-v2/60937/28
  • https://forum.babylonjs.com/t/introducing-inspector-v2/60937/49
• (More community input please!) Scene explorer: More filtering options (negative filter, or entity type filters)
  • https://forum.babylonjs.com/t/introducing-inspector-v2/60937/24
• window.debugNode (see https://forum.babylonjs.com/t/scene-debuglayer-copy-last-command-to-clipboard/49796/18)
• 🎨🎛️More granular control over spinbutton increment/decrement using keyboard modifiers
  • https://forum.babylonjs.com/t/introducing-inspector-v2/60937/39
• 🎨🎛️ Basic math calculation in number input controls
  • https://forum.babylonjs.com/t/introducing-inspector-v2/60937/39
• More flexibility when opening Inspector in terms of which services/features are enabled (e.g. excluding certain panes, or the default settings, etc.)
  • https://forum.babylonjs.com/t/introducing-inspector-v2/60937/46
• Some kind of flow graph support
  • https://forum.babylonjs.com/t/introducing-inspector-v2/60937/48
• Move the copy button to declutter the UI (maybe context menu, or ctrl+click on label, etc.)
  • https://forum.babylonjs.com/t/introducing-inspector-v2/60937/49
• Some kind of property filtering
  • https://forum.babylonjs.com/t/introducing-inspector-v2/60937/49
• Dispose entities directly from scene explorer (e.g. delete command, and ideally with keyboard support)
  • https://forum.babylonjs.com/t/introducing-inspector-v2/60937/60
• 🎨🎛️ Replace sliders (for synced slider) with something more inline, like Tweakpane does it
  • https://forum.babylonjs.com/t/introducing-inspector-v2/60937/39
  • https://forum.babylonjs.com/t/introducing-inspector-v2/60937/70
• Scene Explorer sorting
  • https://forum.babylonjs.com/t/introducing-inspector-v2/60937/90
  • https://forum.babylonjs.com/t/introducing-inspector-v2/60937/92
  • Also requested by @coryboyle
• Scene Explorer colored icons
  • https://forum.babylonjs.com/t/introducing-inspector-v2/60937/103
  • https://forum.babylonjs.com/t/introducing-inspector-v2/60937/162
• EntitySelector should maybe always have an edit button (not unlink), and use standard nullable checkbox when nullable, and when entering edit mode, always open the dropdown by default
• Option for unconstrained morph target weights
  • https://forum.babylonjs.com/t/introducing-inspector-v2/60937/179

[coryboyle]

The Scene Explorer can only render a depth of ~8 nodes before the margins of the children get reset.

[ryantrem]

Oh wow, thank you for bringing this to our attention!

[ryantrem]

Fixes for debugNode and Scene Explorer depth limit here: #17304

[eldinor]

Seems like a typo (the dot after question sign)

Babylon.js/packages/dev/inspector-v2/src/components/properties/commonGeneralProperties.tsx

Line 28 in 3020ed3

const className = commonEntity.getClassName?.();

[sebavan]

I always have the same feeling but it works :-) https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Optional_chaining

[aknshr]

Hi.

Regarding 'Some kind of property filtering', here's a suggestion:

• Live test app:
  https://aknshr.github.io/babylonjs-i2/
• Changes:
  master...aknshr:Babylon.js:PinnedSection

I'm new to this and I'm not sure if it's correct, but please let me know if this is of interest.

[sebavan]

CC @ryantrem

[ryantrem]

Thank you @aknshr for testing this out and sharing your approach, it's cool to see a functional POC of this! I think my main thought here is that since with this POC it is effectively a feature of the Accordion component, we can probably handle it entirely in the Accordion component without any changes to property line related components. Accordion already picks apart its children, so it seems like we could:

1. Use the child's key plus the section name containing it as the unique identifier since a child component already must be have a unique key within its Accordion section (though we are currently inconsistent with the use of keys in this context, but we can fix that and require keys).
2. Have Accordion wrap all its children in component wrappers that brings in the pin/unpin support. Accordion is re-rendering the children, so it can do so inside wrapper components.
3. Have that wrapper add/remove the component key to/from a local state array for the special pinned section.

Then, anywhere we use an Accordion, you only need to make sure items inside the Accordion have unique keys, and the pinning can be handled fully from the Accordion component. Curious to hear your thoughts, or if you agree and want to give it a go by all means please do, otherwise we will try to pick this up after we get the rest of the Inspector v2 feature work done to get to parity with v1.

[aknshr]

Hi @ryantrem ! Thanks for your feedback !

I actually tried this approach first, but when I got to (extensibleAccordion.tsx - line 174), the component (e.g. "CommonGeneralProperties"), returns 3 children for properties ("ID", "Name" and "Unique ID") that I couldn't access via props.children from it's grandparent. My goal is to pin individual property lines, however, it seems they're currently grouped within several components. Is there something I'm missing?

[ryantrem]

Oh I see, the children I mentioned in the context of the Accordion are actually the sections, not the children within the sections. If I'm understanding correctly this is why you made it a prop for PropertyLine etc. But I'm also not sure how this would work when PropertyLines are another level deep yet, for example when they are part of a collapsible group (e.g. when they are conditional). If we have a way to pin the direct children of the section in a generic way, then that would mean a collapsible group of property lines would be pinned together in one unit, which I think would make more sense. But I'm also not sure how we can do this unless we introduce something like an AccordionItem wrapper component and require every property line / line container to be wrapped inside one, which would make the source way more verbose. 🤔

[aknshr]

I followed your suggestions and implemented AccordionSectionItem (since AccordionItem already exists). I relocated all logic to the Accordion module, leaving only the wrapper, which forwards the item's ID, in LineContainer. I think this is a good place because we can capture all property lines in a centralized location, independent of their position in the tree, thereby avoiding the need to modify the components that declare the property lines.

I also implemented AccordionSectionContext to capture the section's title and combine it with the item's ID to create a unique ID. I created a new optional itemId prop in PropertyLine and ButtonLine, which can be provided when the label prop (used as default ID) changes dynamically.

• Updated live test app:
  https://aknshr.github.io/babylonjs-i2/
• Changes (comments in code):
  master...aknshr:Babylon.js:PinnedSection-2

Please let me know if you have any questions or suggestions!

[ryantrem]

I think this seems pretty reasonable overall. Some thoughts:

• By using AccordionSectionItem inside LineContainer and PropertyLine, we are kind of saying LineContainer/PropertyLine are really expected to be used within an Accordion. I think this is probably ok. @georginahalpern?
• But also by using AccordionSectionItem inside LineContainer and PropertyLine, it keeps the usage simple. I think the only sweeping change we need is to make sure to have an itemId that is unique within a section for every LineContainer/PropertyLine, right?
• Portals seems like a clever way to solve this problem in a generic way. We can simplify slightly by using the Portal component from @fluentui/react-components (which we already use in a few other cases). It is just a thin wrapper around the createPortal API that lets you create a portal as a React component. So AccordionItem could conditionally render "inline" or within a Portal.
• I think this approach could also support property filtering. We could generalize AccordionPinnedContext to AccordionContext and include a filter string, and AccordionItem could conditionally render nothing. Maybe we could have an additional prop that is a string array of search terms, and be smart about defaults (like including the label of a PropertyLine). This could be done in the future in a different set of changes, but it's worth considering how that scenario could also be solved by this approach.

@georginahalpern any additional thoughts?