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

ext_meshopt_compression support (?) #205

Closed
wants to merge 8 commits into from

Conversation

MatsErdkamp
Copy link
Contributor

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
Copy link
Owner

jnsmalm commented May 19, 2024

@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
Copy link
Owner

jnsmalm commented May 19, 2024

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

@MatsErdkamp MatsErdkamp reopened this May 26, 2024
@MatsErdkamp
Copy link
Contributor Author

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 MatsErdkamp reopened this May 27, 2024
@MatsErdkamp
Copy link
Contributor Author

MatsErdkamp commented May 27, 2024

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
Copy link
Owner

jnsmalm commented May 27, 2024

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
Copy link
Contributor Author

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

@jnsmalm
Copy link
Owner

jnsmalm commented May 27, 2024

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
Copy link
Contributor Author

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
Copy link
Owner

jnsmalm commented May 29, 2024

Yes, something like that would work!

@MatsErdkamp
Copy link
Contributor Author

MatsErdkamp commented May 30, 2024

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
Copy link
Owner

jnsmalm commented Jun 3, 2024

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

@jnsmalm
Copy link
Owner

jnsmalm commented Jun 6, 2024

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
Copy link
Owner

jnsmalm commented Jun 6, 2024

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.

@jnsmalm jnsmalm closed this Aug 28, 2024
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.

2 participants