-
Notifications
You must be signed in to change notification settings - Fork 98
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
emscripten tbb stuff #653
emscripten tbb stuff #653
Changes from all commits
c7b4215
e11498a
d36cdcb
53a6dfa
9b6d6b5
d3d62c2
82044c7
6937f94
d578c86
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,7 +30,14 @@ file(MAKE_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/examples/built) | |
# ensure that interface files are copied over when modified | ||
add_custom_target(js_deps ALL | ||
DEPENDS manifoldjs | ||
${CMAKE_CURRENT_SOURCE_DIR}/manifold*.d.ts) | ||
${CMAKE_CURRENT_SOURCE_DIR}/manifold*.d.ts | ||
${CMAKE_SOURCE_DIR}/patch-emscripten.sh) | ||
|
||
if(MANIFOLD_PAR STREQUAL "TBB") | ||
add_custom_command( | ||
TARGET js_deps POST_BUILD | ||
COMMAND ${CMAKE_SOURCE_DIR}/patch-emscripten.sh ${CMAKE_CURRENT_BINARY_DIR}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is the patch applied both here and in the manifold.yml file? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The patch applied in |
||
endif() | ||
|
||
# copy WASM build back here for publishing to npm | ||
add_custom_command( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,18 @@ export default defineConfig({ | |
worker: { | ||
format: 'es', | ||
}, | ||
plugins: [ | ||
{ | ||
name: 'configure-response-headers', | ||
configureServer: (server) => { | ||
server.middlewares.use((_req, res, next) => { | ||
res.setHeader('Cross-Origin-Embedder-Policy', 'require-corp'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the cross-origin policy required for SharedArrayBuffer to work, there is no way around this. Hope that github pages supports this. |
||
res.setHeader('Cross-Origin-Opener-Policy', 'same-origin'); | ||
next(); | ||
}); | ||
}, | ||
}, | ||
], | ||
build: { | ||
target: 'esnext', | ||
rollupOptions: { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,19 +14,11 @@ | |
|
||
import '@vitest/web-worker'; | ||
|
||
import {WebIO} from '@gltf-transform/core'; | ||
import {expect, suite, test} from 'vitest'; | ||
|
||
import Module from './built/manifold.js'; | ||
import {readMesh, setupIO} from './gltf-io'; | ||
import {examples} from './public/examples.js'; | ||
import ManifoldWorker from './worker?worker'; | ||
|
||
const io = setupIO(new WebIO()); | ||
|
||
const wasm = await Module(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the weird part. For some reason if we load Perhaps this is caused by pthread worker loader implementation. No idea about this, will try to come up with a minimal example and submit the issue to emscripten. I currently workaround this issue by having only a single manifold worker, and move the get properties and genus calls to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you ever submit this to emscripten? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I forgot to try to reproduce this, will try later. |
||
wasm.setup(); | ||
|
||
function initialized(worker) { | ||
return new Promise((resolve) => { | ||
worker.onmessage = function(e) { | ||
|
@@ -58,29 +50,16 @@ async function runExample(name) { | |
URL.revokeObjectURL(glbURL); | ||
glbURL = e.data.glbURL; | ||
if (glbURL == null) { | ||
reject('no glbURL)'); | ||
} | ||
const docIn = await io.read(glbURL); | ||
const nodes = docIn.getRoot().listNodes(); | ||
for (const node of nodes) { | ||
const docMesh = node.getMesh(); | ||
if (!docMesh) { | ||
continue; | ||
} | ||
const {mesh} = readMesh(docMesh); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, the one problem I see with this refactor is we're no longer testing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually we do test writeMesh, in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I see it now - okay, that's perfect. Thanks! |
||
const manifold = wasm.Manifold(mesh); | ||
const prop = manifold.getProperties(); | ||
const genus = manifold.genus(); | ||
manifold.delete(); | ||
// Return properties of first mesh encountered. | ||
resolve({...prop, genus}); | ||
reject('no glbURL'); | ||
} | ||
resolve({...e.data.prop, genus: e.data.genus}); | ||
} catch (e) { | ||
reject(e); | ||
} | ||
}; | ||
|
||
worker.postMessage(examples.functionBodies.get(name)); | ||
worker.postMessage( | ||
{needProps: true, script: examples.functionBodies.get(name)}); | ||
}); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
#/usr/bin/env bash | ||
if test -f $1/manifold.js; then | ||
# I still have no idea why this is needed... | ||
sed -i 's/new Worker/new (ENVIRONMENT_IS_NODE ? global.Worker : Worker)/g' $1/manifold.js | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have no idea why Also,
while
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it seems that nodejs dynamic import will define There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From your other comment it sounds like you think this might be a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it is related to |
||
fi | ||
sed -i 's/var nodeWorkerThreads=require("worker_threads");/const{createRequire:createRequire}=await import("module");var require=createRequire(import.meta.url);var nodeWorkerThreads=require("worker_threads");/g' $1/*.worker.js | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just some ES6 compatibility issue. This is something upstream should fix, and pretty easy to fix. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this one that already got fixed upstream? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, but I think they have not yet released a new version. |
||
sed -i 's/__filename/import.meta.url/g' $1/*.worker.js |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't set it ourselves,
PTHREAD_POOL_SIZE
defaults to zero and I think it is a link-time constant, so we must set it to some sensible value here...