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

Needed changes to enable the inspector on the viewer #15780

Merged
merged 12 commits into from
Nov 15, 2024

Conversation

RaananW
Copy link
Member

@RaananW RaananW commented Nov 6, 2024

Added the ability to toggle the inspector in the viewr alpha test app.

A few notes:

  • Many file changes to the repository were required to get this to work. The reason is that vite requires the moduled scss files to be called ".module.scss" and we called them. ".modules.scss". In webpack it is configurable.
  • as the viewer renders the canvas in a shadow dom, adding the inspector to the canvas as its parent won't work - the css rules will nto be applied there. Therefore it is needed to add it to an inspector host. it defaults to body if not provided, but the body needs to be styled correctly in order for that to work. The viewer has another div element that is stretched on the screen, which is used as the host.
  • The inspector is async-loaded.
  • All require keywords were removed from the source. it is fine with webpack, but not with vite nor rollup

These changes actually allow the inspector to be released as a standalone es6 module (i.e. - not bundled). As this can be considered a breaking change I did not include it in this PR. we can discuss it in the future. for now no change was made to break anything.


EDIT from @ryantrem:

  • After trying for a bit I wasn't able to get this to work with the viewer distribution bundle, so I removed the showInspector function from the viewer element.
  • However, I kept the "show inspector" button in the viewer test app, which directly invokes the inspector (so we can both test inspector with the viewer and use it in local development).
  • I made some small changes to Inspector to make it work with shadow doms.

We can circle back to the inspector integration later and try to get it included as a feature, particularly if we think it is important to have it work with the viewer distribution bundle. At least now with these changes if both the viewer and the inspector are consumed by an app being build/bundled, inspector will work with the viewer.

@bjsplat
Copy link
Collaborator

bjsplat commented Nov 6, 2024

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.

Copy link
Member

@sebavan sebavan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good but one tiny neat, would be cool to be able to align our build tech :-)

packages/tools/viewer-alpha/vite.config.mjs Outdated Show resolved Hide resolved
@ryantrem
Copy link
Member

ryantrem commented Nov 6, 2024

good but one tiny neat, would be cool to be able to align our build tech :-)

Yea I was thinking about this as well. If having Vite and Rollup mixed in is potentially going to cause more future pain, maybe we need to revisit this conversation sooner than later. Main issue is I could not find any way to create an ES bundle with WebPack. I'm not sure if Vite/Rollup can/should replace all our WebPack usage either.

@bjsplat
Copy link
Collaborator

bjsplat commented Nov 6, 2024

@bjsplat
Copy link
Collaborator

bjsplat commented Nov 6, 2024

@bjsplat
Copy link
Collaborator

bjsplat commented Nov 6, 2024

@ryantrem ryantrem marked this pull request as draft November 7, 2024 22:26
Move showInspector from Viewer to ViewerElement
@ryantrem ryantrem marked this pull request as ready for review November 13, 2024 21:30
@ryantrem ryantrem enabled auto-merge November 15, 2024 17:26
@ryantrem ryantrem merged commit ba9fcea into BabylonJS:master Nov 15, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants