-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Inspector v2 foundation + some partial impl + stubs #16636
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
base: master
Are you sure you want to change the base?
Conversation
Flesh out other panes a bit more
Fix ESLint issues
Top level readme
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
Reviewer - this PR has made changes to one or more package.json files. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are temp changes to make testing inspector v2 easy. They won't be merged.
Snapshot stored with reference name: Test environment: To test a playground add it to the URL, for example: https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/16636/merge/index.html#WGZLGJ#4600 Links to test babylon tools with this snapshot: https://playground.babylonjs.com/?snapshot=refs/pull/16636/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/16636/merge#BCU1XR#0 |
"@fluentui/react-icons": "^2.0.271", | ||
"react": "^18.2.0", | ||
"react-dom": "^18.2.0", | ||
"usehooks-ts": "^3.1.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an additional dependency that we can discuss. I'm mostly using the local storage hook, which does useful things like watching for local storage changes and updating the state accordingly. I'm also using a theme related hook. This came over with the pivot from the predecessor project this code was part of. If we really don't want this dependency, we could implement our own, but this is a common package and I don't think there is much risk in taking a dependency on it.
Visualization tests for WebGPU |
WebGL2 visualization test reporter: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks solid to me!
|
Hey @kzhsw, thanks for sharing some feedback here!
Yes, in fact the new scene explorer already supports virtualization, and I've tested with a model with ~16k nodes. Expanding the tree is basically instantaneous, and scrolling is very low lag. This is one of the types of improvements that is motivating moving to a modern, well-resourced React UI framework.
A few thoughts on this one:
This inspector v2 work won't automatically address every forum ask for the inspector, but it should address a subset and lead to a lot more flexibility for customizing the inspector to meet a wide variety of needs.
Yes, the plan is to make it the default once it reaches parity with the current inspector and we have sufficient time for community feedback. |
* @returns The current value of the accessor. | ||
*/ | ||
// eslint-disable-next-line @typescript-eslint/naming-convention | ||
export function useEventfulState<T>(accessor: () => T, element: HTMLElement | null | undefined, ...eventNames: string[]): T { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the initial PR i wonder if we should just go ahead and create the new shared tools package to hold the pieces that will be used by both of our workstreams (this would include the components, the hooks, the themes, some of the misc folder that are not specific to inspector )
And then what remains in the InspectorV2 package could be separated into 'truly just used for inspector v2' and a 'sharedTools' folder which holds the things that are meant to eventually move to shared package once/if we embark on adapting our other tools to use the modularTool framework? the main benefit of keeping them in inspectorv2 instead of moving directly to new shared tooling package being that old tools v1 will immediately start using the shared tooling package with my workstream, but won't be usign the modularTool concepts for a while, so we can keep that distinction more clear to open source folks who dont have full context on our future plans
Can you explain what is the gain of a addPropertiesProvider inside a PropertiesServiceDefinition? Before we simply folded line of properties inside a LineContainer. Now I feel it is overly complex so what are we gaining for the increase complexity of having addPropertiesProvider? |
Maybe this is redundant to ask but, I firmly believe the inspector should grow into a full editor for BJS, it just makes sense, every game engine has one, and having it integrated allows for real-time creation / updating. One major feature of that would be the ability to easily "extend" the editor with game specific features, addons, etc. because every game and toolset might have more advanced needs and features, and being able to add windows / controls / modify existing would go a long way I think. These kinds of composable controls are popular with frameworks like Docusaurus, Vite, etc. with the ability to override and extend internal features simply by hooking in. All in all, this is 100% a step in the right direction and we salute you! |
Thanks for the feedback @digisomni! For sure our aim with Inspector v2 is for the most common use cases to be covered by default, optional/opt-in extensions/plugins for more specific use cases, and full extensibility for app-specific use cases. We'll try to get to a point where the community can try it out as fast as possible and give more feedback, and when we are at that point we'll definitely post about it on the forum! |
This PR includes:
Here is what it looks like when testing in the Viewer test app:

Review this PR in this order: