elalish / manifold

Geometry library for topological robustness
Apache License 2.0
874 stars 96 forks source link

Error when `slice`-ing the bottom face of a manifold #929

Closed neilpa closed 1 month ago

neilpa commented 1 month ago

It appears the slice function doesn't work as advertised for extracting the bottom faces of a manifold. I'm hitting an undefined error that I can reproduce with a trivial example on manifoldcad.org

const box = Manifold.cube(1)
console.log(box.boundingBox().min);
console.log(box.slice(0));

Produces

[
    0,
    0,
    0
]
TypeError: A[0] is undefined

Related, is there a way to use slice to extract the top faces?

using a height equal to the top of the bounding box will return empty.

Assuming this bug gets resolved, I can work around it by mirroring and slicing the bottom, but that feels like a kludge.

neilpa commented 1 month ago

It actually appears slice doesn't work at all, get the same error with cube(1).slice(0.5). I suspect something is broken with the WASM bindings.

I debugged this a bit, the failure is here in polygons2vec.

Screen Shot 2024-09-15 at 8 59 00 AM

I've got limited WASM experience, but it looks like a WASM-bridged C++ object is getting passed to the function rather than an expected JS array?

Screen Shot 2024-09-15 at 9 01 10 AM

If I'm reading correctly, it's coming directly from Manifold._Slice.

elalish commented 1 month ago

Thanks! Looks like we must be missing a WASM test example that uses slice. @pca006132 wasn't there a recent refactor of this? Maybe something got missed?

pca006132 commented 1 month ago

Yeah, probably something simple, will check later when I have time.

neilpa commented 1 month ago

I noticed project as the same problem. Seems like this is the culprit change - 85d71f5cef43d9b026bc67f9ddf9d64e7872a8e8

elalish commented 1 month ago

There must be something I don't understand here. I just tried

const box = Manifold.cube(1)
const result = Manifold.extrude(box.slice(0), 0.1);

in ManifoldCAD.org and it worked just fine - made a thin box as expected. What is the bug?

neilpa commented 1 month ago

Weird. This works for me now too on manifoldcad.org. However, I double checked on my local build and I can still repro the issue w/out my changes in #930.

This screenshot is with the most recent changes on master (ced53e2)

Screen Shot 2024-09-17 at 6 15 56 PM

dsafa commented 1 month ago

I don't know if this is intended, but the reason manifold.org is working is cause it seems to have to fixed code from the pr? at least I'm able to see the new example on there

image

neilpa commented 1 month ago

I don't know if this is intended, but the reason manifold.org is working is cause it seems to have to fixed code from the pr?

Nope and that would do it. I’m guessing one of the CI actions on PRs is inadvertently deploying/updating manifoldcad.org

pca006132 commented 1 month ago

Ha! This is a security issue.

https://github.com/elalish/manifold/blob/ced53e210972adee15edade7c7308fcf2777f707/.github/workflows/manifold.yml#L154-L161

I think we should only upload the artifact when it is triggered from the master branch.

elalish commented 1 month ago

Ha! Wow, that's not how I thought THAT worked. Besides, I didn't think uploading an artifact did anything - I thought only our deploy action that downloads said artifact and packs it into our Github pages deployment would actually put it on a server. I'm so confused...

pca006132 commented 1 month ago

Well, uploading an artifact probably does nothing, but when you have a race condition and the deployment job is running on the master branch...

elalish commented 1 month ago

Oh, that's interesting. I feel like there's supposed to be a way to ensure you only get an artifact from your same job - am I crazy or did we I mess that up?

pca006132 commented 1 month ago

https://github.com/orgs/community/discussions/106300

Seems a bit complicated.