jnsmalm / pixi3d

The 3D renderer for PixiJS. Seamless integration with 2D applications.
https://pixi3d.org
MIT License
752 stars 43 forks source link

ext_meshopt_compression support (?) #205

Closed MatsErdkamp closed 1 week ago

MatsErdkamp commented 3 months ago

As mentioned in #89, I implemented meshoptimizer.js' decodeGltfBuffer(). Two compressed models (src: polyhaven.com, CC0) have been added to the test scene and they seem to work. More extensive testing should probably still be done.

jnsmalm commented 3 months ago

@MatsErdkamp Hey, you have done changes in files that probably not should be changed. You have also added a bunch of new files, please correct this.

jnsmalm commented 3 months ago

@MatsErdkamp It would also be great if you could add some tests for this

MatsErdkamp commented 3 months ago

Currently, the meshopt decoding works on what seems to be the majority of meshes. It does coincidentally not work for the meshoptimized teapot model. I think it is close to working but I can't quite figure it out. Closing the PR for now. Apologies. Will reopen if I figure it out.

MatsErdkamp commented 3 months ago

I consulted with Arseny Kapoulkine , the creator of meshoptimizer. With his help I was able to fix the errors. The meshopt test now passes.

Arseny also pointed out the following: "typically you'd only decompress one bufferView once, caching the result, which I would strongly suggest as an optimization after the result is correct". This is something to look into as well.

Also, meshoptimizer's usage technically requires attribution, so it might be nice to add that to the readme if you agree @jnsmalm

jnsmalm commented 3 months ago

Hey again! I compared the build size with this version and the previous one, it's not that great :(

Previous: 166 KB This version: 225 KB

MatsErdkamp commented 3 months ago

Hmm that's indeed annoying.. Any ideas for solutions? A 'bring your own meshopt decoder' approach could maybe work?

jnsmalm commented 3 months ago

Yes, that's what I'm thinking. Do you think you could come up with a proposal how something like this could be designed when loading a model? So a user can create a hook somewhere and inject this type of functionality? That would be awesome!

MatsErdkamp commented 3 months ago

I will try my best. THREE.js takes an approach similar to this I believe:

const modelLoader = new PIXI3D.Model;
  modelLoader.setMeshoptDecoder(meshoptDecoder)
  let model = app.stage.addChild(
    modelLoader.from(resources["assets/teapot/teapot-meshopt.glb"].gltf)
  );

Alternatively I guess it could also look like this:

let model = app.stage.addChild(
    model.from(resources["assets/teapot/teapot-meshopt.glb"].gltf, meshoptDecoder=meshoptDecoder)
  );

Any preference? Or other ideas?

jnsmalm commented 3 months ago

Yes, something like that would work!

MatsErdkamp commented 3 months ago

hmmm, a few things

1) this approach works, users can provide meshoptDecoder as an argument to model.from(). But, since there is also the materialFactory optional argument, you would need to do model.from(source, undefined, MeshoptDecoder)? I am just now learning that optional arguments in javascript work like this. It seems that there are workaround in the current day, but I think they would break compatibility for users that have set materialFactory.

2) the test seems to run fine when doing npm run test:browser, but npm run test fails. Any ideas why? Does it have something to do with the meshoptimizer import?

What do you think about the optional argument stuff? Is there a way to fix it or should another approach be taken? (such as option A I mentioned 3 days ago)

jnsmalm commented 3 months ago

Sorry for late reply, I have been busy with work. I'll take a look soon!

jnsmalm commented 3 months ago

I think the approach where you just add an additional argument to the "from" function would work.

model.from(source, undefined, MeshoptDecoder)

And yes, you would have to provide that undefined value to not break the API. I think it would be best to provide an options object to that function and options objects would have the MeshoptDecoder as a property, like this:

const options = {
  meshoptDecoder: // ...
}

model.from(source, undefined, options)

I'll take a look at your test and see if I can figure it out

jnsmalm commented 3 months ago

As you said about the tests, it's probably due to something with puppeteer. It's ok if the test is only working in browser, it's the most important.

You can do this:

Create a new test file called "./gltf-meshopt.test.mjs" and move your test to that file. Then only include that test file in the browser-test-context.js file, that way the test will only run in browser tests.

Another thing, please don't use "any" for the public API in the "from" method, instead create an interface that follows the expected signature of that object.