Update JS bindings with CrossSection + reorganize #440

geoffder · 2023-05-25T21:26:59Z

Fixes #360

add CrossSection class
move static method (constructors) under their respective classes
add Manifold.{split, splitByPlane}
enable usage of instanceof on Manifold (and now CrossSection) objects
simplify registry indirection in worker.ts, enabling method de-structuring (opening static function names out of class module name-spaces)

Build changes:

break C++ helpers out from bindings.cpp to helpers.cpp
ensure that ts files are copied during builds even when nothing is recompiled

Note that in addition to the new bindings, the most of the existing toplevel functions have been moved under their respective classes as static methods (breaking change!). I haven't added any new examples yet, but the existing ones are passing and do make use of CrossSection now (some transparently, and some directly), but I think enough of this is together that review is worthwhile.

codecov · 2023-05-25T21:53:53Z

Codecov Report

Patch and project coverage have no change.

Comparison is base (9dac47a) 89.67% compared to head (b4f2605) 89.67%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #440   +/-   ##
=======================================
  Coverage   89.67%   89.67%           
=======================================
  Files          35       35           
  Lines        4243     4243           
=======================================
  Hits         3805     3805           
  Misses        438      438

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

geoffder · 2023-05-25T23:08:30Z

Ah looks like the previously mentioned addition to debug in worker.ts must have been dropped while I was resolving conflicts, woops. For reference, I think it might be helpful to allow show and only to work on CrossSection as well, similar to how 2d shapes show up in OpenSCAD's preview mode.

function debug(shape: Manifold | CrossSection, map: Map<number, Mesh>) {
  const manifold = (shape instanceof module.CrossSection) 
                   ? shape.extrude(1).translate([0, 0, -0.5]) 
                   : shape;
  const result = manifold.asOriginal();
  map.set(result.originalID(), result.getMesh());
  return result;
};

Also, after looking at it again just now, if the cleanup step is required to make sure that Manifolds get freed, is debug currently (even without this addition) leaking the result of asOriginal() (in need of a memoryRegistry.push(result))?

elalish · 2023-05-26T05:20:45Z

Good question - I think that gets cleaned up by the modified asOriginal function like the rest, but let me know if I'm missing something. I like the cross-section debug idea - how about extrude 1/100 of the max dimension to make it scale invariant?

elalish

Looking pretty good, but let's clean up the format so I can do a better pass. The examples especially will break pretty badly, since their white space gets processed.

bindings/wasm/examples/worker.ts

bindings/wasm/manifold.d.ts

bindings/wasm/helpers.cpp

geoffder · 2023-05-26T05:41:40Z

Good question - I think that gets cleaned up by the modified asOriginal function like the rest, but let me know if I'm missing something.

This doesn't go through the modified version does it? It comes before the indirection is set up.maybe if it was moved down the file.

I like the cross-section debug idea - how about extrude 1/100 of the max dimension to make it scale invariant?

Sure, that doesn't sound bad.

elalish · 2023-05-26T05:56:53Z

This doesn't go through the modified version does it? It comes before the indirection is set up.maybe if it was moved down the file.

Oh, good catch! Let's do that.

bindings/wasm/examples/vite.config.js

bindings/wasm/examples/public/examples.js

bindings/wasm/examples/worker.ts

elalish

Looking great, thanks! Would you mind pulling master so we can double-check the formatting?

bindings/wasm/examples/public/examples.js

bindings/wasm/bindings.cpp

bindings/wasm/bindings.js

bindings/wasm/examples/public/examples.js

bindings/wasm/helpers.cpp

geoffder · 2023-05-26T23:54:16Z

Gyroid example just refusing to not timeout it seems.

elalish

Looks like this is ready once the tests pass. Thanks! If you click the merge button, please check when deployment completes and then test ManifoldCAD.org. Since we don't have any staging, I'd like to catch problems early.

bindings/wasm/examples/vite.config.js

bindings/wasm/bindings.js

geoffder · 2023-05-27T04:44:19Z

Looks like this is ready once the tests pass. Thanks! If you click the merge button, please check when deployment completes and then test ManifoldCAD.org. Since we don't have any staging, I'd like to catch problems early.

