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

Adds a node-compatible export #12

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

gkjohnson
Copy link
Contributor

@gkjohnson gkjohnson commented Oct 16, 2024

Fix #9

  • Moves the node variant of the unwrapper to a node.ts file.
  • Adds a separate node entry to the webpack build so two entry points are built (one for browser, one for node)
  • Adds a second import path xatlas-three/node so the node variant can be used easily.
  • Automatically finds the xatlasjs files and initializes the wasm paths so there's no need to call "loadLibrary" explicitly. Unfortunately I don't think this can be done for the browser variant easily due to the state of workers and wasm imports...
  • Add "xatlasjs" dependency since it's required for node support, now.
  • Await the library to be loaded in the base unwrapper

Once node is working correctly there are a few things I'd like to adjust in future PRs if that's okay - let me know how this sounds:

  • Add a "verbose" flag and / or log callback to enable / disable the automatic logging that occurs when loading the xatlasjs files.
  • Add a "dispose" function to dispose of the wasm memory and workers used by the unwrappers.
  • Remove the need to explicitly pass in three.js' BufferAttribute to the constructor.

Comment on lines +11 to +12
constructor(...args:any[]) {
super(...args);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Typescript seems to not like this spread syntax for passing through the arguments to the parent constructor. I looked it up but there didn't seem to be a clear solution. Do we have to reproduce all the arguments and types for this constructor?

@gkjohnson
Copy link
Contributor Author

I'm also seeing this warning when running the node instance - looks like there's maybe a memory leak in xatlasjs? Or "comlink"?

(node:91465) MaxListenersExceededWarning: Possible EventTarget memory leak detected. 11 message listeners added to [MessagePort [EventTarget]]. Use events.setMaxListeners() to increase limit
    at [kNewListener] (node:internal/event_target:534:17)
    at MessagePort.<anonymous> (node:internal/worker/io:308:12)
    at MessagePort.addEventListener (node:internal/event_target:645:23)
    at /Users/garrett/Desktop/git/xatlas-three/node_modules/comlink/dist/umd/comlink.js:331:16
    at new Promise (<anonymous>)
    at requestResponseMessage (/Users/garrett/Desktop/git/xatlas-three/node_modules/comlink/dist/umd/comlink.js:329:16)
    at Object.apply (/Users/garrett/Desktop/git/xatlas-three/node_modules/comlink/dist/umd/comlink.js:257:24)
    at _onAtlasProgress (/Users/garrett/Desktop/git/xatlas-three/node_modules/xatlasjs/dist/node/xatlas.js:9:63204)
    at wasm://wasm/0008d70a:wasm-function[31]:0xbf8
    at wasm://wasm/0008d70a:wasm-function[133]:0xdfc7

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.

Instructions for running in Node.js?
1 participant