search

gkjohnson / three-gpu-pathtracer

Path tracing renderer and utilities for three.js built on top of three-mesh-bvh.
https://gkjohnson.github.io/three-gpu-pathtracer/example/bundle/index.html
MIT License
1.34k stars 133 forks source link

WebWorker in three-mesh-bvh fails when using bundler #107

Closed ajayns closed 2 years ago

ajayns commented 2 years ago

Describe the bug

When using the package as per example in the repo, it crashes cause WebWorker isn't generated/compiled correctly.

To Reproduce

Steps to reproduce the behavior:

  1. Use any bundle like vite, parcel
  2. Import one of the examples, I've used the interior.js from examples to illustrate.
  3. Run on browser, notice error
Uncaught TypeError: Failed to construct 'URL': Invalid URL

Code

CodeSandbox

Live example

CodeSandbox

Expected behavior

I know web workers don't play well with bundlers when imported from a node module, but perhaps probably some solution can be found and noted in the README.md?

I've tried using an inline webworker - but that solution isn't best suited as well cause we have some three.js imports in the worker and it only works for js code can be made into a blob directly.

Screenshots and Repro Model Not much to show here it crashes on load

Platform:

gkjohnson commented 2 years ago

Thanks for the report -- I'm thinking about taking the same approach as three-mesh-bvh and separating out the components that require webworkers into separate exports the user has to import them on their own if it doesn't work with their bundle system.

Ie PathTracingSceneGenerator would just be a synchronous implementation:

import { PathTracingSceneGenerator } from 'three-gpu-pathtracer';

const generator = new PathTracingSceneGenerator();
const result = generator.generate( scene );
// ...

And a new AsyncPathTracingSceneGenerator class (the class names could use some work...) would be added and imported like so (or copied and modified to a projects needs):

import { AsyncPathTracingSceneGenerator } from 'three-gpu-pathtracer/src/workers/AsyncPathTracingSceneGenerator.js';

const generator = new AsyncPathTracingSceneGenerator();
const result = await generator.generate( scene );
// ...

What do you think @ajayns? Any thoughts on better class names would be helpful, as well 😁

ajayns commented 2 years ago

Yep that makes sense to me API wise! Just thinking out loud, would maybe make sense to have "worker" somewhere in the class name so as to indicate to devs that its in fact a worker and hence requires special consideration, esp with bundlers?

gkjohnson commented 2 years ago

@ajayns just added "PathTracingSceneWorker" which will be available in the next release to separate out the worker requirements. Also updated the docs -- let me know how it looks.