Open zalo opened 3 years ago
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
Awesome :slightly_smiling_face:. Can't wait for this to be fully functional!
@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.
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"!
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:
The face around the cylinder appears to be malformed in the 7.4.0p1 version.
I appreciate you sharing your thoughts and concerns...
To avoid confusion:
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?
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.
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
.
...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:).
protected
method from BOPAlgo_Options
over multiple levels to all the BRepAlgoAPI
Cut
, Fuse
, etc. classes.public
in v7.4.0
and in v7.4.0p1
in the BRepAlgoAPI_Algo
class with this using ...
syntax.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:).
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.
Perfect; thank you, @donalffons!
I’ll be sure to test them both soon!
(Apologies, it’s been a bit of a busy work week)
@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
.
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:
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.
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!
... 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...
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
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:
post_instantiate
to load those.My current plan is this:
Will keep you updated. I hope, this will improve the situation with the next release of opencascade.js.
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:
Just wanted to let you know that there will be something coming soon(ish)...
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.
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'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).
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:
docker build
phase of the Docker image, i.e. it will be published to Dockerhub.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).
@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.
That gif is it re-tessellating on the fly? That's awesome.
Yes, you can try it here.
@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:
SetFuzzyValue()
on the Fuse and Cut CSG operations?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 :-)
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.
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:).
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.
...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.
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?
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(?!).
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.
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.
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?
...that's certainly not intended. I will have a look and correct that behavior. Thanks for letting me know!
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:
Cons:
Function_X()
variants which can be very annoying to manage (but it's not a deal-breaker with typescript definitions)..get()
to dereference...Feel free to make PRs into this PR.