v0.8.0 Taxonomy and Tessellation

[Krande]

Hey @aothms!

The performance improvements in your twitter post regarding using CGAL for tessellating geometry in IfcOpenShell are quite impressive and I am eager to test it out for myself :)

For those who did not see it, here is a figure from the post (hope it's okay I share it here as well?) comparing performance between OPENCASCADE and CGAL:

Is the v0.8.0 branch in such a state that we can start to build a daily conda package for testing you think?

[aothms]

Definitely. It also shouldn't be hugely complex to get it working. The approach is still mostly the same :) let me know how it goes and where I can help out!

[Krande]

I am seeing progress on the conda daily builds! I have successfully gotten to the compilation stage for all 3 platforms now actions.

Is my understanding of the taxonomy correct in assuming it is a "neutral" geometry description based on the different geometries in the IFC standard (ie. taxonomy acts as some kind of geometry-adapter between the IFC standard and the tessellation libs).

Would it be possible to instantiate the taxonomy objects directly from c++ and batch-tessallate a list of geometries without instantiating any IFC objects?

I see a lot of use-cases where I just want to visualize (ie. tessellate) a bunch of geometries really fast and was thinking I could try to solve this in c++ and hopefully by using the taxonomy geometry lib you have created

[aothms]

Cool! That's great news.

Is my understanding of the taxonomy correct in assuming it is a "neutral" geometry description based on the different geometries in the IFC standard (ie. taxonomy acts as some kind of geometry-adapter between the IFC standard and the tessellation libs).

Correct.

Would it be possible to instantiate the taxonomy objects directly from c++ and batch-tessallate a list of geometries without instantiating any IFC objects?

In theory yes. It's probably not the most convenient to use still. (I'm still debating things like smart pointers vs naked pointers and various other things). Also the API is probably a bit confusing in terms of how to get from items to evaluated geometry (have a look in Converter.h|cpp.).

Let me know how it goes. I'm quite enthusiastic about the idea of giving this taxonomy a more central place and more ease of use.

[Krande]

Okay, the compilation is almost at 100%.

It seems there is an issue with ifc-to-cityjson referencing unrecognized CGAL module "parameters" (https://github.com/IfcOpenShell/ifc-to-cityjson/blob/master/radius_execution_context.cpp#L945).

CGAL has changed the names since this was written. It is a easy fix though

This was okay in CGAL 4.14.3 but not in CGAL 5.5.2

CGAL::Polygon_mesh_processing::parameters

This is fine in CGAL 5.5.2

CGAL::parameters

For now I will try to solve this by adding a CITYJSON_SUPPORT option and setting it by default to OFF.

[aothms]

Or maybe try CGAL 5.3 that's what the build script uses https://github.com/IfcOpenShell/IfcOpenShell/blob/v0.8.0/nix/build-all.py#L92

[Krande]

Can confirm CGAL 5.3 works locally for cityjson as is. Have started github actions using cgal 5.3 with cityjson and 5.5.2 without cityjson.

FYI: It also seems that CGAL::parameters is allowed in CGAL 5.3.

[Krande]

Hm,

looks like linking IfcConvert fails on all platforms with CGAL 5.5.2. See failed steps for linux and windows respectively.

https://github.com/Krande/IfcOpenShell/actions/runs/5472847975/jobs/9965532381#step:6:23674
https://github.com/Krande/IfcOpenShell/actions/runs/5472847975/jobs/9965532245#step:6:14444

From windows

[1001/1008] Linking CXX executable IfcConvert.exe
FAILED: IfcConvert.exe 
cmd.exe /C "cd . && %BUILD_PREFIX%\Library\bin\cmake.exe -E vs_link_exe --intdir=CMakeFiles\IfcConvert.dir --rc=C:\PROGRA~2\WI3CF2~1\10\bin\100226~1.0\x64\rc.exe --mt=C:\PROGRA~2\WI3CF2~1\10\bin\100226~1.0\x64\mt.exe --manifests  -- C:\PROGRA~1\MICROS~2\2022\ENTERP~1\VC\Tools\MSVC\1436~1.325\bin\Hostx64\x64\link.exe /nologo @CMakeFiles\IfcConvert.rsp  /out:IfcConvert.exe /implib:IfcConvert.lib /pdb:IfcConvert.pdb /version:0.0 /machine:x64 /FORCE:MULTIPLE /INCREMENTAL:NO /subsystem:console  && cd ."
LINK: command "C:\PROGRA~1\MICROS~2\2022\ENTERP~1\VC\Tools\MSVC\1436~1.325\bin\Hostx64\x64\link.exe /nologo @CMakeFiles\IfcConvert.rsp /out:IfcConvert.exe /implib:IfcConvert.lib /pdb:IfcConvert.pdb /version:0.0 /machine:x64 /FORCE:MULTIPLE /INCREMENTAL:NO /subsystem:console /MANIFEST /MANIFESTFILE:IfcConvert.exe.manifest" failed (exit code 1120) with the following output:
LINK : warning LNK4044: unrecognized option '/lpthreads'; ignored

IfcConvert.cpp.obj : error LNK2019: unresolved external symbol "void __cdecl perform(struct geobim_settings)" (?perform@@YAXUgeobim_settings@@@Z) referenced in function wmain

IfcConvert.exe : fatal error LNK1120: 1 unresolved externals

Any suggestions? :)

