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

[WIP] Upgrade to Embind Bindings #12

Open
wants to merge 23 commits into
base: feat-typescript
Choose a base branch
from
Open

[WIP] Upgrade to Embind Bindings #12

wants to merge 23 commits into from

Conversation

zalo
Copy link
Owner

@zalo zalo commented Sep 16, 2020

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.

This branch-in-progress grabs [the latest `embind` version from the opencascade.js repo](https://github.com/donalffons/opencascade.js/tree/embind).

Thes new bindings will support much better error message reporting and detailed intellisense and typescript documentation.

The bindings and WASM are mirrored through githack to bypass CORS, but it may add a small (couple minute) delay to changes.
@vercel
Copy link

vercel bot commented Sep 16, 2020

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

@zalo zalo added the documentation Improvements or additions to documentation label Sep 18, 2020
@donalffons
Copy link

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

@zalo
Copy link
Owner Author

zalo commented Sep 28, 2020

@donalffons It's probably better to move iteration talk here rather than a closed/merged PR ( donalffons/opencascade.js#8 (comment) ).

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.

@donalffons
Copy link

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 😉 )...

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
Copy link

donalffons commented Jul 4, 2021

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.

@Irev-Dev
Copy link

Irev-Dev commented Jul 4, 2021

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

@donalffons
Copy link

Yes, you can try it here.

@zalo
Copy link
Owner Author

zalo commented Jul 7, 2021

@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 zalo changed the base branch from master to feat-typescript July 7, 2021 02:47
@zalo
Copy link
Owner Author

zalo commented Jul 7, 2021

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.

@donalffons
Copy link

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 😉).

@donalffons
Copy link

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
Copy link

donalffons commented Jul 7, 2021

...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.

@zalo
Copy link
Owner Author

zalo commented Jul 7, 2021

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?

@donalffons
Copy link

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(?!).

@zalo
Copy link
Owner Author

zalo commented Jul 7, 2021

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.

@donalffons
Copy link

The fact that SetFuzzyValue was not available was due to me not having updated this path for the patch files. This should be fixed in the latest @beta release.

Why is Quantity_AbsorbedDose used in-place of Standard_Real?
@zalo
Copy link
Owner Author

zalo commented Jul 12, 2021

Thank you for taking a look at that! That does indeed restore the SetFuzzyValue() Functionality 😎

I noticed the latest beta also has some funniness with the types going on; it seems like Standard_Real has been replaced with Quantity_AbsorbedDose in most places (among other things). Was that intended?

@donalffons
Copy link

...that's certainly not intended. I will have a look and correct that behavior. Thanks for letting me know!

@zalo zalo mentioned this pull request Oct 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants