CascadeStudio icon indicating copy to clipboard operation
CascadeStudio copied to clipboard

[WIP] Upgrade to Embind Bindings

Open zalo opened this issue 3 years ago • 33 comments

THIS PR IS A WORK IN PROGRESS; SOME FUNCTIONALITY IS BROKEN

This branch-in-progress uses the newer embind version from the opencascade.js repo.

The Embind refactor has a number of Pros and Cons: Pros:

  • Upgrade OpenCASCADE from 7.4.0 -> 7.4.0p1
  • Better Error Messages (for the most part)
  • Vastly Expanded API Exposure (and support of Overloads)
  • *Eventually going to support extremely detailed Intellisense and documentation.

Cons:

  • The WASM is ~twice as large (64 megabytes vs. 34 megabytes); it takes twice as long to load.
  • Any function with an overload is split into a number of Function_X() variants which can be very annoying to manage (but it's not a deal-breaker with typescript definitions).
  • Default arguments are no longer supported; all arguments must be specified all the time and Handles require an additional .get() to dereference...
  • Typescript definitions preliminarily ballooning from 75Kb -> >16mb!
  • General function execution is noticeably slower (due to the increased wasm size?? Different compiler optimizations? 7.4.0p1?)
  • The Fuzzy Value option in Shape Booleans was removed! (in OCCT 7.4.0p1?)
  • Likewise, Shape Booleans now exhibit some unstable behavior (see master vs. embind)

Feel free to make PRs into this PR.

zalo avatar Sep 16 '20 23:09 zalo

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/zalo/cascade-studio/vJqXS1im1weuHDY2PvxAcfAvusWp
✅ Preview: https://cascade-studio-git-feat-embind-zalo.vercel.app

vercel[bot] avatar Sep 16 '20 23:09 vercel[bot]

Awesome :slightly_smiling_face:. Can't wait for this to be fully functional!

donalffons avatar Sep 28 '20 20:09 donalffons

@donalffons It's probably better to move iteration talk here rather than a closed/merged PR ( https://github.com/donalffons/opencascade.js/pull/8#issuecomment-699593769 ).

The good news: I seem to have resolved most of the major overload-based API-breakages, so the new Embind bindings are mostly operational within the IDE!

The bad news: There are a few regressions in the Embind system that make me hesitant to switch CascadeStudio fully to the new Embind branch (which I've now outlined in the OP of this PR).

My primary worries (in addition to the typescript definitions) are the expanded size of the binaries and the regressions in boolean operations in 7.4.0p1. CascadeStudio (which wasn't terribly fast, to begin with) now feels fairly sluggish.

A fair number of things changed in the Embind PR (the OCCT version (7.4.0->7.4.0p1), compiler (optimizations?), binding system, number of exposed functions, etc.). And I'd like to help you get to the bottom of this.

Suggestion 1:

If it's not too much trouble, a "lite" version of the bindings (which is restricted to: Standard, GProp, TopoDS, TopAbs, TopExp, BRepGProp, BRepAlgoAPI, BRepBuilderAPI, BRepFilletAPI, BRepMesh, BRepPrimAPI, GC, STEPControl, XSControl, Poly, TColgp, StdPrs, IGESControl, Bnd, BRepBndLib, gp, StlAPI, GeomAPI, Geom, Geom2d, GCE2d, BRepLib, BRepOffsetAPI, ShapeFix, ShapeUpgrade, BRepAdaptor, GCPnts, Poly, and BRepPrimAPI) would still fully cover the CascadeStudio/MakeBottle use-case, while (I believe) significantly shrinking the output binaries (you would know better, however...). If it's small enough (under 50mb?), it might even support Github Actions building again!

Perhaps even a flag in the build script that distinguishes between "lite" and "full" versions of the library, so it can be switched at compile-time by the "user"!

Suggestion 2:

Revert back to the older OCCT commit (for now). The biggest modeling "feature highlights" for 7.3.0 are actually regressions when tested in the editor (see the last bullet point of the first post for the live model links).

This gif of a Union() operation switches between the pre-embind (7.4.0) and post-embind (7.4.0p1) bindings: BadCSG

The face around the cylinder appears to be malformed in the 7.4.0p1 version.

zalo avatar Sep 28 '20 22:09 zalo

I appreciate you sharing your thoughts and concerns...

OCCT Versions

To avoid confusion:

  • Currently, OpenCascade.js is using OCCT version 7.4.0p1.
  • The version used during most of the WebIDL-days was OCCT version 7.4.0 (that's the version your fork of the library is using). Release Notes.
  • OCCT versions 7.3.0 and 7.1.0 have not been used by OpenCascade.js.

New "lite"-builds

  1. I created a lite version of the library in the branch named lite-occtv7.4.0p1. This is using the latest OCCT version 7.4.0p1 (latest stable). That build works fine - feel free to test it. The WASM file size is 47M.
  2. I started working on another branch named lite-occtv7.3.0p3. This branch is using the OCCT version 7.3.0p3. I just realized now, that this version is actually quite old and has not been used by this project (also not in the WebIDL days). That build is currently having some issues. If you're interested in testing this, let me know. If you're not interested, I will remove the branch after a while.
  3. I just created a new branch named lite-occtv7.4.0. This is using OCCT version 7.4.0, which is the version that was used in most of the 0.x versions of OpenCascade.js. The build is running right now.

Binary size, typescript, boolean

My primary worries (in addition to the typescript definitions) are the expanded size of the binaries and the regressions in boolean operations in 7.3.0. CascadeStudio (which wasn't terribly fast, to begin with) now feels fairly sluggish.

Binary size The size of the binaries is clearly too large for most web-apps at the moment. As you know, I would like to split the library up into modules and load them as dynamic libraries. This will however take some time until this is implemented. Until this is done, I would follow your suggestion to create customized builds by slightly modifying the autobind.py script. The mechanism I have in mind for this, is to modify the processIncludeFile, processClass, processMethod, processTypedef and processEnum functions. Those functions return True if a specified entity should be processed (i.e. binding code should be generated) or else False. For the "lite" version of the library, I modified the processClass function of the library like this. You can certainly shave off much more from the binary size by being even more specific on which entities should be exported. (Side note regarding binary sizes of WASM files: WASM files tend to get really small after gzipping them (~75% reduction in size). I think that WASM files (like most assets) are also gzipped by most servers before being shipped. Therefore, I think the file size is not quite as bad as it looks)

Typescript definitions Implement this will take a while, but I think the new typescript definitions will be quite helpful. I made some progress with adding comments today (probably they're currently all in the wrong spot :wink: )...

Boolean regressions Yeah, this is pretty weird. I also experience the same "sluggish" performance using the new version of the library in your demo. The artifacts after the union operation are definitely a problem. It would be interesting to compare Embind and WebIDL version of the same OCCT version of the library. I do hope that there is no performance penalty from using Embind (I think there shouldn't be one, but who knows). I'm curious to see if the artifacts disappear. Maybe you can modify your demo to use the new builds and see if that changes anything?

donalffons avatar Sep 29 '20 20:09 donalffons

compiler (optimizations?) I did upgrade the Emscripten version to 2.0.4 from 1.39.20 in your fork. Optimization settings seem to be identical. Before doing any benchmarking, we might want to make sure that the same value for TOTAL_MEMORY is used. In my understanding, this is the initial size of the memory.

edit: I will do some benchmarking on the bottle example and let you know if I notice a difference between Embind and WebIDL.

donalffons avatar Sep 29 '20 21:09 donalffons

Apologies for confusing the OCCT versions, I was mostly just guessing due to the available documentation links I tried and correlating the release notes. Thank you for clarifying that!

And thank you for being so quick with the "lite" versions of the libraries! That's a very clean solution and I will definitely try these ASAP over the next couple of days.

Regarding the speed of loading, I recently learned that Vercel can have extraordinarily high "time-to-first-byte" times on the requests and it uses br (Brotli) as its compression scheme rather than gzip, so those may also be contributing to the increased time. Who knows if streaming WASM compilation is supported in Brotli yet...

Those comments are coming along really nicely! They look like they're in the right place at least... though it may be necessary to duplicate them for the overloads :( (Luckily, that should compress well too!)

I'll make some forks hooking into these various builds and get back to you on the relative performance/behavior. It's super weird to me that the SetFuzzyValue option in the Boolean Ops class would have been removed between 7.4.0 and 7.4.0p1.

zalo avatar Sep 29 '20 23:09 zalo

...I just had another look at the SetFuzzyValue issue. That method is still present in v7.4.0p1. Its my fault that it is currently not exported (because of course it is :slightly_smiling_face:).

  • That method is inherited as a protected method from BOPAlgo_Options over multiple levels to all the BRepAlgoAPI Cut, Fuse, etc. classes.
  • That method is made public in v7.4.0 and in v7.4.0p1 in the BRepAlgoAPI_Algo class with this using ... syntax.
  • The auto-binding script is currently not clever enough to understand the using ... syntax. Therefore, the method is considered non-public and therefore no bindings are created.

It should probably be quite easy to export that method in the auto-binding script :crossed_fingers:. I will look into fixing this by the end of the week.

Super happy that we caught this limitation of the auto-binding script (I'm sure it will not be the last :wink:).

donalffons avatar Sep 30 '20 08:09 donalffons

I think I fixed the SetFuzzyValue - issue. The additional methods are all exposed, calling SetFuzzyValue works without issues and the bottle example is also still running fine. Could you test the new builds in the branch bind-using?! These are currently non-lite builds. After your feedback, I will merge the changes into master and the lite branches.

donalffons avatar Oct 01 '20 14:10 donalffons

Perfect; thank you, @donalffons!

I’ll be sure to test them both soon!
(Apologies, it’s been a bit of a busy work week)

zalo avatar Oct 01 '20 17:10 zalo

@donalffons SetFuzzyValue() works with the WASM from the bind-using branch (🥳 ), but the odd regression in the CSG remains :-(

And it looks like I forgot a bunch of classes (most crucially TopLoc_Location) from the lite builds too (sorry!)

Here are the classes(?) that I think are missing: TopLoc, BRepPrim, TopTools, Abs, BRep, TColStd, DOMString, IFSelect, Message, ShapeExtend, BRepFill, and ChFi2d.

zalo avatar Oct 02 '20 01:10 zalo

I pushed new builds to the lite branches with the changes you requested. The lite-v7.4.0 and lite-v7.4.0p1 look fine. The lite-v7.3.0p3 seems to have issues. However, I don't think we have a problem with the OCCT version anyways, so checking the older versions is maybe not so relevant...

By the way, DOMString is nothing that is defined in OCCT. This could be an Emscripten-defined object, but probably you can just use a normal JavaScript string instead of that object?!

Also, the problem is starting to look weird... Locally, I made a small modification to OpenCascade, by adding this method to BrepAlgoAPI_Fuse:

#include <iostream>
void BRepAlgoAPI_Fuse::test() {
  std::cout << "myFuzzyValue" << std::endl;
  std::cout << myFuzzyValue << std::endl;
}

Here is what I see in JavaScript, when I compare the ouputs of the FuzzyValue() and test() methods: image

That doesn't look right... And probably the fuzzy value, you set is never actually used by the algorithm.

This does look like a bug/weirdness in Emscripten. But the binding-code could also be to blame... I will continue looking into this.

donalffons avatar Oct 03 '20 15:10 donalffons

That looks like it! This whole time (wrt to Fuzz values), I've only been trying 0.1 and 0; I'd never considered that the default was actually 1e-07. When 1e-07 is manually fed into the non-embind branch, it exhibits the same CSG behavior that the embind branches do.

Thank you for pointing that out!

zalo avatar Oct 04 '20 03:10 zalo

... I think what this issue boils down to is that C++ allows a derived class to have multiple base classes and JavaScript doesn't. It also looks like Emscripten has no workaround for that situation and I don't think I could implement one easily.

Anyways, for this particular problem, the easiest solution is (in my opinion) to replace the using syntax with "proper" method definitions. This seems to work nicely and I think (and hope) that this should not have any side effects. There are a couple of other places, where this kind of using syntax is used. I put those on my to-do list and will look at them later.

@zalo, if you want, you can use the lite branch from now on. I will delete all other lite branches and the bind-using branch soon...

donalffons avatar Oct 06 '20 10:10 donalffons

I don't have much to add besides the idea of dynamic modules ideas sounds like a really solid way to clamp down on the asset size @donalffons, Might add some complexity in figuring out which modules need to be loaded in the UI but I think would be worth it.

I can confirm with the Gziping point, the current wasm file come through at about 7mb

Irev-Dev avatar Jan 21 '21 06:01 Irev-Dev

I don't have much to add besides the idea of dynamic modules ideas sounds like a really solid way to clamp down on the asset size

The dynamic modules "solution" improves the loading times a little bit, but unfortunately the loading are still very long and memory consumption is very high. There seem to be two contributing factors to those issues - download times aside:

  1. Amount of bindings: Bindings are generated for classes, enums, template typedef's, etc. Emscripten requires significant time in post_instantiate to load those.
  2. Amound of WASM code: The WASM code is compiled by the browser during WASM instantiation relatively fast.

My current plan is this:

  • For development and prototyping with opencascade.js, I think it makes sense to have the "full" library available with all bindings - which comes with some significant loading times. For this use case, I want to publish a modularized version of the library. But it will be possible to use all working parts of the library.
  • For production builds, I want to provide a simplified interface for creating custom builds, using YAML-files. In those YAML-files, you can specify which code should be included and which bindings should be generated.

Will keep you updated. I hope, this will improve the situation with the next release of opencascade.js.

donalffons avatar Jan 21 '21 21:01 donalffons

Hi @zalo and @Irev-Dev! It's been a while... And it has been a bit of bumpy ride for opencascade.js, as I have tried out several approaches, some of which didn't turn out to be great. Esspecially dynamic modules...

Anyhow, I do have a version of the library now, which has the capability to be "production ready" again. It's not fully ready yet (and some parts aren't yet pushed to the git repository), but will be over the next few weeks or so.

Some of the important new features / changes include:

  • I will dump the approach of using dynamic modules since they make everything worse and improve nothing.
  • The build system can build a "full" version of the library efficiently, now. I will ship a full version through NPM. A full version comes in at around 45MB. (12.5MB gzipped) Both download and startup times will be reasonably quick. Memory consumption will be lower (compared to the version using dynamic modules).
  • There will be support for APIs that use references to built-in types.
  • There is a relatively simple way of creating custom builds using a Docker image, if you don't want the capabilities of the full library. Creating even the largest custom builds will be quick and in most cases take less than 5 minutes.
  • When creating a custom build, it will be possible to add custom C++ code. That's the back door, in case the library is missing some critical features.
  • Not using dynamic modules will allow for enabling multithreading, e.g. for tessellation. My tests are promising that this can be included without causing issues.

Just wanted to let you know that there will be something coming soon(ish)...

donalffons avatar Jun 28 '21 11:06 donalffons

Thank you @donalffons ! Exciting to hear about faster builds, built-in type references and custom code injection.

It will be nice to not have to use workarounds like generating U and V isolines to get the UV Bounds of each face.

I also have a new (cleaner, ESBuild-based) opencascade.js -based project coming down the pipe that may be able to take advantage of this as well.

P. S. Have you seen Matt Keeter's .STEP file parser?

zalo avatar Jun 28 '21 20:06 zalo

I also have a new (cleaner, ESBuild-based) opencascade.js -based project [...]

Very excited to hear that :slightly_smiling_face:. Would be happy to hear more when you're ready to share.

I haven't heard about that STEP file parser. That blog post looks interesting, the fact that its written in Rust is super nice. I will read up on that - thanks for sharing. I've thrown some STEP files at it and it was blazingly fast - although not always accurate and for some files it didn't show any results.

Another note: I have worked on a small proof-of-concept of a STEP file viewer, trying to use some of the more "advanced" features of opencascade.js:

  • It uses multithreading for tessellation
  • It can handle relatively large STEP files. I had it working with files up to ~150MB, after that I tend to get Out-Of-Memory errors, presumably (but not verified) because Emscripten is hitting the 4GB WASM limit.
  • It uses a custom build and therefore loads fast
  • It has a progress indicator. Which probably sounds like a small thing, but was a bit tricky since it involves reading the value of some raw pointers in WASM's ArrayBuffer.
  • The tessellation process can be cancelled (by hovering your mouse over the progress indicator). Also tricky, because I need to write into WASM's ArrayBuffer.
  • I'm exporting to GLTF from within OpenCascade. Then I'm using donmccurdy GLTF-SDK to post-process the GLTF to make them more efficient to render (improving the frame rate by an order of magnitude for certain files). That GLTF file is then read using ThreeJS.

It's not pretty - just a tech demo for the moment. Once things get a bit more stable, I want to share (parts of) the code of that viewer. (in case you want to have access to that currently private project, let me know. it's relatively straightforward, I think).

donalffons avatar Jun 28 '21 21:06 donalffons

Exciting to hear about faster builds

To be precise... The total build time is still quite long and probably more than 10 hours on my machine.

What's new and improved is:

  • The build system prepares a lot of stuff upfront, so that the final build step (creating a custom build) takes only a few minutes.
  • I want to put as much as possible of this "upfront" stuff into the docker build phase of the Docker image, i.e. it will be published to Dockerhub.
  • The build system now runs multi-threaded most of the time. So it would be possible to speed up build times significantly by throwing more compute at it.

donalffons avatar Jun 28 '21 21:06 donalffons

A new build of the "full" version has been published as opencascade.js@beta, the docs and Docker image also have been updated. The examples are now using that new build and therefore load significantly faster. Performance in general is much better and might improve even more when switching to multithreaded tessellation (needs more testing first).

2021-07-04 08-59-46

@zalo, are you considering to update to the new version in CascadeStudio? As you know, this would be a breaking change for your project, due to some API changes.

donalffons avatar Jul 04 '21 07:07 donalffons

That gif is it re-tessellating on the fly? That's awesome.

Irev-Dev avatar Jul 04 '21 07:07 Irev-Dev

Yes, you can try it here.

donalffons avatar Jul 04 '21 07:07 donalffons

@donalffons Wow! I can hardly believe it's regenerating that whole model with every refresh! It looks quite fast!

I spent some time today porting this into my new project (still not public yet, unfortunately) and I have a few questions:

  • Is there an equivalent or alternative way to SetFuzzyValue() on the Fuse and Cut CSG operations?
  • Is there an equivalent or alternative way to call new this.oc.TopoDS_Shape(someShape);?

The typescript definitions have been utterly invaluable for updating my module-based project; I'm hoping that they'll be similarly helpful for CascadeStudio :-)

zalo avatar Jul 07 '21 02:07 zalo

Just updated this branch to your new beta as well.

You should even be able to see the new library's intellisense (under the oc. namespace). It will be super cool to get the library's own comments in there too; let me know if I can help with that.

Some operations are a little more brittle without the fuzzy values and tactical new this.oc.TopoDS_Shape(); (which I've been using to dissociate shapes from the operations that form them), but otherwise things seem to be working well.

zalo avatar Jul 07 '21 03:07 zalo

Is there an equivalent or alternative way to SetFuzzyValue() on the Fuse and Cut CSG operations?

This doesn't work?! That probably means that the patches are not applied properly (once again). I will look into that.

Is there an equivalent or alternative way to call new this.oc.TopoDS_Shape(someShape);?

I guess you probably want to use this move constructor?! That's not supported bindings-auto-generator. Since this is a templated method, I would have to generate one binding for every possible value of that type parameter T2, i.e. every class derived from TopoDS_Shape in this case. I think implementing this in the bindings system (in a generalized way) would be insanely complex. Maybe instead, you could create a custom build using additionalCppCode and then create a derived class from TopoDS_Shape, which has the specific move constructor(s) that you need? I'm happy to help with that if you (or the build system) get stuck somewhere.

(The additionalCppCode stuff is broken in the current Docker Image, but will hopefully be fixed in the next beta release - build is running and will be published in ~2 hours or so)

It will be super cool to get the library's own comments in there too; let me know if I can help with that.

Help would be much appreciated, of course. It shouldn't be too complicated to do this. I added a ticket for that, too and just posted some more detailed information there. Let me know if you want to give this a shot. If not, I will do it (eventually :wink:).

donalffons avatar Jul 07 '21 09:07 donalffons

I (think I) finally fixed the issues with additionalCppCode just now, if you want to try that. I will focus on adding testing back in, so that these kinds of issues can hopefully be caught before releasing anything.

donalffons avatar Jul 07 '21 14:07 donalffons

...just a note: I just managed to get the multi-threaded build running with the bottle example. Besides the fact that it's quite messy to set up (loading a "normal" js module, a web worker and a wasm file and making sure that they all know about each other), there is very little difference in performance in this case. In this test, multi-threading is only applied during the tessellation phase.

In single-threaded mode, each call to addShapeToScene (i.e. construction, tessellation, add to ThreeJS scene graph) takes ~50 ms. In multi-threaded mode, that call takes ~40-50 ms.

I think in this particular case, the tessellation is just not the real bottleneck.

donalffons avatar Jul 07 '21 23:07 donalffons

Hopefully the WASM -> Worker -> Main thread communication overhead isn't too high; I know workers like to serialize everything to JSON and back unless SharedArrayBuffers are used. I have seen what I think is tessellation take longer on some of the more complex .STEP files I've loaded in; perhaps it will yield a stronger signal?

zalo avatar Jul 07 '21 23:07 zalo

There's probably quite a bit of overhead involved with threading, since that's not natively supported in WASM (yet) and Emscripten basically does a work-around using web workers. I would expect this to use quite a bit more memory, too.

After your comment, I made another quick test passing in a new WebAssembly.Memory with shared: true to Emscripten's Module object, which creates a SharedArrayBuffer under the hood. The results were pretty much the same. Not sure what that means, though... Maybe Emscripten uses that approach by default in multi-threaded mode? I wasn't able to check that.

By the way, none of the currently released builds have multi-threading enabled. There's currently just this build (in the build artifacts) where I tested it. Enabling that feature requires to make an entire build from scratch (i.e. rebuilding the Docker Image with the correct flags), as the intermediate build products (.o files) are incompatible between single- and multi-threaded versions. Not sure, but I think I might leave multi-threading out (at least as a "released feature") for a future version of the library, as this might make things much more complex for users of the library(?!).

donalffons avatar Jul 07 '21 23:07 donalffons

Given the limitations that Safari often exhibits around webworkers (and the minimal improvements) it's probably better to leave it out for now to reduce headaches; as far as CascadeStudio goes, I'd probably keep it disabled if there was any instability at all anyways.

zalo avatar Jul 07 '21 23:07 zalo