Seems like most things are resolved, but I haven't touched the interface files yet which are giving inaccurate/incomplete completion information with all of the static function reorganization and the CrossSection addition unaccounted for.

elalish · 2023-05-27T05:11:50Z

bindings/wasm/CMakeLists.txt

+set_source_files_properties(bindings.cpp
+  OBJECT_DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/bindings.js
+  # FIXME: can we rewrite the custom command to run after these are modified when building
+  # without recompiling bindings.cpp?


Probably, but my CMake foo is weak. Feel free to play around!

I believe that I've got something working for the interface copying that does not require rebuilding the bindings needlessly. 👍

geoffder · 2023-05-27T06:06:47Z

Ah looks like I should have tested the scale cleanup before committing, might have been what broke here.

- add CrossSection class - move static method (constructors) under their respective classes - add Manifold.{split, splitByPlane}

bindings/wasm/CMakeLists.txt

bindings/wasm/bindings.js

bindings/wasm/examples/worker.ts

bindings/wasm/helpers.cpp

elalish · 2023-05-30T04:55:33Z

bindings/wasm/manifold-encapsulated-types.d.ts

+   *
+   * @param manifolds A list of Manifolds to lazy-union together.
+   */
+  static compose(manifolds: Manifold[]): Manifold;


Nit: should we put all the static functions next to each other?

We could, I was just organizing them by purpose. I'm not sure what the way that this interface will normally be viewed.

Probably mostly by other maintainers. Just try to make it obvious where someone would put the next method.

geoffder · 2023-05-30T05:19:20Z

As an aside, I have come across a funny bug, the following fails in the editor with RuntimeError: index out of bounds

const box = CrossSection.square([80, 80], true).offset(10, "Round").extrude(100, 0, 0, [1, 1], false);

However, changing the final argument (center) to true (which simply translates the manifold down) works.

elalish · 2023-05-30T05:22:35Z

As an aside, I have come across a funny bug, the following fails in the editor with RuntimeError: index out of bounds
const box = CrossSection.square([80, 80], true).offset(10, "Round").extrude(100, 0, 0, [1, 1], false);
However, changing the final argument (center) to true (which simply translates the manifold down) works.

Ugh, WASM errors are so obtuse. Can you repro it in a C++ test? I just fixed some like that for Warp, which turned out to be because I wasn't checking inputs strictly enough.

geoffder · 2023-05-30T05:40:38Z

I did try doing GetMesh on the non-centred version in cpp, and it exported fine without crashing 😬.

TEST(CrossSection, RoundedBox) {
  auto sq = CrossSection::Square({80, 80}, true)
                .Offset(10, CrossSection::JoinType::Round);
  auto uncentred = Manifold::Extrude(sq, 100, 0, 0, {1, 1});
  auto ball = Manifold::Sphere(60, 100);
  auto mesh = (uncentred - ball).GetMesh();
#ifdef MANIFOLD_EXPORT
  if (options.exportModels)
    ExportMesh("cross_section_rounded_box.glb", mesh, {});
#endif
}

It did however take almost 37s to run though.

elalish · 2023-05-30T05:51:59Z

Weird, it seems way too simple to take that long. How many facets for "round"? Oh well, we should probably move this discussion to a separate issue anyway.

geoffder · 2023-05-30T06:42:53Z

Weird, it seems way too simple to take that long. How many facets for "round"? Oh well, we should probably move this discussion to a separate issue anyway.

Ok. It is a pretty weird problem given than a mere translation makes the difference. This use of offset is bringing it out, but where the error is and how insidious I don't know.

pca006132 · 2023-05-30T06:46:50Z

I finally have a break now. I just have a look at it, there are 281010 edges in the uncentred mesh, so most of the time is spent on RecursiveSwapEdge and causing the slowness.

pca006132 · 2023-05-30T06:48:51Z

Related: http://www.angusj.com/clipper2/Docs/Units/Clipper.Offset/Classes/ClipperOffset/Properties/ArcTolerance.htm
Perhaps we should have this in CrossSection, similar to what we have with Manifold.

geoffder · 2023-05-30T06:52:07Z

From how smooth the roundover looks I'm not too surprised.

Calculating a default ourselves based on the delta, rather than leaving it up to the Clipper2 default or the user you mean?

pca006132 · 2023-05-30T06:59:42Z

Yeah, we can allow the user to set the minCircularAngle/circularSegments and calculate it ourselves, or ask the user to specify a minCircularEdgeLength. We should probably have the minCircularAngle set to 10 or something, so the default performance is good enough.

geoffder · 2023-05-31T00:07:42Z

Caught what I hope to be the last bugs while testing out CrossSection in the editor earlier, think it is looking pretty good now.

I remembered a question I'd forgotten around the business in getManifoldDTS in editor.js. I've copied the corresponding updated block over what was there before, but I'm wondering what the purpose is, since naively, it seems incomplete. It gets encapsulated, but then doesn't seem to be aliased as T to work with the subsequent typeof declarations, and the top-level types aren't exported as they are in manifold.d.ts either. Though, again, I've come into working on this with no familiarity to the js/ts ecosystem, so I could be missing something obvious.

If this stuff is actually (still?) necessary, it seems pretty fragile given that it requires manually copying over the interface into the string block (though it isn't something that happens too often I suppose).

elalish · 2023-05-31T06:05:25Z

I believe #348 is where the getManifoldDTS came from - linked issues there may give a little background. It's a total hack to work around Monaco not liking .d.ts files that import others. I don't have a chance to review right now - it's entirely possible it is now out of date or there's a better way.

geoffder · 2023-05-31T22:05:35Z

Ok, well I think I'll leave it for another PR then if any problems arise. The types are working for completion and linting in my use of the editor locally at least.

elalish

Just some minor things and then I think we can merge. Let's try to avoid any more functional changes to this PR so we can close the review cycle. We'll need another one before release anyway to address the offset API.

elalish · 2023-06-01T04:42:12Z

bindings/wasm/examples/public/examples.js

-          circle.push([Math.cos(dPhi * i) + offset, Math.sin(dPhi * i)]);
-        }
+        const offset = 2.
+        const circle = CrossSection.circle(1., n).translate([offset, 0.]);


No need for . in JS - everything is a double - even integers!

Ah ocaml muscle memory acting up again.

bindings/wasm/examples/public/examples.js

elalish · 2023-06-01T04:46:29Z

bindings/wasm/examples/public/examples.js

+      const {square} = CrossSection;
+      const box = square([70, 70], true)
+                      .offset(15, 'Round')
+                      .extrude(100, 0, 0, [1, 1], true);


I appreciate the idea, but I'm intentionally keeping this first landing-page example super simple. Let's revert this for now. We should add a cool example that showcases more of the CrossSection API though at some point.

elalish · 2023-06-01T04:49:05Z

bindings/wasm/helpers.cpp

+  auto jt = join_type == 0   ? CrossSection::JoinType::Square
+            : join_type == 1 ? CrossSection::JoinType::Round
+                             : CrossSection::JoinType::Miter;
+  return cross_section.Offset(delta, jt, miter_limit, arc_tolerance);


Let's do this in a separate PR, but I agree with @pca006132 that instead of arc_tolerance we should use our setMinCircularAngle etc. functions to define this.

elalish · 2023-12-12T09:56:27Z

bindings/wasm/examples/worker.ts

+  const cls = module[className];
+  const obj = areStatic ? cls : cls.prototype;
+  for (const name of methodNames) {
+    if (name != 'cylinder') {


I must have missed this in code review - is this intentional? Just want to make sure I can safely remove it now.

hrgdavor · 2023-12-12T10:08:34Z

love the addition of slice :)

- add CrossSection class - move static method (constructors) under their respective classes -add Manifold.{split, splitByPlane} - enable usage of instanceof on Manifold (and now CrossSection) objects simplify registry indirection in worker.ts, enabling method de-structuring (opening static function names out of class module name-spaces) Build changes: - break C++ helpers out from bindings.cpp to helpers.cpp - ensure that ts files are copied during builds even when nothing is recompiled Note that in addition to the new bindings, the most of the existing toplevel functions have been moved under their respective classes as static methods (breaking change!). I haven't added any new examples yet, but the existing ones are passing and do make use of CrossSection now (some transparently, and some directly), but I think enough of this is together that review is worthwhile.

geoffder requested a review from elalish May 25, 2023 21:30

elalish requested changes May 26, 2023

View reviewed changes

bindings/wasm/examples/worker.ts Outdated Show resolved Hide resolved

bindings/wasm/examples/worker.ts Outdated Show resolved Hide resolved

bindings/wasm/manifold.d.ts Outdated Show resolved Hide resolved

bindings/wasm/helpers.cpp Show resolved Hide resolved

geoffder commented May 26, 2023

View reviewed changes

bindings/wasm/examples/vite.config.js Outdated Show resolved Hide resolved

bindings/wasm/examples/public/examples.js Show resolved Hide resolved

bindings/wasm/examples/worker.ts Outdated Show resolved Hide resolved

elalish requested changes May 26, 2023

View reviewed changes

elalish approved these changes May 27, 2023

View reviewed changes

bindings/wasm/examples/vite.config.js Outdated Show resolved Hide resolved

bindings/wasm/bindings.js Show resolved Hide resolved

elalish reviewed May 27, 2023

View reviewed changes

geoffder added 16 commits May 27, 2023 00:28

Update JS bindings with CrossSection + reorganize

69ebacd

- add CrossSection class - move static method (constructors) under their respective classes - add Manifold.{split, splitByPlane}

ignore no-arg call of Module

fdd6f56

roughly group functions names by purpose

47c674e

clang-format pass over js files

356c9bf

Move debug setup below memory registry indirection

7c25583

Re-reformat

8a9b062

Add back undefined check

623306b

remove spurious newline

39ed5d7

Move reserveIDs under Manifold

b06c89e

Add center param to extrude

c6b89f5

Add missing this argument back to _Extrude

2908a20

Fix wrong timeout key

8423e6e

rebuild on interface modification (ensure copying)

2df7829

Remove redundant checks in scale methods

bbd8ee1

Undo scale change (vararg2vec not redundant)

618836c

Fix bindings.cpp properties (js deps)

4301220

elalish requested changes May 30, 2023

View reviewed changes

Remove std::string from enum handling

97e4381

Undo flubbed bracelet change, update intro test

f0a518e

geoffder added 2 commits May 29, 2023 22:54

organizational comments

2dc2b21

Revert show/only changes

11bc0c9

geoffder added 3 commits May 30, 2023 11:38

Organizational comments

5b05c42

Fix vec to polygon conversion

1612ff4

add missing static to ofPolygons signature

507b844

elalish requested changes Jun 1, 2023

View reviewed changes

geoffder added 2 commits May 31, 2023 22:36

Revert to old intro and drop unneeded decimal pts

0a3dfe1

missed intro example test reversion

b4f2605

geoffder merged commit cbfeed1 into elalish:master Jun 1, 2023
21 checks passed

elalish mentioned this pull request Jun 1, 2023

ManifoldCAD docs update #446

Merged

geoffder deleted the update_js branch June 1, 2023 18:54

elalish mentioned this pull request Nov 3, 2023

V2.2.1 #589

Merged

elalish reviewed Dec 12, 2023

View reviewed changes

Update JS bindings with CrossSection + reorganize #440

Update JS bindings with CrossSection + reorganize #440

Conversation

geoffder commented May 25, 2023 • edited

codecov bot commented May 25, 2023 • edited

Codecov Report

geoffder commented May 25, 2023

elalish commented May 26, 2023

elalish left a comment

Choose a reason for hiding this comment

geoffder commented May 26, 2023

elalish commented May 26, 2023

elalish left a comment

Choose a reason for hiding this comment

geoffder commented May 26, 2023

elalish left a comment

Choose a reason for hiding this comment

geoffder commented May 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoffder commented May 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoffder commented May 30, 2023

elalish commented May 30, 2023

geoffder commented May 30, 2023

elalish commented May 30, 2023

geoffder commented May 30, 2023

pca006132 commented May 30, 2023

pca006132 commented May 30, 2023

geoffder commented May 30, 2023

pca006132 commented May 30, 2023

geoffder commented May 31, 2023

elalish commented May 31, 2023

geoffder commented May 31, 2023

elalish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hrgdavor commented Dec 12, 2023

geoffder commented May 25, 2023 •

edited

codecov bot commented May 25, 2023 •

edited