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

Use Embind instead of WebIDL #8

Merged
merged 339 commits into from Sep 24, 2020
Merged

Use Embind instead of WebIDL #8

merged 339 commits into from Sep 24, 2020

Conversation

donalffons
Copy link
Owner

@donalffons donalffons commented Aug 25, 2020

This pull request will implement the bindings to OCCT using Emscripten's Embind instead of the current WebIDL approach.

The advantages:

  • All overloaded functions and constructors can be exposed. You don't have to decide which overloads you want to use and / or recompile the library.
  • All operators can be exposed.
  • The binding code is a bit cleaner and less verbose than with the WebIDL approach (at least for non-overloaded functions).
  • It seems like Emscripten offers better error messages now (but that could also be due to an Emscripten update I made).

Code conventions / How-To:

  • I wrote down a few ideas on how I would organize and name things (like overloaded functions, overloaded constructors, operators) in this document.
  • Look inside the embind folder. Start with main.cpp and then look at the header files in that folder.

Current limitations:

  • Only a few classes have been exposed so far. However, I wanted to have this pull-request here, so that there is a place to discuss (if there is any need for that...).
  • I could not find any solution to generate Typescript types using the Embind workflow. This is a huge drawback at the moment. Currently, I see following options on how to resolve that.
    1. Write the typescript definitions by hand (ew...)
    2. Use something like cppast to analyze the OCCT source files and auto-generate typescript bindings from those files. If there is the need for manual adjustments after this step, we might be able to use patch-files?!
    3. Wait until someone else creates a method to generate types from Embind definitions

Other improvements:

  • Updated Emscripten
  • Updated OCCT
  • Updated Python
  • Actions push to the respective branch, now

