Closed ix-dcourtois closed 2 years ago
Hi Damien,
Thanks for the PR, really appreciate it!
As you have realized, I build against the USD dev branch. I link a very specific commit in the README, since this is right after Autodesk's MaterialX 1.38.4 PR has been merged. The update includes the glTF MaterialX PBR, and this is why I don't plan to support older USD or MaterialX versions.
USD 22.08, which I presume to be released this or next month, will be the first stable USD version that can be used with guc. I'll try to keep backwards compatibility with old USD versions as much as possible and document supported versions in the README.
Regarding your changes:
find_package
calls of MaterialX and OIIO is a nice addition, and we can merge it like that. As for the symbol problems: I now link MikkTSpace statically - should have done this in the first place - and I plan to use the __declspec
keyword together with a CMake option to expose the library symbols. You can set the CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS var to simulate this.Thanks again for this PR! I'd like to hear your thoughts on these topics.
Pablo
Hi,
Ok I'll rewrite this whole pull request :p
I'll build against the dev branch of USD, this will save the last commit.
If you want, I can handle the declspec stuff (CMake automatically add a libguc_EXPORTS
definition, so the only thing to do is the import/export macros in the header, and flag the struct and the function in guc.h. Just tell me if you have a preference for the macro name (e.g. GUC_API
or something else)
The only thing I'm not sure is the runtime handling. Thing is, I couldn't make CMAKE_MSVC_RUNTIME_LIBRARY
work as expected. Even after I added the new policy (cf note at the end of the documentation page for that variable) I can try again, maybe I missed something (because I totally agree that it would be the best approach)
And I will need to do some additional tests, but I'm pretty sure that the auto-linking of TBB and Boost (in UsdDefaults.cmake
) can be disabled on Windows. Those are "private" dependencies of USD and never directly used in Guc, so on Windows (at least) you don't need to link against them. And that auto-link thing will create compile-time errors if you try to compile Guc in Debug with a release version of USD (which at least I do: don't really like debugging in relwithdebinfo, and using a debug build of USD+deps is awfully slow, and I never have to step into it anyway :p )
I'll update the PR when I'm done rebuilding USD and doing additional tests.
Ok, I've updated the pull request, this should be a lot less intrusive, let me know what you think (and don't hesitate to edit things if you want to, I think you can, directly on Github)
Hi Damien, I've spend some time evaluating possible solutions to our problems.
There are two points which I think we need to change.
1) I don't want to hardcode libguc as a SHARED library, for obvious reasons. This keyword should be removed - the library is automatically build as shared when -DBUILD_SHARED_LIBS=ON
is specified, which I want to document in the README after merging this PR.
2) After having a closer look at the WINDOWS_EXPORT_ALL_SYMBOLS
target property, I think it's a simpler solution than the __declspec
macros. In fact, we don't even need to modify the guc.h header in any way. Draco uses it, too. I've implemented it in commit bbe7f59c.
Lastly, I wanted to ask whether the if(NOT MSVC)
statement for TBB_USE_DEBUG is really necessary - is an error thrown when the definition is not excluded?
Also, above you say that CMAKE_MSVC_RUNTIME_LIBRARY
didn't work properly - doesn't that make the cmake_policy
line redundant? Did you properly clear the cache - or got it to work in a different way?
Sorry for the delay, looking forward to hearing from you.
Pablo
Hi,
__declspec
approach)TBB_USE_DEBUG
, which conflicts with the fact that you can perfectly well link against a release TBB lib, even when building Guc in full Debug (as long as the MSVC runtime lib is the release one for the Guc project) I've checked the TBB code that handles this TBB_USE_DEBUG
definition, and on Windows, it seems to be correctly defined based on the currently selected runtime lib. So I thought I could ignore it on Windows. I can double check that and make some more tests if I can find the time.CMAKE_MSVC_RUNTIME_LIBRARY
: I probably made a mistake the first time I checked, in fact it works fine when the policy is set.Edit: for the TBB_USE_DEBUG
thing, this is the relevant part of the tbb_config.h
file (edited for readability) :
#ifndef TBB_USE_DEBUG
# ifdef _DEBUG
# define __TBB_IS__DEBUG_EMPTY (__TBB_IS_MACRO_EMPTY(_DEBUG,IGNORED)==__TBB_MACRO_EMPTY)
# if __TBB_IS__DEBUG_EMPTY
# define TBB_USE_DEBUG 1
# else
# define TBB_USE_DEBUG _DEBUG
# endif
# else
# define TBB_USE_DEBUG 0
# endif
#endif
And the _DEBUG
which is checked depends on the MSVC runtime lib selection, see: https://docs.microsoft.com/en-us/cpp/c-runtime-library/debug?view=msvc-170
Hi Damien,
I just tested everything on MacOS and Windows. Unfortunately, I'm having some trouble with the TBB define change on Windows. To explore this further, I just wanted to ask: which command-line flags are you using to build USD? Which MSVC and Python versions are used?
Also, could you check if this line fixes the issue?
target_compile_definitions(libguc PRIVATE TBB_USE_DEBUG=0)
Pablo
Hi,
Hmmm weird for the TBB thing, I was pretty sure it would be ok on Windows... Anyway, here are the flags I'm using:
%python% %root%\sources\build_scripts\build_usd.py ^
--generator "Ninja" ^
--build-args openexr,-DCMAKE_DEBUG_POSTFIX="" ^
--alembic ^
--materialx ^
--openimageio ^
--verbose --verbose ^
--build %builddir% ^
--src %builddir%/src ^
--jobs 6 ^
%installdir%
Basically just a basic release build. The version of Visual Studio is 2017 x64, and I'm using Python 3.7 (although I doubt that one would make a difference :p)
I'll run some more tests to see how this define can be handled in a way that works both with "normal" builds (e.g. everyone's in the same config) and our way of doing things (e.g. using -DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreadedDLL
)
Edit: seems like --build-variant debug
fails on Windows currently: IlmBase/OpenEXR libs are built using a _d
suffix, which CMake scripts don't account for in usage (Alembic and USD both fail finding IlmBase/OpenEXR. Did you patch your version of USD to make in build in full debug?
Edit2: fixed the build by adding the --build-args openexr,-DCMAKE_DEBUG_POSTFIX="" ^
line, now I can run some tests :)
Sorry, I've been drowning in 3rd parties recently, I must have mixed things up in some way: the TBB define can be left alone (I removed my modification from the commit) It just prints some warnings during compilation, but builds fine and runs fine, in both full debug (guc + usd&deps) and partial debug (guc in debug, runtime&usd&deps in release)
That only leaves the auto link removal (not needed on Windows, no link errors on any mode, and if left, creates link errors in partial debug) and the hints to find OIIO and MaterialX from the USD root.
All this back and forth for such a small PR, I should have spend a bit more time testing things out, sorry.
Hi Damien - your points were definitely justified and I've addressed multiple issues you raised outside of this PR!
I know that it's a hassle to deal with different build configurations due to problems with caching and USD rebuilds taking quite a long time.
Your contributions are very valuable - and thank you for investing quite some of your time into debugging the issues!
I'll do some final testing in a few days when I have time again and get this merged.
Hello,
I'm not sure if you're open to pull requests, but here I am :p I'm wanted to use Guc to produce test USD assets from Polyhaven, but I had a few issues on Windows, so I thought I would contribute the fixes I did. If you have any comment, I'm happy to update the pull request (code convention, naming or whatever)
So basically there are 3 changes:
mx::Node::addInputsFromNodeDef
method. My guess is that you're using thedev
branch of USD which might have bumped the MaterialX version. So in this "fix" I just copied the implementation from MaterialX as a free function (fortunately it's just a helper that only use other public methods) but maybe this commit can be ignored if you don't want to bother with older USD releases.Sorry for the lengthy message, don't hesitate to tell me if there's anything I can do (especially the last commit: I can remove it if you prefer to use the dev branch and future v22.xx versions of USD)