Closed MarkLeone closed 2 years ago
Mark, were you going to move ImageSource into its own project?
No, it seemed unlikely to have any use outside of DemandLoading, so I incorporated it as a subdirectory. I'd be glad to revisit that if you think there are good reasons to do otherwise.
There are two vestiges of the previous arrangement that might have caught your eye: there's a separate imageSource namespace, and the header files are "public", i.e. located in optix-toolkit/DemandLoading/include/ImageSource. I left things that way to minimize the differences between OTK and the SDK. Once we ship everything and create a release branch, I'll clean that up.
Some comments on the current state of the install:
#include
statements. If you want to support concurrently installed versions of a library and still have each version's headers, then I would recommend ${CMAKE_INSTALL_INCLUDEDIR}/OptiXToolKit/${VERSION}/OptiXToolKit
, where each target would have an INSTALL_INTERFACE
of ${CMAKE_INSTALL_INCLUDEDIR}/OptiXToolKit/${VERSION}
. This would let C++ code be forward compatible where possible (i.e. avoid needless version changes in include paths).add_subdirectory()
to build external dependencies can be very unwieldy and is generally not recommended for libraries (generally fine for executable projects, though). The reason is that each dependent project is written as a standalone install, meaning on make install
of optix-toolkit ends up doing an install for all the dependencies too. For example, a stock build currently installs headers for glad, GLFW, gmock, gtest, Imath, KHR, and OpenEXR. To make optix-toolkit friendlier to package managers, optix-toolkit should really only ever install itself and primarily rely on these dependencies to be imported (as opposed to built locally). For users that really want them also to be built the option maybe could exist, but as a default this ends up being quite messy. One tactic I've used is to provide a "superbuild" CMake script that is optional to use, where it chains a bunch of ExternalProject_Add()
commands together to build everything that is needed, but is ultimately optional to use. For example, all but glad is available as a standard package on Ubuntu or vcpkg for Windows.OTK_
, which will naturally group all the options that optix-toolkit has without colliding with any other variables introduced by dependent libraries. This is only really an issue when add_subdirectory()
is used to build external projects, but it's generally good practice to make your options more discoverable (usually in ccmake
or cmake-gui
). The exception to this rule is for built-in CMake options such as BUILD_SHARED_LIBS
or BUILD_TESTING
, because it is good for those options to propagate down each subdirectory/subproject uniformly and not need to go look in the source to see if they are setting the actual CMake vars correctly.lib/cmake
instead of just lib/
. It's such a strong convention I didn't know that lib/
was a standard option checked by find_package()
because I have never seen it before. This is a more picky suggestion, but at least worth mentioning.That's what I've got thus far...the exports and OptiXToolkitConfig
look good so far!
New updates are looking good! I do have one other item of feedback to consider:
CMAKE_INSTALL_INCLUDEDIR
, probably ${CMAKE_INSTALL_INCLUDEDIR}/OptiXToolkit
. This keeps everything installed by the project from ever colliding with other projects when installing to multi-library install roots (such as /usr
or /usr/local
). This is just a tweak to the destination of install()
commands which install headers.
Merge DemandLoading library from dev/mleone into optix-toolkit/master.