Details

Reviewers

Summary

This patch enables building of pyopenvdb with make deps. This is a first step towards integration of pyopenvdb into Blender, see design task T73201. Aim is to be able to import pyopenvdb in Blender Python Console. This does not include ability to modify Blender OpenVDB grids, only to have access to out-of-box pyopenvdb.

Related devtalk thread for discussion: https://devtalk.blender.org/t/build-pyopenvdb-as-part-of-make-deps/14148

this patch requires D8212 is applied (build Boost Python)
currently works at least on Windows and XUbuntu 20.04.
to enable, add CMake option WITH_PYOPENVDB to make deps in GNUmakefile by e.g. command sed -i 's/cmake\ \-H\"\$(DEPS_SOURCE_DIR/cmake\ -DWITH_PYOPENVDB\=ON\ -DWITH_BOOST_PYTHON\=ON\ \-H\"\$(DEPS_SOURCE_DIR/' GNUmakefile
NumPy is enabled, since it allows use of copyFromArray and copyToArray for fast data transfer from NumPy arrays, which is likely needed by users.
open issue: provide pyopenvdb as static or dynamic library setup?
currently after building Blender with make, Blender needs to be started with command LD_PRELOAD=/usr/lib/x86_64-linux-gnu/libjemalloc.so.2 /path/to/blender in order to avoid ImportError: /lib/x86_64-linux-gnu/libjemalloc.so.2: cannot allocate memory in static TLS block error upon import pyopenvdb

Diff Detail

Repository: rB Blender

Event Timeline

Tuomo Keskitalo (tkeskita) requested review of this revision.Jun 25 2020, 5:19 PM

Tuomo Keskitalo (tkeskita) created this revision.

remove extra print line

Tuomo Keskitalo (tkeskita) added inline comments.Jun 25 2020, 5:28 PM

