Fix undefined aliasing issue when compiling with clang-cl

AcademySoftwareFoundation / openvdb

OpenVDB - Sparse volume data structure and tools

http://www.openvdb.org/

Apache License 2.0

2.71k stars 660 forks source link

Fix undefined aliasing issue when compiling with clang-cl #1786

Open bgottamd opened 7 months ago

bgottamd commented 7 months ago

Fixes the inner ValueT alias throw an undefined error when compiling with clang/clang-cl on Windows by converting the convertValue into a template function.

linux-foundation-easycla[bot] commented 7 months ago

The committers listed above are authorized under a signed CLA.

:white_check_mark: login: bgottamd (776b6fd6beaa4805baf73f0fc731af08659087fd)

bgottamd commented 6 months ago

Adding a quick bump. This small code change is essential so that Blender can support both clang - merged PR here - and openvdb. Without it, blender fails to build due to the cited issue.

LazyDodo commented 5 months ago

any progress here? We'd like to get this working out of the box for blender, and while i have no issue including selected upstream landed patches for our dependencies, upstream unreviewed patches are a bit of a different story.

jo-rn commented 4 months ago

I've hit the same clang bug before and reported it: llvm/llvm-project#64996. Using -fno-delayed-template-parsing, as was suggested there, makes OpenVDB compile successfully with clang-cl.

bgottamd commented 4 months ago

Thanks @jo-rn - this does fix the issue with the header file compilation in blender. However, there is some performance degradation when this flag is used:

File	args	fastest	slowest	avg
classroom 3840x2160 256 samples	header template fix	1209	1217	1213
-fno-delayed-template-parsing	1218	1228	1223

This equates to ~1% drop in performance just from this flag. Hoping this templating change or a modification of the alias can still be merged in.

LazyDodo commented 4 months ago

Odd result, classroom is a blender 2.79 scene, openvdb wasn't integrated till blender 2.83, so the odds of it having an openvdb volume has to be near the low end of things. Did you put the -fno-delayed-template-parsing flag globally on the whole blender project? or isolated it to just the few openvdb related files that needed this fix?

bgottamd commented 4 months ago

I isolated the flag to projects that complained about the header issue. This ended up being only extern_mantaflow and cycles_scene

anthony-linaro commented 3 months ago

For those keeping track of this issue affecting Blender, I have created a PR to work around the issue using the approach described in this thread: https://projects.blender.org/blender/blender/pulls/126236