Next steps:

  1. Continue to expose all APIs required for running the bottle example and make that work with the new system
  2. Expose everything that is currently implemented using WebIDL (also from @zalo's branch)
  3. Merge

Any comments would be welcome, particularly from @zalo, @guidovanhilst, @ancorasir, @Johnly1986 (because you guys have actually already used this library 🙂)

@zalo
Copy link
Contributor

zalo commented Aug 25, 2020

With this technique, would it eventually be possible to expose the entire OpenCascade API... automatically?

If not, this might be a heretical idea, but would it make sense to use the WebIDL to generate the intermediate embind exposure interface (the same way that the typescript is generated)? In that way, you get the benefit of all of the already exposed WebIDL, the typescript definitions, AND you get to keep the function overloading...

Also, the informative error messages look really exciting!

@donalffons
Copy link
Owner Author

Yeah, I have been playing around with code-generating the bindings a bit today. First, I looked into cppast and doxygen but did not have great success with both. Then I came accross CppHeaderParser and wrote a small test script. Some known issues still remain (for example), but I'm carefully optimistic that this tool might be able to code-generate the bindings automatically. I will do more testing with it and share the results...

If the auto-generation script works, I can have it export Embind and WebIDL bindings and then use the WebIDL file to generate the Typescript definitions. I think that would be pretty neat...

Your suggestion of using WebIDL as the input for the bindings might also work... Probably, I would have to come up with some additional definitions in the IDL file to mark overloaded methods and constructors, so that the Embind bindings and the Typescript definitions can be created correctly from that file.

I'll post here, if I have some updates.

@zalo
Copy link
Contributor

zalo commented Sep 23, 2020

Have you considered uncommenting the if-always under "Upload Build Products" to see if they're good?

It's also possible to append a timeout for the step like so:

      # Build the docker container
      - name: Build OpenCascade.js Bindings
        timeout-minutes: 200
        run: |
          docker run --init \
          -v "$(pwd)/build/":"/opencascade/build/" \
          -v "$(pwd)/dist/":"/opencascade/dist/" \
          -v "$(pwd)/emscripten-cache/":"/emscripten/upstream/emscripten/cache/" \
          my_awesome_image

That should shave a couple hours off of the timeout step (timing out at an arbitrarily-chosen 3 hours and 20 minutes).

Not that it's a long-term solution, but it might help with progress...

@donalffons donalffons merged commit bedcd7d into master Sep 24, 2020
@donalffons
Copy link
Owner Author

Hi @zalo, I just merged this into the master branch and created a release tagged with v1.0.0. This release is also available on npm.

I gave up on most of the build system, for the moment. As mentioned before, I think (hope) the build system will come back to life when I split up the library into smaller chunks (which can be loaded on demand, allowing for smaller web apps).

I'm sure that there will still be bugs in the library and I would greatly appreciate it if you would report them if you find any.

Next up on my todo-list are typescript support and modularization. I think I want to start working on a basic version of the typescript-support first. Once that is working again, I want to look into modularization. Modularization might involve breaking changes, as this could mean that library then must run inside a web-worker (see here, for example) - except if there is a workaround - need to investigate this more closely. But probably that's not a big issue for CascadeStudio, as you're running OpenCascade inside a web-worker already.

@zalo
Copy link
Contributor

zalo commented Sep 24, 2020

Alrighty; I'll look into porting the standard library to use the new embind overload naming scheme; I'll let you know how it goes 👍

@zalo
Copy link
Contributor

zalo commented Sep 25, 2020

It doesn't look like inheritance is handled quite the same way... Is there any way to cast types between each other?

Pretty much every time I use BRepBuilderAPI_MakeEdge, I use it with a Handle_Geom_TrimmedCurve, which is a child class of Handle_Geom_Curve (what the _24th constructor expects). Can I cast TrimmedCurves to Curves? Looks like there's an extra .get() I have to append to the Handle for MakeEdge, but BRepBuilderAPI_MakeWire's .Add() function expects a wire when MakeEdge().Edge() just outputs a generic shape...

Also, could I trouble you to add the oc.BRep_Tool.PolygonOnTriangulation() function and TColgp_Array1OfPnt constructor? Low Res Model Outlines look funny again without them...

@donalffons
Copy link
Owner Author

donalffons commented Sep 25, 2020

I'll look into porting the standard library to use the new embind overload naming scheme; I'll let you know how it goes

Perfect. If you're making any progress regarding the automatically detecting the correct overload, let me know.
Do you think, I should merge the improved error messages that you worked on, into this project?

Also, could I trouble you to add the oc.BRep_Tool.PolygonOnTriangulation() function

That should be in the library already (maybe try out the v1.0.1 builds if it doesn't work in v1.0.0). The interface for static methods changed slightly.
EDIT: try openCascade.BRep_Tool.PolygonOnTriangulation_1, _2 and _3.

... and TColgp_Array1OfPnt constructor?

The build is running. Will let you know when it's done.

@donalffons
Copy link
Owner Author

New builds are up. Let me know if you need anything else!

@zalo
Copy link
Contributor

zalo commented Sep 25, 2020

Thanks! I must have missed the Triangulation one because my patch only works on Constructors right now...

Unfortunately, the error right after supplying the correct overload of PolygonOnTriangulation is that TColStd_Array1OfInteger is unbound when trying to get the nodes...

If it's not a big deal, it would be nice if TColStd_Array1OfReal and maybe even TopTools_ListOfShape were added too (if it's not already present).

@donalffons
Copy link
Owner Author

I just implemented bindings for most specializations of the NCollection_Array1 and NCollection_List template classes, i.e. everything that has *_Array1Of* or *_ListOf* in its name. The only exception being OpenGl_ListOfStructure, which Emscripten doesn't like for some reason.
Will post again when the builds have gone through.

@donalffons
Copy link
Owner Author

...uploaded new builds ✌️.

@donalffons donalffons deleted the embind branch September 26, 2020 20:54
@zalo
Copy link
Contributor

zalo commented Sep 26, 2020

It looks like there's an issue where the array got included twice :(

"BindingError: Cannot register type 'TColQuantity_Array1OfLength' twice
    at Object.<anonymous> (blob:http://127.0.0.1:5500/7df7cdaf-8d22-4c99-a558-6d311fa95836:11:86851)
    at new <anonymous> (blob:http://127.0.0.1:5500/7df7cdaf-8d22-4c99-a558-6d311fa95836:11:86668)
    at throwBindingError (blob:http://127.0.0.1:5500/7df7cdaf-8d22-4c99-a558-6d311fa95836:11:87292)
    at registerType (blob:http://127.0.0.1:5500/7df7cdaf-8d22-4c99-a558-6d311fa95836:11:88806)
    at onComplete (blob:http://127.0.0.1:5500/7df7cdaf-8d22-4c99-a558-6d311fa95836:11:87792)
    at whenDependentTypesAreResolved (blob:http://127.0.0.1:5500/7df7cdaf-8d22-4c99-a558-6d311fa95836:11:88352)
    at __embind_register_class (blob:http://127.0.0.1:5500/7df7cdaf-8d22-4c99-a558-6d311fa95836:11:104447)
    at <anonymous>:wasm-function[83378]:0x35abbc2
    at Bo (<anonymous>:wasm-function[83379]:0x35c38f9)
    at Module.___wasm_call_ctors (blob:http://127.0.0.1:5500/7df7cdaf-8d22-4c99-a558-6d311fa95836:23:89833)"

It would be cool to run MakeBottle as a separate Github Action unit test (triggered on each commit etc.)

@zalo
Copy link
Contributor

zalo commented Sep 26, 2020

(oops, sorry, Github formatting clobbered the actual error before, it's been edited to include it now)

@donalffons
Copy link
Owner Author

donalffons commented Sep 26, 2020

Thanks for that. Yeah, I agree that it would be good to have testing. I will put that on my list...

I'm running another build which will hopefully work better.

@donalffons
Copy link
Owner Author

Can you try the new builds? Bottle example is working fine, here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants