blender
/
blender
136
406
Fork 589
Code Issues 6.3k Pull Requests 766 Projects 19 Wiki Activity

Cycles: upgrade Embree to version 4.0 #105974

Merged
Xavier Hallade merged 5 commits from xavierh/blender:cycles_embree4 into main 2023-04-05 11:03:19 +02:00
Conversation 14 Commits 5 Files Changed 17 +778 -347
Xavier Hallade commented 2023-03-21 22:19:33 +01:00
Member
Copy Link

Upgrade to new Embree 4 while staying compatible with Embree 3.
For more information about Embree 3->4 API changes:
https://github.com/embree/embree/blob/master/doc/src/api.md#upgrading-from-embree-3-to-embree-4

This is not yet enabling HW RT on Arc GPUs using Embree, that PR will come next.
Here we start with landing embree 4 hopefully smoothly on all CPU platforms.

Upgrade to new Embree 4 while staying compatible with Embree 3. For more information about Embree 3->4 API changes: https://github.com/embree/embree/blob/master/doc/src/api.md#upgrading-from-embree-3-to-embree-4 This is not yet enabling HW RT on Arc GPUs using Embree, that PR will come next. Here we start with landing embree 4 hopefully smoothly on all CPU platforms.
Xavier Hallade added this to the 3.6 LTS milestone 2023-03-21 22:19:33 +01:00
Xavier Hallade added the
Module
Render & Cycles
 label 2023-03-21 22:19:33 +01:00
Brecht Van Lommel was assigned by Xavier Hallade 2023-03-21 22:19:33 +01:00
Sergey Sharybin was assigned by Xavier Hallade 2023-03-21 22:19:33 +01:00
Xavier Hallade added this to the Render & Cycles project 2023-03-21 22:37:31 +01:00
Brecht Van Lommel was unassigned by Xavier Hallade 2023-03-21 22:37:53 +01:00
Sergey Sharybin was unassigned by Xavier Hallade 2023-03-21 22:37:53 +01:00
Nikita Sirgienko was assigned by Xavier Hallade 2023-03-21 22:38:04 +01:00
Xavier Hallade requested review from Sergey Sharybin 2023-03-21 22:38:14 +01:00
Xavier Hallade requested review from Brecht Van Lommel 2023-03-21 22:38:22 +01:00
Xavier Hallade force-pushed cycles_embree4 from 9ae1b7bb90 to 4adf43de88 2023-03-22 09:56:33 +01:00 Compare
Sergey Sharybin commented 2023-03-22 11:52:53 +01:00
Owner
Copy Link

@blender-bot build

@blender-bot build
Xavier Hallade force-pushed cycles_embree4 from 29154f8303 to 683fc0d376 2023-03-22 13:19:12 +01:00 Compare
Sergey Sharybin commented 2023-03-22 17:30:39 +01:00
Owner
Copy Link

@blender-bot build

@blender-bot build
Sergey Sharybin commented 2023-03-22 17:35:18 +01:00
Owner
Copy Link

Tested the change on Intel macOS. It compiles fine, tests are passing, and there are no speed changes for the Embree3. The code I only checked from a top level, didn't do careful line-by-line investigation, but from this perspective it also seems good.

Unfortunately, I can not currently test Embree4 as the dependency builder fails on my machine with an absolutely unrelated error (also happens in the main branch).

Tested the change on Intel macOS. It compiles fine, tests are passing, and there are no speed changes for the Embree3. The code I only checked from a top level, didn't do careful line-by-line investigation, but from this perspective it also seems good. Unfortunately, I can not currently test Embree4 as the dependency builder fails on my machine with an absolutely unrelated error (also happens in the main branch).
👍 1
Xavier Hallade commented 2023-03-22 17:55:56 +01:00
Author
Member
Copy Link

great! Here is one more commit to enable building embree with GPU support (before we can use it in next PR), not sure if you want to have it split in a separate PR or keep it here with overall Embree 4 update.

It requires using SYCL compiler to compile Embree. Easy on Linux with deps builder.
On Windows... it's easy in case deps builder can use Ninja (that's how I've tested it), if not, well, as VS cmake generator doesn't support using an external compiler, that's more complicated -> @LazyDodo any opinion on c90211d280 ?

great! Here is one more commit to enable building embree with GPU support (before we can use it in next PR), not sure if you want to have it split in a separate PR or keep it here with overall Embree 4 update. It requires using SYCL compiler to compile Embree. Easy on Linux with deps builder. On Windows... it's easy in case deps builder can use Ninja (that's how I've tested it), if not, well, as VS cmake generator doesn't support using an external compiler, that's more complicated -> @LazyDodo any opinion on https://projects.blender.org/blender/blender/commit/c90211d28069980c069f4292c6849f89c650a0e2 ?
Ray molenkamp commented 2023-03-22 18:11:52 +01:00
Member
Copy Link

Haven't looked at it in detail, I can say I have been slowly been migrating most deps over to ninja on windows. If embree isn't already using it I have no objections to switching it over, it'll happen sooner or later, so it may as well be now.

Haven't looked at it in detail, I can say I have been slowly been migrating most deps over to ninja on windows. If embree isn't already using it I have no objections to switching it over, it'll happen sooner or later, so it may as well be now.
Ray molenkamp commented 2023-03-22 21:56:19 +01:00
Member
Copy Link

I kinda took "It requires using SYCL compiler to compile Embree." to mean just the intel GPU kernels would build with dpcpp. But by the looks of it, it replaces GCC/MSVC completely with DPCPP even the CPU based code-paths for non intel gpu-users would now be build by it.

I'm OK with the intel specific GPU code being build with essentially the "flavor of the day" dpcpp version, using it for all code is making me a bit uncomfortable.

I kinda took "It requires using SYCL compiler to compile Embree." to mean just the intel GPU kernels would build with dpcpp. But by the looks of it, it replaces GCC/MSVC completely with DPCPP even the CPU based code-paths for non intel gpu-users would now be build by it. I'm OK with the intel specific GPU code being build with essentially the "flavor of the day" dpcpp version, using it for all code is making me a bit uncomfortable.
Xavier Hallade commented 2023-03-22 22:09:54 +01:00
Author
Member
Copy Link

I'm OK with the intel specific GPU code being build with essentially the "flavor of the day" dpcpp version, using it for all code is making me a bit uncomfortable.

All Embree* code. dpcpp changes vs LLVM are almost exclusive to the SYCL code path so for CPU, it's not as nightly as the name implies.
If that's a no-go, as dpcpp matured quite much since we took nightlies, it should be fine switching to tagged releases. Latest one is https://github.com/intel/llvm/releases/tag/2022-12 - do you want to udpate this dep first?

> I'm OK with the intel specific GPU code being build with essentially the "flavor of the day" dpcpp version, using it for all code is making me a bit uncomfortable. All Embree* code. dpcpp changes vs LLVM are almost exclusive to the SYCL code path so for CPU, it's not as nightly as the name implies. If that's a no-go, as dpcpp matured quite much since we took nightlies, it should be fine switching to tagged releases. Latest one is https://github.com/intel/llvm/releases/tag/2022-12 - do you want to udpate this dep first?
Ray molenkamp commented 2023-03-22 22:27:00 +01:00
Member
Copy Link

Lets wait to see what @brecht and @Sergey 's feelings are on this.

My personal preference would be not to have DPCPP in the mix at all for non intel GPU code paths, but I could be alone in that. Let's first figure out first what we want, and we'll find a way to get there together.

Lets wait to see what @brecht and @Sergey 's feelings are on this. My personal preference would be not to have DPCPP in the mix at all for non intel GPU code paths, but I could be alone in that. Let's first figure out first what we want, and we'll find a way to get there together.
Brecht Van Lommel commented 2023-03-22 23:00:09 +01:00
Owner
Copy Link

By itself I don't mind Embree being compiled with DPCPP, to me it's not any different than what we have for OpenImageDenoise and ISPC.

What I do really dislike is the ever growing complexity of building Intel libraries, compilers and GPU kernels. As I've suggested before I would much rather have GPU support in Intel libraries be provided through header files that we can compile ourselves as part of the Blender build, for OpenImageDenoise, OpenPGL and Embree.

The way this is being done now is very complicated not just for Blender but other software that wants to use these libraries, as well as Linux distributions. The amount of complexity for this GPU backend compared to the number of Blender users that benefit from it is really off.

I don't think there is much we can do to change that immediately, but I really hope GPU support in OpenImageDenoise and OpenPGL is not going to make this significantly more complicated again.

By itself I don't mind Embree being compiled with DPCPP, to me it's not any different than what we have for OpenImageDenoise and ISPC. What I do really dislike is the ever growing complexity of building Intel libraries, compilers and GPU kernels. As I've suggested before I would much rather have GPU support in Intel libraries be provided through header files that we can compile ourselves as part of the Blender build, for OpenImageDenoise, OpenPGL and Embree. The way this is being done now is very complicated not just for Blender but other software that wants to use these libraries, as well as Linux distributions. The amount of complexity for this GPU backend compared to the number of Blender users that benefit from it is really off. I don't think there is much we can do to change that immediately, but I really hope GPU support in OpenImageDenoise and OpenPGL is not going to make this significantly more complicated again.
Ray molenkamp commented 2023-03-23 00:06:42 +01:00
Member
Copy Link

small change needed to get this to build with dpcpp on windows, clang++ wanted to use lld-link as its linker, which wasn't enabled causing cmake to error out during compiler discovery.

diff --git a/build_files/build_environment/cmake/dpcpp.cmake b/build_files/build_environment/cmake/dpcpp.cmake
index 4a8ddf7de80..fe5740357fc 100644
--- a/build_files/build_environment/cmake/dpcpp.cmake
+++ b/build_files/build_environment/cmake/dpcpp.cmake
@@ -48,7 +48,7 @@ set(DPCPP_EXTRA_ARGS
   -DXPTI_SOURCE_DIR=${DPCPP_SOURCE_ROOT}/xpti
   -DLLVM_EXTERNAL_XPTIFW_SOURCE_DIR=${DPCPP_SOURCE_ROOT}/xptifw
   -DLLVM_EXTERNAL_LIBDEVICE_SOURCE_DIR=${DPCPP_SOURCE_ROOT}/libdevice
-  -DLLVM_ENABLE_PROJECTS=clang^^sycl^^llvm-spirv^^opencl^^libdevice^^xpti^^xptifw
+  -DLLVM_ENABLE_PROJECTS=clang^^sycl^^llvm-spirv^^opencl^^libdevice^^xpti^^xptifw^^lld
   -DLIBCLC_TARGETS_TO_BUILD=
   -DLIBCLC_GENERATE_REMANGLED_VARIANTS=OFF
   -DSYCL_BUILD_PI_HIP_PLATFORM=AMD
small change needed to get this to build with dpcpp on windows, clang++ wanted to use `lld-link` as its linker, which wasn't enabled causing cmake to error out during compiler discovery. ``` diff --git a/build_files/build_environment/cmake/dpcpp.cmake b/build_files/build_environment/cmake/dpcpp.cmake index 4a8ddf7de80..fe5740357fc 100644 --- a/build_files/build_environment/cmake/dpcpp.cmake +++ b/build_files/build_environment/cmake/dpcpp.cmake @@ -48,7 +48,7 @@ set(DPCPP_EXTRA_ARGS -DXPTI_SOURCE_DIR=${DPCPP_SOURCE_ROOT}/xpti -DLLVM_EXTERNAL_XPTIFW_SOURCE_DIR=${DPCPP_SOURCE_ROOT}/xptifw -DLLVM_EXTERNAL_LIBDEVICE_SOURCE_DIR=${DPCPP_SOURCE_ROOT}/libdevice - -DLLVM_ENABLE_PROJECTS=clang^^sycl^^llvm-spirv^^opencl^^libdevice^^xpti^^xptifw + -DLLVM_ENABLE_PROJECTS=clang^^sycl^^llvm-spirv^^opencl^^libdevice^^xpti^^xptifw^^lld -DLIBCLC_TARGETS_TO_BUILD= -DLIBCLC_GENERATE_REMANGLED_VARIANTS=OFF -DSYCL_BUILD_PI_HIP_PLATFORM=AMD ```
Ray molenkamp reviewed 2023-03-23 00:09:15 +01:00
build_files/build_environment/cmake/embree.cmake Outdated
@ -25,0 +34,4 @@
)
endif()
if(WIN32)
if(CMAKE_GENERATOR MATCHES "Visual*")
Ray molenkamp commented 2023-03-23 00:09:15 +01:00
Member
Copy Link

This can be removed, the top level (the one that manages the order the deps get build in) generator will still be msbuild, but the embree dep will build with ninja because you added CMAKE_GENERATOR in ExternalProject_Add

This can be removed, the top level (the one that manages the order the deps get build in) generator will still be msbuild, but the embree dep will build with ninja because you added `CMAKE_GENERATOR` in `ExternalProject_Add`
xavierh marked this conversation as resolved
Ray molenkamp commented 2023-03-23 05:08:37 +01:00
Member
Copy Link

By itself I don't mind Embree being compiled with DPCPP, to me it's not any different than what we have for OpenImageDenoise and ISPC.

This is different though, OIDN has only ever had one way of building its kernels, here we change the compiler for the normal CPU kernels that have always build with msvc/gcc just fine to dpcpp for all embree users just to satisfy the needs of a single GPU target.

I'm all for taking some risk with intel's GPU arch, I don't even mind the complexity and all that much, it's all being done in isolation, intel GPU support never had the opportunity to cause behavioral changes for any of the other code paths, this changes that, intel GPU or not, you are now running dpcpp output, some pause and reflection ought to be had, even if we agree it's OK in the end.

> By itself I don't mind Embree being compiled with DPCPP, to me it's not any different than what we have for OpenImageDenoise and ISPC. This is different though, OIDN has only ever had one way of building its kernels, here we change the compiler for the normal CPU kernels that have always build with msvc/gcc just fine to dpcpp for _all_ embree users _just_ to satisfy the needs of a single GPU target. I'm all for taking some risk with intel's GPU arch, I don't even mind the complexity and all that much, it's all being done in isolation, intel GPU support never had the opportunity to cause behavioral changes for any of the other code paths, this changes that, intel GPU or not, you are now running dpcpp output, some pause and reflection ought to be had, even if we agree it's OK in the end.
Xavier Hallade force-pushed cycles_embree4 from 837f3da67c to b48c680419 2023-03-24 17:40:29 +01:00 Compare
Brecht Van Lommel requested changes 2023-03-27 13:09:54 +02:00
build_files/build_environment/cmake/embree.cmake Outdated
@ -25,0 +49,4 @@
-DCMAKE_C_FLAGS_MINSIZEREL=${EMBREE_CMAKE_COMPILER_FLAGS_MinSizeRel}
-DCMAKE_C_FLAGS_RELWITHDEBINFO=${EMBREE_CMAKE_COMPILER_FLAGS_RelWithDebInfo}
-DCMAKE_C_FLAGS_DEBUG=${EMBREE_CMAKE_COMPILER_FLAGS_Debug}
)
Brecht Van Lommel commented 2023-03-27 12:57:27 +02:00
Owner
Copy Link

Can we move this to options.cmake and call this BLENDER_CLANG_CMAKE_FLAGS?

So we can more easily keep it in sync with the Visual Studio flags.

Can we move this to `options.cmake` and call this `BLENDER_CLANG_CMAKE_FLAGS`? So we can more easily keep it in sync with the Visual Studio flags.
xavierh marked this conversation as resolved
build_files/build_environment/cmake/embree.cmake Outdated
@ -69,0 +113,4 @@
COMMAND ${CMAKE_COMMAND} -E copy ${LIBDIR}/embree/lib/tasking.lib ${HARVEST_TARGET}/embree/lib/tasking_d.lib
DEPENDEES install
)
else()
Brecht Van Lommel commented 2023-03-27 12:59:10 +02:00
Owner
Copy Link

Can deduplicate code here with a second call to ExternalProject_Add_Step(external_embree after_install for the sycl/rthwif libraries?

Can deduplicate code here with a second call to `ExternalProject_Add_Step(external_embree after_install` for the sycl/rthwif libraries?
xavierh marked this conversation as resolved
build_files/cmake/Modules/FindEmbree.cmake Outdated
@ -70,0 +82,4 @@
IF(EMBREE_EMBREE4_LIBRARY)
SET(EMBREE_SHARED TRUE)
SET(_embree_LIBRARIES ${EMBREE_EMBREE4_LIBRARY})
BREAK()
Brecht Van Lommel commented 2023-03-27 13:03:26 +02:00
Owner
Copy Link

This logic is still rather fuzzy to me. It seems like it will prefer embree3 over embree4, and break before embree_rthwif is found?

Can you detect the Embree version first (e.g. with the existence of a header file, or parsing the version number from a header file), and then set the libraries to search for based on that?

This logic is still rather fuzzy to me. It seems like it will prefer embree3 over embree4, and break before `embree_rthwif` is found? Can you detect the Embree version first (e.g. with the existence of a header file, or parsing the version number from a header file), and then set the libraries to search for based on that?
xavierh marked this conversation as resolved
build_files/cmake/Modules/FindEmbree.cmake Outdated
@ -84,0 +104,4 @@
SET(EMBREE_MAJOR_VERSION 3)
endif()
if(EMBREE_EMBREE4_SYCL_LIBRARY)
SET(EMBREE_WITH_GPU_SUPPORT TRUE)
Brecht Van Lommel commented 2023-03-27 13:08:01 +02:00
Owner
Copy Link

I'd prefer to have EMBREE_LIBRARIES_GPU instead of this, so we can link to them depending if oneAPI is used.

I'd prefer to have `EMBREE_LIBRARIES_GPU` instead of this, so we can link to them depending if oneAPI is used.
xavierh marked this conversation as resolved
build_files/cmake/platform/platform_unix.cmake Outdated
@ -318,3 +318,3 @@
endif()
if(WITH_CYCLES AND WITH_CYCLES_DEVICE_ONEAPI)
if(WITH_CYCLES AND (WITH_CYCLES_DEVICE_ONEAPI OR EMBREE_WITH_GPU_SUPPORT))
Brecht Van Lommel commented 2023-03-27 12:52:13 +02:00
Owner
Copy Link

Can we not link to embree_rthwif and embree4_sycl instead of this? Or does embree4 have a dependency on this?

Can we not link to `embree_rthwif` and `embree4_sycl` instead of this? Or does `embree4` have a dependency on this?
xavierh marked this conversation as resolved
build_files/cmake/platform/platform_win32.cmake Outdated
@ -850,1 +851,4 @@
set(EMBREE_INCLUDE_DIRS ${LIBDIR}/embree/include)
if(NOT EXISTS ${LIBDIR}/embree/lib/sys.lib)
set(EMBREE_SHARED TRUE)
Brecht Van Lommel commented 2023-03-27 13:06:28 +02:00
Owner
Copy Link

Is the plan to switch Embree to be a shared library? Since we generally don't have code for both cases in Windows, rather just what we actually use.

I'd be fine with, as we have other shared libraries as well now and maybe it's easier. Not sure if there are any performance implications.

Is the plan to switch Embree to be a shared library? Since we generally don't have code for both cases in Windows, rather just what we actually use. I'd be fine with, as we have other shared libraries as well now and maybe it's easier. Not sure if there are any performance implications.
xavierh marked this conversation as resolved
intern/cycles/blender/python.cpp Outdated
@ -1030,2 +1030,4 @@
PyModule_AddObject(mod, "with_embree", Py_True);
Py_INCREF(Py_True);
PyModule_AddIntConstant(mod, "embree_major_version", EMBREE_MAJOR_VERSION);
# ifdef EMBREE_WITH_GPU_SUPPORT
Brecht Van Lommel commented 2023-03-27 13:08:44 +02:00
Owner
Copy Link

WITH_EMBREE_GPU and with_embree_gpu I think would be more consistent naming.

`WITH_EMBREE_GPU` and `with_embree_gpu` I think would be more consistent naming.
xavierh marked this conversation as resolved
intern/cycles/bvh/CMakeLists.txt Outdated
@ -64,0 +66,4 @@
list(APPEND LIB debug ${SYCL_LIBRARY_DEBUG} optimized ${SYCL_LIBRARY})
else()
list(APPEND LIB ${SYCL_LIBRARY})
endif()
Brecht Van Lommel commented 2023-03-27 12:54:26 +02:00
Owner
Copy Link

Can this Windows specific logic be kept in platform_win32.cmake? We normally don't need to do this debug/optimized distinction in places like this.

Can this Windows specific logic be kept in `platform_win32.cmake`? We normally don't need to do this debug/optimized distinction in places like this.
xavierh marked this conversation as resolved
Xavier Hallade reviewed 2023-03-27 14:02:29 +02:00
build_files/cmake/Modules/FindEmbree.cmake Outdated
@ -70,0 +82,4 @@
IF(EMBREE_EMBREE4_LIBRARY)
SET(EMBREE_SHARED TRUE)
SET(_embree_LIBRARIES ${EMBREE_EMBREE4_LIBRARY})
BREAK()
Xavier Hallade commented 2023-03-27 13:36:44 +02:00
Author
Member
Copy Link

The whole foreach is for the case in which there are no static libraries, as initially.
In that case, there is no embree_rthwif to find and either EMBREE_EMBREE4_LIBRARY or EMBREE_EMBREE3_LIBRARY will exist but not both.
We can determine embree version with the include_dir search (embree4/rtcore.h vs embree3/rtcore.h)

The whole foreach is for the case in which there are no static libraries, as initially. In that case, there is no embree_rthwif to find and either EMBREE_EMBREE4_LIBRARY or EMBREE_EMBREE3_LIBRARY will exist but not both. We can determine embree version with the include_dir search (embree4/rtcore.h vs embree3/rtcore.h)
xavierh marked this conversation as resolved
build_files/cmake/platform/platform_unix.cmake Outdated
@ -318,3 +318,3 @@
endif()
if(WITH_CYCLES AND WITH_CYCLES_DEVICE_ONEAPI)
if(WITH_CYCLES AND (WITH_CYCLES_DEVICE_ONEAPI OR EMBREE_WITH_GPU_SUPPORT))
Xavier Hallade commented 2023-03-27 13:31:43 +02:00
Author
Member
Copy Link

embree4 depends on embree_rthwif and embree4_sycl when it's built with GPU support.

embree4 depends on embree_rthwif and embree4_sycl when it's built with GPU support.
xavierh marked this conversation as resolved
build_files/cmake/platform/platform_win32.cmake Outdated
@ -850,1 +851,4 @@
set(EMBREE_INCLUDE_DIRS ${LIBDIR}/embree/include)
if(NOT EXISTS ${LIBDIR}/embree/lib/sys.lib)
set(EMBREE_SHARED TRUE)
Xavier Hallade commented 2023-03-27 13:38:19 +02:00
Author
Member
Copy Link

It's not really a plan but there were already traces for support of Embree as shared lib in the current codebase, and it got useful to me to debug one issue coming from static linking, so I kept it working.

It's not really a plan but there were already traces for support of Embree as shared lib in the current codebase, and it got useful to me to debug one issue coming from static linking, so I kept it working.
xavierh marked this conversation as resolved
intern/cycles/blender/python.cpp Outdated
@ -1030,2 +1030,4 @@
PyModule_AddObject(mod, "with_embree", Py_True);
Py_INCREF(Py_True);
PyModule_AddIntConstant(mod, "embree_major_version", EMBREE_MAJOR_VERSION);
# ifdef EMBREE_WITH_GPU_SUPPORT
Xavier Hallade commented 2023-03-27 13:45:22 +02:00
Author
Member
Copy Link

EMBREE_WITH_GPU_SUPPORT means embree is built with GPU support, which has implications like the dependency on sycl and its compiler.
In my view, WITH_EMBREE_GPU would mean it's used from a code path but in current PR, it's not.

`EMBREE_WITH_GPU_SUPPORT` means embree is built with GPU support, which has implications like the dependency on sycl and its compiler. In my view, `WITH_EMBREE_GPU` would mean it's used from a code path but in current PR, it's not.
xavierh marked this conversation as resolved
Ray molenkamp reviewed 2023-03-27 17:10:55 +02:00
build_files/cmake/Modules/FindEmbree.cmake Outdated
@ -84,0 +104,4 @@
SET(EMBREE_MAJOR_VERSION 3)
endif()
if(EMBREE_EMBREE4_SYCL_LIBRARY)
SET(EMBREE_WITH_GPU_SUPPORT TRUE)
Ray molenkamp commented 2023-03-27 17:09:01 +02:00
Member
Copy Link

We can't, when build with dpcpp it'll have sycl references in sys.lib and embree4.lib, regardless if oneAPI is ON or OFF, we're gonna have to link sycl.

We can't, when build with dpcpp it'll have sycl references in sys.lib and embree4.lib, regardless if oneAPI is ON or OFF, we're gonna have to link sycl.
xavierh marked this conversation as resolved
Brecht Van Lommel requested changes 2023-03-27 18:18:04 +02:00
build_files/cmake/platform/platform_unix.cmake Outdated
@ -318,3 +318,3 @@
endif()
if(WITH_CYCLES AND WITH_CYCLES_DEVICE_ONEAPI)
if(WITH_CYCLES AND (WITH_CYCLES_DEVICE_ONEAPI OR EMBREE_WITH_GPU_SUPPORT))
Brecht Van Lommel commented 2023-03-27 18:17:58 +02:00
Owner
Copy Link

Ok, it should check WITH_CYCLES_EMBREE as well then.

Ok, it should check `WITH_CYCLES_EMBREE` as well then.
xavierh marked this conversation as resolved
intern/cycles/blender/python.cpp Outdated
@ -1030,2 +1030,4 @@
PyModule_AddObject(mod, "with_embree", Py_True);
Py_INCREF(Py_True);
PyModule_AddIntConstant(mod, "embree_major_version", EMBREE_MAJOR_VERSION);
# ifdef EMBREE_WITH_GPU_SUPPORT
Brecht Van Lommel commented 2023-03-27 18:16:08 +02:00
Owner
Copy Link

Since it's not used in this PR, let's just leave it out then and add something when its purpose is clear.

Since it's not used in this PR, let's just leave it out then and add something when its purpose is clear.
xavierh marked this conversation as resolved
Xavier Hallade requested review from Brecht Van Lommel 2023-03-27 19:59:04 +02:00
Xavier Hallade force-pushed cycles_embree4 from 89b2383643 to d3f32875fe 2023-03-29 14:12:21 +02:00 Compare
Xavier Hallade force-pushed cycles_embree4 from d3f32875fe to 2e91f1a31f 2023-03-29 14:17:10 +02:00 Compare
Xavier Hallade commented 2023-03-29 14:20:49 +02:00
Author
Member
Copy Link

To makes things smoother, I propose to first land with just dynamic library + CPU-only support.
I've rebased and squashed the remaining commits while dropping the changes related to building Embree with SYCL support, these will go into the next PR for Embree GPU support.

To makes things smoother, I propose to first land with just dynamic library + CPU-only support. I've rebased and squashed the remaining commits while dropping the changes related to building Embree with SYCL support, these will go into the next PR for Embree GPU support.
Brecht Van Lommel requested changes 2023-03-29 18:25:36 +02:00
Brecht Van Lommel left a comment
Owner
Copy Link

This looks fine overall now, just one comment.

This looks fine overall now, just one comment.
build_files/cmake/Modules/FindEmbree.cmake Outdated
@ -32,0 +43,4 @@
)
ENDIF()
FILE(READ ${EMBREE_INCLUDE_DIR}/embree${EMBREE_MAJOR_VERSION}/rtcore_config.h _embree_config_header)
Brecht Van Lommel commented 2023-03-29 18:23:19 +02:00
Owner
Copy Link

Should this check if EMBREE_INCLUDE_DIR was found, to gracefully handle Embree not being found?

Should this check if EMBREE_INCLUDE_DIR was found, to gracefully handle Embree not being found?
xavierh marked this conversation as resolved
Brecht Van Lommel commented 2023-03-29 18:46:46 +02:00
Owner
Copy Link

@blender-bot build

@blender-bot build
Xavier Hallade requested review from Brecht Van Lommel 2023-03-29 18:46:49 +02:00
Brecht Van Lommel approved these changes 2023-03-29 18:48:11 +02:00
Brecht Van Lommel left a comment
Owner
Copy Link

Accepting assuming the build succeeds.

Accepting assuming the build succeeds.
Ray molenkamp requested review from Ray molenkamp 2023-03-29 18:53:56 +02:00
Ray molenkamp commented 2023-03-29 18:54:22 +02:00
Member
Copy Link

I'd like a closer look before landing

I'd like a closer look before landing
Ray molenkamp requested changes 2023-03-29 23:26:49 +02:00
Ray molenkamp left a comment
Member
Copy Link

part of the review only, i still need to check out the blender side of things but that build is still running and i'm heading home for the day.

part of the review only, i still need to check out the blender side of things but that build is still running and i'm heading home for the day.
build_files/build_environment/cmake/embree.cmake Outdated
@ -63,2 +58,2 @@
COMMAND ${CMAKE_COMMAND} -E copy ${LIBDIR}/embree/lib/sys.lib ${HARVEST_TARGET}/embree/lib/sys_d.lib
COMMAND ${CMAKE_COMMAND} -E copy ${LIBDIR}/embree/lib/tasking.lib ${HARVEST_TARGET}/embree/lib/tasking_d.lib
COMMAND ${CMAKE_COMMAND} -E copy ${LIBDIR}/embree/lib/embree4_d.dll ${HARVEST_TARGET}/embree/lib/embree4_d.dll
COMMAND ${CMAKE_COMMAND} -E copy ${LIBDIR}/embree/lib/embree4_d.lib ${HARVEST_TARGET}/embree/lib/embree4_d.lib
Ray molenkamp commented 2023-03-29 22:55:03 +02:00
Member
Copy Link
  • The dll lives in /bin, not /lib
  • the release branch above now copies a bunch of runtime dlls which is undesirable
  • my copy had some whitespace issues in this block