build_files/build_environment/cmake/osl.cmake
80 ↗	(On Diff #26285)	This avoids `python: not found` error on default xubuntu 20.04.

The deps builder is hard enough to maintain as is, without tagging on stuff that we do not ship in the official versions, not entirely sure adding things like this is having any benefits beyond a burden to maintain it.

that is unless we decide pyopenvdb is something we'd want to ship with the official blender versions, but that a conversation that should probably have been had long before we reached the patch review stage.

Sorry my miscommunication. My aim is to develop/maintain this diff for the people who need pyopenvdb in Blender, until such a time that pyopenvdb is included in Blender (if ever). Please let me know if there is a better approach, thanks!

Not entirely sure how to deal with this, paging @Brecht Van Lommel (brecht)

We do want to ship pyopenvdb with Blender at some point (it's mentioned in T73201).

However for that will have to find a way to pass Blender OpenVDB grids to Python, which I think requires changes to pyopenvdb. This change by itself is not enough to ship it.

I suggest to add a WITH_PYOPENVDB cmake option similar to how we have WITH_WEBP, so that it can be disabled by default until we properly integrate it with the Blender volume object.

GNUmakefile
340–355 ↗	(On Diff #26285)	Leave out these unrelated changes.

This revision now requires changes to proceed.Jun 26 2020, 2:06 PM

Thanks @Brecht Van Lommel (brecht), I'll try do that. There are currently several issues related to building and using this that I need to document and to communicate with users if they try this on "make deps". What is a proper place for such information and user questions/discussions, maybe a devtalk thread, where I update the documentation?

From the Blender project side, I consider this an incremental step towards a feature we want to add. But it's not something we would document in this state (besides a description for the CMake option) or advise users to try.

If you want to start a devtalk thread, you can. It's not clear to me what you would document though.

@Tuomo Keskitalo (tkeskita) did some cleanup and fixed up windows support, mind if i update your patch ?

edit: actually this will need a little more work, and we'll have to move openvdb to a dynamic lib to make it come together nicely at-least on windows, unsure on the other platforms.

either way doesn't feel like we could easily hide this behind a switch

@Ray Molenkamp (LazyDodo) please feel free to update patch when you have something, any Windows help is appreciated. Yes, there are some things that need work. I'll update the description later on about my findings so far.

Given "we're doing this" and there's several steps that don't necessary should go together, and we're already in bcon2 i don't see this making it in for 2.90 meaning we have plenty of time to do it right for 2.91 (the prep-work that is, i make no guarantees when/if pyopenvdb will actually land)

I'd like to split this out over several changes that can be done independently

OSL python dep, have tested this fix already, i'll commit this later today
Enabling boost::python should be done in it's own change set, i'd like to get this in when 2.91 hits bcon0 (which is looking like the end of july at this point)
Linking OpenVDB Statically into both the python pyopenvdb lib and blender is not gonna be good for our on disk footprint and may cause some issues with any global state openvdb may maintain, so i'd like to move openvdb to a dynamic lib at-least on windows, the other platforms will have to make their own call there, also aiming for 2.91 here.

At this point any changes to enable building/deployment of pyopenvdb should be minimal and be easily be hidden behind some kind of switch.

@Brecht Van Lommel (brecht) , thoughts?

Cleaned up patch, updated summary with my current knowledge.

Ray Molenkamp (LazyDodo) mentioned this in rBfa2c7709ff7a: Deps Builder: Fix OSL build error..Jun 28 2020, 4:00 PM

Added few inline comments.

build_files/build_environment/cmake/python.cmake
91–99 ↗	(On Diff #26345)	This is ugly, but I haven't found another way to make Boost detect correct python.
build_files/build_environment/patches/openvdb.diff
82 ↗	(On Diff #26345)	tried ${PYTHON_SHORT_VERSION} here, it didn't work..?

Ray Molenkamp (LazyDodo) added inline comments.Jun 29 2020, 6:14 PM

build_files/build_environment/cmake/python.cmake

91–99 ↗

(On Diff #26345)

on windows i use a user jam file to point it to the right place.

set(semi_path "${PATCH_DIR}/semi.txt")
FILE(TO_NATIVE_PATH ${semi_path} semi_path)
set(BOOST_CONFIGURE_COMMAND bootstrap.bat &&
              echo using python : ${PYTHON_SHORT_VERSION} : ${LIBDIR}/python/python${PYTHON_POSTFIX}.exe > "${JAM_FILE}" &&
              echo.   : ${LIBDIR}/python/include >> "${JAM_FILE}" &&
              echo.   : ${LIBDIR}/python/libs >> "${JAM_FILE}" &&
              type ${semi_path} >> "${JAM_FILE}"
)
set(BOOST_BUILD_COMMAND bjam)
set(BOOST_BUILD_OPTIONS runtime-link=shared --user-config=user-config.jam)

the semi.txt is somewhat if a dirty hack since i was unable to get it to emit a ; without it.

Ray Molenkamp (LazyDodo) added inline comments.Jun 29 2020, 6:16 PM

build_files/build_environment/patches/openvdb.diff
82 ↗	(On Diff #26345)	`PYTHON_SHORT_VERSION` lives in the blender build scripts, it will not be available to external dependencies unless you pass the value with with `-Dblah=${blah}` in the configure command.

we're getting awfully chatty though, not really the point of code-review could be better to move this to either devalk or blender.chat

OK, devtalk thread created, let's discuss over there: https://devtalk.blender.org/t/build-pyopenvdb-as-part-of-make-deps/14148

In D8123#198025, @Ray Molenkamp (LazyDodo) wrote:

Enabling boost::python should be done in it's own change set, i'd like to get this in when 2.91 hits bcon0 (which is looking like the end of july at this point)

Linking OpenVDB Statically into both the python pyopenvdb lib and blender is not gonna be good for our on disk footprint and may cause some issues with any global state openvdb may maintain, so i'd like to move openvdb to a dynamic lib at-least on windows, the other platforms will have to make their own call there, also aiming for 2.91 here.

At this point any changes to enable building/deployment of pyopenvdb should be minimal and be easily be hidden behind some kind of switch.

I would put the boost::python building behind the same switch. Moving OpenVDB to a shared library on Windows already is fine with me. For Linux and macOS, this is not something we have done so far for other libraries and is likely to cause new problems. We may have to do this, but I don't want to do it by default yet until we are either ready to ship with pyopenvdb or decide to start using shared libraries in general.

To be clear, I don't know when exactly I will have time to add pyopenvdb support in Blender. The OpenVDB project also seems to be trying to get rid of Boost as a dependency (and maybe use pybind11?). If there's something we can easily have behind a CMake option it's fine, I just don't want to cause a bunch of work for platform maintainers at this point.

Ray Molenkamp (LazyDodo) mentioned this in D8212: Enable building boost::python from the deps builder.Jul 5 2020, 8:36 PM

Updated patch with D8212 as prerequisite

Tuomo Keskitalo (tkeskita) added inline comments.Jul 6 2020, 1:02 PM

build_files/build_environment/cmake/openvdb.cmake
54	This was needed for `find_package(Python)` to find above Python

Almost there.

build_files/build_environment/cmake/harvest.cmake
143	Add `if(WITH_PYOPENVDB)`

This revision now requires changes to proceed.Jul 6 2020, 6:45 PM

In D8123#199781, @Brecht Van Lommel (brecht) wrote:

Almost there.

Windows is still horribly broken, we're pretty far from "there"

build_files/build_environment/cmake/openvdb.cmake
103	missing boost dependency, which may cause build order issues.
build_files/build_environment/cmake/options.cmake
26	Always tell users if you are forcibly enable an option, in blender we generally take the negative route though. if a dep is on, but its requirements are not met it would be turned off, see here how we do it in blenders cmake. I'd like to keep this behavior consistent.

Updated diff from comments

build_files/build_environment/cmake/openvdb.cmake
103	`external_boost` is there above in the first `add_dependencies`, I hope no need to repeat?

Tuomo Keskitalo (tkeskita) marked 2 inline comments as done.Jul 11 2020, 2:28 PM

Tuomo Keskitalo (tkeskita) edited the summary of this revision. (Show Details)Jul 11 2020, 2:33 PM

Ray Molenkamp (LazyDodo) mentioned this in D8282: Deps:Build openvdb as a shared lib on windows.Jul 13 2020, 6:15 PM

It looks fine to me now, but did not test on Windows.

This revision is now accepted and ready to land.Jul 15 2020, 1:59 PM

Windows still needs changes to deal with statically linked boost with dynamically linked openvdb which they did not count on ever happening, I have a rough version but given that patch depends on 2 other patches (D8212 + D8282) already and i'm just bad at juggling multiple depended patches , i'm going to sit it out and wait until we hit bcon1 for 2.91 and i can land D8212 and D8282 before stacking more stuff on top.