feat: add experimental @pixiv/vrm-viewer package #1395
Conversation
There was a problem hiding this comment.
I think we should merge all examples folders at root dir to avoid duplicates of vrms, envmaps, etc...
| @@ -0,0 +1,106 @@ | |||
| import { VRM, VRMLoaderPlugin, VRMUtils } from '@pixiv/three-vrm'; | |||
| import ModelViewerElementBase, { | |||
There was a problem hiding this comment.
For reviewing this reading some <model-viewer />'s source code would be helpful.
| import { ModelViewerElement } from '@google/model-viewer/src/model-viewer'; | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/naming-convention | ||
| export const VRMViewerElement = VRMMixin(ModelViewerElement); |
There was a problem hiding this comment.
not work with <model-viewer-effects /> yet due to node name check. I think it have to be fixed upstream.
There was a problem hiding this comment.
Currently type build fails and waiting for google/model-viewer#4786
https://github.com/pixiv/three-vrm/actions/runs/9203519761/job/25315221048#step:5:252
| @@ -78,6 +83,8 @@ async function buildPackage(absWorkingDir) { | |||
| loader: { '.frag': 'text', '.vert': 'text' }, | |||
| sourcemap: DEV ? 'inline' : false, | |||
| absWorkingDir, | |||
| outdir: 'lib', | |||
| tsconfig: path.join(absWorkingDir, 'tsconfig.json'), | |||
There was a problem hiding this comment.
needed for decorators
There was a problem hiding this comment.
what happens if we don't specify tsconfig here? Does it attempt to discover tsconfig.json near from its project root instead of the cwd?
| absWorkingDir, | ||
| }); | ||
|
|
||
| await esmContext.watch(); | ||
| const extraContext = |
There was a problem hiding this comment.
esbuild is clever enough to not bundle mutiple three, so created a bundle-all build
There was a problem hiding this comment.
so created a bundle-all build
wdym
There was a problem hiding this comment.
When using rollup there is a issue that three is bundled multiple times and that issue is lifted after switching to esbuild; therefore It becomes possible to create a build that includes three (not treat three as external to avoid importmap at all).
There was a problem hiding this comment.
What exactly is "bundle-all" here? Are you simply mentioning the vrm-viewer.js build includes Three.js?
There was a problem hiding this comment.
Are you simply mentioning the vrm-viewer.js build includes Three.js
yes, including three,three-vrm,model-viewer
| function vrmViewOptions(baseBuildOptions) { | ||
| return { | ||
| ...baseBuildOptions, | ||
| format: 'esm', | ||
| entryPoints: { | ||
| [`vrm-viewer${minifySuffix}`]: 'src/index.ts', | ||
| }, | ||
| external: [], // include three | ||
| }; | ||
| } |
There was a problem hiding this comment.
Please clarify the intention why we have to separate options only for vrm-viewer.
I guess it's because:
- we don't have to output
cjsbuilds - we want to specify that the output filename is
vrm-viewer.jsorvrm-viewer.min.js - we want to include Three.js in the build
There was a problem hiding this comment.
we don't have to output cjs builds
YES, but can be avoided by reusing the esm context.
we want to specify that the output filename is vrm-viewer.js or vrm-viewer.min.js
YES, but can be avoided by reusing existing context.
we want to include Three.js in the build
YES, this is the main reason since external is fixed per context.
There was a problem hiding this comment.
would you put them in the code as comments?
|
|
Those have different concerns and controls.
|
Add experimental
@pixiv/vrm-viewerpackage via model-viewer