for convenience here's what this whole block needs to be

if(WIN32)
  if(BUILD_MODE STREQUAL Release)
    ExternalProject_Add_Step(external_embree after_install
      COMMAND ${CMAKE_COMMAND} -E copy_directory ${LIBDIR}/embree/include ${HARVEST_TARGET}/embree/include
      COMMAND ${CMAKE_COMMAND} -E copy_directory ${LIBDIR}/embree/lib ${HARVEST_TARGET}/embree/lib
      COMMAND ${CMAKE_COMMAND} -E copy_directory ${LIBDIR}/embree/share ${HARVEST_TARGET}/embree/share
      COMMAND ${CMAKE_COMMAND} -E copy ${LIBDIR}/embree/bin/embree4.dll ${HARVEST_TARGET}/embree/bin/embree4.dll
      DEPENDEES install
    )
  else()
    ExternalProject_Add_Step(external_embree after_install
      COMMAND ${CMAKE_COMMAND} -E copy ${LIBDIR}/embree/bin/embree4_d.dll ${HARVEST_TARGET}/embree/bin/embree4_d.dll
      COMMAND ${CMAKE_COMMAND} -E copy ${LIBDIR}/embree/lib/embree4_d.lib ${HARVEST_TARGET}/embree/lib/embree4_d.lib
      DEPENDEES install
    )
  endif()
endif()
- The dll lives in /bin, not /lib - the release branch above now copies a bunch of runtime dlls which is undesirable - my copy had some whitespace issues in this block for convenience here's what this whole block needs to be ``` if(WIN32) if(BUILD_MODE STREQUAL Release) ExternalProject_Add_Step(external_embree after_install COMMAND ${CMAKE_COMMAND} -E copy_directory ${LIBDIR}/embree/include ${HARVEST_TARGET}/embree/include COMMAND ${CMAKE_COMMAND} -E copy_directory ${LIBDIR}/embree/lib ${HARVEST_TARGET}/embree/lib COMMAND ${CMAKE_COMMAND} -E copy_directory ${LIBDIR}/embree/share ${HARVEST_TARGET}/embree/share COMMAND ${CMAKE_COMMAND} -E copy ${LIBDIR}/embree/bin/embree4.dll ${HARVEST_TARGET}/embree/bin/embree4.dll DEPENDEES install ) else() ExternalProject_Add_Step(external_embree after_install COMMAND ${CMAKE_COMMAND} -E copy ${LIBDIR}/embree/bin/embree4_d.dll ${HARVEST_TARGET}/embree/bin/embree4_d.dll COMMAND ${CMAKE_COMMAND} -E copy ${LIBDIR}/embree/lib/embree4_d.lib ${HARVEST_TARGET}/embree/lib/embree4_d.lib DEPENDEES install ) endif() endif() ```
xavierh marked this conversation as resolved
Xavier Hallade reviewed 2023-03-30 09:06:42 +02:00
build_files/build_environment/cmake/embree.cmake Outdated
@ -25,0 +49,4 @@
-DCMAKE_C_FLAGS_MINSIZEREL=${BLENDER_CLANG_CMAKE_C_FLAGS_MINSIZEREL}
-DCMAKE_C_FLAGS_RELWITHDEBINFO=${BLENDER_CLANG_CMAKE_C_FLAGS_RELWITHDEBINFO}
-DCMAKE_C_FLAGS_DEBUG=${EMBREE_CLANG_CMAKE_C_FLAGS_DEBUG}
-DCMAKE_CXX_STANDARD=17
Xavier Hallade commented 2023-03-27 19:36:31 +02:00
Author
Member
Copy Link

done with e8ca1f142a.

done with e8ca1f142a.
xavierh marked this conversation as resolved
build_files/build_environment/cmake/embree.cmake Outdated
@ -66,3 +59,1 @@
)
endif()
ExternalProject_Add_Step(external_embree after_install
Xavier Hallade commented 2023-03-30 09:05:41 +02:00
Author
Member
Copy Link

Good catch. Runtime DLLs appeared when switching back to MSVC and I missed using bin when slicing the patch back to have only embree4 + dynamic library.
Thanks for the block. I've pushed the change.

Good catch. Runtime DLLs appeared when switching back to MSVC and I missed using bin when slicing the patch back to have only embree4 + dynamic library. Thanks for the block. I've pushed the change.
xavierh marked this conversation as resolved
build_files/build_environment/cmake/embree.cmake Outdated
@ -69,0 +113,4 @@
)
if(TARGET external_dpcpp)
ExternalProject_Add_Step(external_embree after_install
COMMAND ${CMAKE_COMMAND} -E copy ${LIBDIR}/embree/lib/embree4_sycl.lib ${HARVEST_TARGET}/embree/lib/embree4_sycl_d.lib
Xavier Hallade commented 2023-03-27 19:35:26 +02:00
Author
Member
Copy Link

done in d91bc2eea7

done in d91bc2eea7
xavierh marked this conversation as resolved
build_files/cmake/Modules/FindEmbree.cmake Outdated
@ -32,0 +43,4 @@
)
ENDIF()
IF(EMBREE_INCLUDE_DIR)
Xavier Hallade commented 2023-03-29 18:46:11 +02:00
Author
Member
Copy Link

ok yes, I've just pushed this change.

ok yes, I've just pushed this change.
xavierh marked this conversation as resolved
build_files/cmake/platform/platform_win32.cmake Outdated
@ -850,1 +851,4 @@
set(EMBREE_INCLUDE_DIRS ${LIBDIR}/embree/include)
if(EXISTS ${LIBDIR}/embree/include/embree4/rtcore_config.h)
set(EMBREE_MAJOR_VERSION 4)
Xavier Hallade commented 2023-03-27 19:58:01 +02:00
Author
Member
Copy Link

as discussed, I now let it be built as a shared library.

as discussed, I now let it be built as a shared library.
xavierh marked this conversation as resolved
intern/cycles/blender/python.cpp Outdated
@ -1030,2 +1030,4 @@
PyModule_AddObject(mod, "with_embree", Py_True);
Py_INCREF(Py_True);
PyModule_AddIntConstant(mod, "embree_major_version", EMBREE_MAJOR_VERSION);
# ifdef EMBREE_WITH_GPU_SUPPORT
Xavier Hallade commented 2023-03-27 17:31:56 +02:00
Author
Member
Copy Link

Instead of EMBREE_WITH_GPU_SUPPORT, we could use EMBREE_SYCL_SUPPORT as it's defined in embree config.

Instead of `EMBREE_WITH_GPU_SUPPORT`, we could use `EMBREE_SYCL_SUPPORT` as it's defined in embree config.
xavierh marked this conversation as resolved
Xavier Hallade requested review from Ray molenkamp 2023-03-30 09:07:43 +02:00
Xavier Hallade force-pushed cycles_embree4 from 1f36ad325c to 350fa6fe6c 2023-03-31 18:14:51 +02:00 Compare
Ray molenkamp approved these changes 2023-04-04 20:12:57 +02:00
Ray molenkamp left a comment
Member
Copy Link

Provisionary accepted, hinging on that PLATFORM_BUNDLED_LIBRARIES fix being done, no need for further review.

Provisionary accepted, hinging on that `PLATFORM_BUNDLED_LIBRARIES` fix being done, no need for further review.
build_files/cmake/platform/platform_win32.cmake Outdated
@ -875,0 +916,4 @@
endif()
endif()
if(NOT EMBREE_STATIC_LIB)
list(APPEND PLATFORM_BUNDLED_LIBRARIES ${EMBREE_ROOT_DIR}/bin/embree${EMBREE_MAJOR_VERSION}$<$<CONFIG:Debug>:_d>.dll)
Ray molenkamp commented 2023-04-04 20:11:51 +02:00
Member
Copy Link

PLATFORM_BUNDLED_LIBRARIES wasn't really meant to work yet on windows and just dropped the libs right next to the blender binary. I added support for using the blender.shared folder in #106348 , this does however mean that the final filenames need to be known at configure time, which precludes the use of generator expressions.