[aothms]

unresolved external symbol "void __cdecl perform(struct geobim_settings)"

My guess is that this is simply the result of disabling this functionality, but still referencing it in IfcConvert.cpp

[Krande]

Ah, I see. Hopefully the build with cgal 5.3 and city json On will fix that!

[Krande]

Minor update!

Linux and Windows builds are now passing.

It seems osx is segfaulting when trying to import the python package link.

FYI: Just out of curiosity I've started experimenting with a minimal python wrapper with the intent of wrapping the taxonomy and tessellation capabilities only here.

[aothms]

It seems osx is segfaulting when trying to import the python package link.

It's likely because this is missing:

https://github.com/IfcOpenShell/IfcOpenShell/blob/v0.7.0/conda/build.sh#L9-L13

Which again triggers some behaviour in CMake:

https://github.com/IfcOpenShell/IfcOpenShell/blob/v0.7.0/src/ifcwrap/CMakeLists.txt#L57-L59

Causing not to link to python at compile time, but rather inherit the symbols at runtime so that some shared state is properly taken into account.

FYI: Just out of curiosity I've started experimenting with a minimal python wrapper with the intent of wrapping the taxonomy and tessellation capabilities only here.

That's awesome! What's your experience so far of using nanobind?

[Krande]

Ah, I remember removing those flags earlier because I didn't really know what they do (hoping I'll run into something later that forced me to bring them back, but I forgot it it seems) :) Hopefully this fixes things on the osx front! I've started a new batch of tests now!

So far my experience with combining Nanobind & scikit-build-core has been good! Keep in mind that my experience with SWIG and pybind11 is limited and I haven't tested nanobind on any really complex projects yet.

From what I can tell Nanobind seems to be relatively well maintained (changelog) and the docs have some simple "hello world" examples that got me started fast without any hassle. I also really liked that the combo made it possible to use only a single pyproject.toml file (instead of relying on a setup.py file) and just do pip install . to compile my c++ package and python wrapper and install it in my active python environenment.

For now the biggest obstacle might be that the project is still early in development, so a lot of the more complex features are not documented/easy to find. To me it seems like most of the undocumented features are basically ports of how pybind11 did things. So I find myself jumping to the pybind11 docs from time to time.

The background for choosing Nanobind and scikit-build-core was kind of random, but it just seemed like a natural choice when I wanted to create a limited set of c++ drop-in replacement functions/classes for ada-py (so I made a new library ada-cpp to improve performance when working with IfcOpenShell, OpenCascade and GMSH.

[aothms]

Hi @Krande I just pushed some major changes to the taxonomy. See commit message 65a5646

It may be a bit disruptive, but at the same time it will facilitate actual usage of this taxonomy with proper memory management (shared ownership so more efficient, integration with python ref counting) and some work towards hashing (so storing these as dict keys as sets). pybind/nanobind shouldn't have any issues wrapping std::shared_ptr, but let me know if you run into difficulties.

[Krande]

Sounds good! Btw. I managed to compile v0.8.0 for all platforms before going on holiday. I'll try to set up daily conda builds for v0.8.0 as soon as I have some extra time :)

[Krande]

Hey @aothms, did 65a5646 break something in the regular tests (error)?

[Krande]

A PR is up (#3534). I have also produced a set of v0.8.0 alpha conda packages in our conda repo:

I'll make some tests with these now

[aothms]

Wonderful, thanks!

How about your taxonomy wrapping? I'm quite interested in pybind/nanobind. Here is a similar write up of the discussion we could have https://github.com/gnuradio/greps/blob/main/grep-0015-remove-swig.md Added to that, more easy fine grained control over what gets wrapped and how, might really reduce the size of our wrapper (especially for WASM). If we better align our C++ and python code bases we could also get some nice speed ups on the python side I think. There's quite a lot of indirections currently. I would also want to have the option to have IFC entity instance as std::shared_ptr in ifcopenshell. That way we can eliminate one of the last remaining reasons for interpreter crashes on the python side. All of this I would rather do on a more modern basis. What do you think?

[Krande]

I figured I should get a conda package of v0.8.0 first before delving deeper into the taxonomy wrapping. Now that the conda packaging is done, I can focus more on the taxonomy wrapping :)

When I looked (very briefly) at the taxonomy before summer, I struggled with figuring out the required "scope" (i.e. which cpp/h files.) I should include in order to be able to use it.

I think I might require some help in understanding the current python/c++ wrapping philosophy, Also a walk-through of the intended ins and outs of the current taxonomy would be really helpful.

So if you're available for a call sometime this week or next week we could discuss some of this?

Let me know what works best for you

[aothms]

Hi @Krande I'm travelling for two more weeks. Starting from the week of the 21th of august we can plan something. Please send a few dates you're available by mail.

[Krande]

@aothms No rush :) Done, I've sent you some dates.