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

USDZ file loader #16005

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Conversation

CedricGuillemet
Copy link
Contributor

@CedricGuillemet CedricGuillemet commented Dec 17, 2024

  • Supports LH/RH
  • Simple validation test
  • Set as experimental for now
  • Module location will change when I'll be done with TinyUSDZLoader new CI pipeline (WIP)
  • _LoadScriptModuleAsync has been copy/pasted. Is there a better way to handle it?
  • Diffuse texture only for now. More material support depending on community and feedbacks (and test files)

@bjsplat
Copy link
Collaborator

bjsplat commented Dec 17, 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.

@bjsplat
Copy link
Collaborator

bjsplat commented Dec 17, 2024

@bjsplat
Copy link
Collaborator

bjsplat commented Dec 17, 2024

@bjsplat
Copy link
Collaborator

bjsplat commented Dec 17, 2024

private _initializeTinyUSDZAsync(): Promise<void> {
return USDFileLoader._LoadScriptModuleAsync(
`
import Module from 'https://lighttransport.github.io/tinyusdz/tinyusdz.js';
Copy link
Member

Choose a reason for hiding this comment

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

We should do that differently in the framework itself. A few notes:

  • Can we host it on our own CDN?
  • We should provide it as part of our @babylonjs/loaders package, if possible
  • This should be overridable. And injectable - i.e. people should be able to host it themselves, or load it themselves and pass the package to the loader

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've reworked the options so user can change module url.
My intent is to do a PR on tinyusdz repo with a new Github action job that will build the wasm and upload it as a npm. Then, use https://unpkg.com like CSG2 did.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, didn't see the csg2 code. i'll resolve this, but we will need to discuss hosting it ourselves.

… usdzLoader

# Conflicts:
#	packages/tools/tests/test/visualization/config.json
}

private static readonly _DefaultLoadingOptions = {
usdLoaderUrl: "https://lighttransport.github.io/tinyusdz/",
Copy link
Member

Choose a reason for hiding this comment

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

default should be https://cdn.babylonjs.com (it is important so we can use the ability to change base URL)

packages/dev/loaders/src/USD/usdFileLoader.ts Show resolved Hide resolved
packages/dev/loaders/src/USD/usdFileLoader.ts Outdated Show resolved Hide resolved
packages/dev/loaders/src/USD/usdFileLoader.ts Outdated Show resolved Hide resolved
packages/dev/loaders/src/USD/usdFileLoader.ts Outdated Show resolved Hide resolved
export function _LoadScriptModuleAsync(scriptUrl: string, scriptId?: string): Promise<any> {
return new Promise((resolve, reject) => {
// Need a relay
let windowAsAny: any;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe type this so at least within this function it is less likely to have a type or type error? And use a map?

Suggested change
let windowAsAny: any;
let windowAsAny: { _LoadScriptModuleResolve?: Map<number, unknown> };

packages/dev/loaders/src/USD/usdFileLoader.ts Outdated Show resolved Hide resolved
* USD(z) file type loader.
* This is a babylon scene loader plugin.
*/
export class USDFileLoader implements ISceneLoaderPluginAsync, ISceneLoaderPluginFactory {
Copy link
Member

Choose a reason for hiding this comment

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

For new code, I would consider not having the plugin itself implement ISceneLoaderPluginFactory. You'll want to add a factory to packages\dev\loaders\src\dynamic.ts, so really you could follow the same pattern for the registerSceneLoaderPlugin side effect.

@CedricGuillemet
Copy link
Contributor Author

Some features are not supported by tinyusdz like flattening for usdz. I've open an issue on tinyusdz repo lighttransport/tinyusdz#216

I'm waiting for some answers before chosing if the work should continue with this PR. Setting it to draft for now.

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.

4 participants