list(APPEND PLATFORM_BUNDLED_LIBRARIES 
  RELEASE ${EMBREE_ROOT_DIR}/bin/embree${EMBREE_MAJOR_VERSION}.dll
  DEBUG ${EMBREE_ROOT_DIR}/bin/embree${EMBREE_MAJOR_VERSION}_d.dll
)

should do the trick (also merge main, as #106348 only landed minutes ago)

`PLATFORM_BUNDLED_LIBRARIES` wasn't really meant to work yet on windows and just dropped the libs right next to the blender binary. I added support for using the `blender.shared` folder in #106348 , this does however mean that the final filenames need to be known at configure time, which precludes the use of generator expressions. ``` list(APPEND PLATFORM_BUNDLED_LIBRARIES RELEASE ${EMBREE_ROOT_DIR}/bin/embree${EMBREE_MAJOR_VERSION}.dll DEBUG ${EMBREE_ROOT_DIR}/bin/embree${EMBREE_MAJOR_VERSION}_d.dll ) ``` should do the trick (also merge main, as #106348 only landed minutes ago)
xavierh marked this conversation as resolved
Xavier Hallade added 2 commits 2023-04-04 23:17:07 +02:00
Xavier Hallade added 1 commit 2023-04-05 10:48:12 +02:00
9193d47ba9 Cycles: enable backface culling in Embree 4 for spheres 
With embree3, logic was on Cycles side, with Embree 4 we filter on
Embree side but backface culling was missing for spheres, leading
cycles_pointcloud_cpu test to fail. We've added it back by patching
Embree 4.

Co-authored-by @Stefan_Werner
Xavier Hallade added 1 commit 2023-04-05 10:56:55 +02:00
Xavier Hallade merged commit 9e9baa9085 into main 2023-04-05 11:03:19 +02:00
Brecht Van Lommel removed this from the Render & Cycles project 2023-05-24 19:05:21 +02:00
Sign in to join this conversation.
Reviewers
No reviewers
Sergey Sharybin
Brecht Van Lommel
Ray molenkamp
Labels
Clear labels   
Interest
Alembic

  
Interest
Animation & Rigging

  
Interest
Asset Browser

  
Interest
Asset Browser Project Overview

  
Interest
Audio

  
Interest
Automated Testing

  
Interest
Blender Asset Bundle

  
Interest
BlendFile

  
Interest
Collada

  
Interest
Compatibility
This issue affects/is about backward or forward compatibility

  
Interest
Compositing

  
Interest
Core

  
Interest
Cycles

  
Interest
Dependency Graph

  
Interest
Development Management

  
Interest
EEVEE

  
Interest
EEVEE & Viewport

  
Interest
Freestyle

  
Interest
Geometry Nodes

  
Interest
Grease Pencil

  
Interest
ID Management

  
Interest
Images & Movies

  
Interest
Import Export

  
Interest
Line Art

  
Interest
Masking

  
Interest
Metal

  
Interest
Modeling

  
Interest
Modifiers

  
Interest
Motion Tracking

  
Interest
Nodes & Physics

  
Interest
OpenGL

  
Interest
Overlay

  
Interest
Overrides

  
Interest
Performance

  
Interest
Physics

  
Interest
Pipeline, Assets & IO

  
Interest
Platforms, Builds & Tests

  
Interest
Python API

  
Interest
Render & Cycles

  
Interest
Render Pipeline

  
Interest
Sculpt, Paint & Texture

  
Interest
Text Editor

  
Interest
Translations

  
Interest
Triaging

  
Interest
Undo

  
Interest
USD

  
Interest
User Interface

  
Interest
UV Editing

  
Interest
VFX & Video

  
Interest
Video Sequencer

  
Interest
Virtual Reality

  
Interest
Vulkan

  
Interest
Wayland
Issues relating to Wayland windowing support.

  
Interest
Workbench

  
Interest: X11

Issues relating to Xorg/X11 support.

  
Legacy
Blender 2.8 Project

  
Legacy
Milestone 1: Basic, Local Asset Browser

  
Legacy
OpenGL Error

  
Meta
Good First Issue

  
Meta
Papercut

  
Meta
Retrospective

  
Meta
Security
Issues relating to security: https://wiki.blender.org/wiki/Process/Vulnerability_Reports

  
Module
Animation & Rigging

  
Module
Core

  
Module
Development Management

  
Module
EEVEE & Viewport

  
Module
Grease Pencil

  
Module
Modeling

  
Module
Nodes & Physics

  
Module
Pipeline, Assets & IO

  
Module
Platforms, Builds & Tests

  
Module
Python API

  
Module
Render & Cycles

  
Module
Sculpt, Paint & Texture

  
Module
Triaging

  
Module
User Interface

  
Module
VFX & Video

  
Platform
FreeBSD

  
Platform
Linux

  
Platform
macOS

  
Platform
Windows

  
Priority
High

  
Priority
Low

  
Priority
Normal

  
Priority
Unbreak Now!

  
Status
Archived

  
Status
Confirmed

  
Status
Duplicate

  
Status
Needs Info from Developers

  
Status
Needs Information from User

  
Status
Needs Triage

  
Status
Resolved

  
Type
Bug

  
Type
Design

  
Type
Known Issue

  
Type
Patch

  
Type
Report

  
Type
To Do

No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
Milestone
Clear milestone
No items
No Milestone
3.6 LTS
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
Nikita Sirgienko
4 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#105974
No description provided.
Delete Branch "xavierh/blender:cycles_embree4"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?