DiligentGraphics / DiligentEngine

A modern cross-platform low-level graphics library and rendering framework
http://diligentgraphics.com/diligent-engine/
Apache License 2.0
3.63k stars 331 forks source link

Build failure with FetchContent #278

Closed QuantumFelidae closed 10 months ago

QuantumFelidae commented 10 months ago

Using

The relevant parts of my CMakeLists

include(FetchContent)
FetchContent_Declare(
    d_core
    GIT_REPOSITORY https://github.com/DiligentGraphics/DiligentCore.git
    GIT_TAG 48f3e615ababafcdfcc0430dd6e7c9ab9c324b25
)

FetchContent_MakeAvailable(d_core)

target_link_libraries(main PRIVATE 
                        Diligent-GraphicsEngineVk-shared)

This successfully configures, and adds the correct targets. However, I am experiencing the following build error:

out\build\x64-debug\_deps\d_core-src\Common\interface\ObjectsRegistry.hpp(26): fatal error C1083: Cannot open include file: '../../../DiligentCore/Platforms/Basic/interface/DebugUtilities.hpp': No such file or directory

I have confirmed that this file exists, but I am doing an out of source build**, so I assume there is some base CMake issue going on here with the use of relative paths. I believe my error would be fixed if the Vk-shared target had the necessary headers "target_include"d instead of using the relative path for this file. I, however, am unfamiliar with the entirety of your build system, and wouldn't know how to go about testing this.

** This is the preferred default for FetchContent - there is a source folder and a build folder within a _deps folder automatically created in the build location of your main target

TheMostDiligent commented 10 months ago

This path has been fixed recently - try fetching the latest version

QuantumFelidae commented 10 months ago

Is there a stable commit you would recommend? Or is HEAD good enough?

TheMostDiligent commented 10 months ago

Head is always stable

QuantumFelidae commented 10 months ago

Great, my build is succeeding, thank you for the prompt assistance.

QuantumFelidae commented 10 months ago

I'll reopen/move this to the tools page as needed, but there is a similar issue with attempting to build the imgui backend - all of the hpp files use relative includes to get access to files from core. I don't know what that would take to fix, so if there is a recommended way to just use the backend header files with a current install of upstream IMGUI, I'd be happy to copy over the backend files to my project and make any modifications I need to get it to compile. (the provided one is five versions behind, and does not seem to correctly detect my existing version as it still target includes the headers from the provided old version).

TheMostDiligent commented 10 months ago

You can provide your own ImGui by specifying DILIGENT_DEAR_IMGUI_PATH

TheMostDiligent commented 10 months ago

Also if you need all submodules, you can fetch the main repo, which will preserve the folder structure

QuantumFelidae commented 10 months ago

Right, I saw that option. The issue is that the rest of the cmake project uses the vcpkg package manager for everything other than diligent - so I can't perfectly set the path in an easy way (everything is target based, and paths are nicely abstracted away). Looking at the diligent-imgui target, the target linking of the backends to the provided imgui only occurs if there does not already exist a imgui target. This is good, as I can simply create an CMake alias library for targets provided by find_package(imgui CONFIG REQUIRED), and my imgui target will be linked instead. However, the target include located directly below it is not wrapped in such a conditional, and thus the headers from the provided imgui and the imgui alias are both provided.

I am aware of the submodule option as opposed to using fetch content, but I'd like to keep that as a last resort for now - currently the cmake and manifest file specify all third party dependencies, and allow the project's dependencies to be specified, updated, added, and removed without any dependency on (or contamination of) the git version control system.

I'll likely make a copy of the backends that I need and place them in the project itself for now with some CMake changes for now. I'm happy to close this issue (unless you want it open for the relative file paths affecting out of source builds).

Thanks for the help and enjoy your New Year!

TheMostDiligent commented 10 months ago

However, the target include located directly below it is not wrapped in such a conditional, and thus the headers from the provided imgui and the imgui alias are both provided.

I am not sure I got what causes the problem here - can you clarify?

QuantumFelidae commented 10 months ago

I've created a small working example of what I would like to be able to do with a target based version of the Dear Imgui backend. It can be seen here: https://github.com/QuantumFelidae/Diligent-Imgui

It will only work on win32 (I assume), but that's only because I didn't bother to add handling for the various other dependencies other platforms have.

I've also fixed the relative paths issue and fixed an include ordering issue within imguizmo.quat. I am happy to open a PR with these changes against the main tools repository, if interested, let me know

While I do understand the current suggested use of submodules and the dependencies on relative paths (and am very aware of the effort involved in everything else), I find myself generally not a fan of the reliance on a version control system to do package management.

As someone that has used vcpkg/contributed a port/failed to contribute a more complicated port, and has read the associated issue in this repository (#40), I would agree that vcpkg is not currently worth the effort. I am however, a large fan of the power of cmake's FetchContent module, and attempt to display that in the readme in the linked repository, and would be invested in seeing a DiligentEngine that fully works within this system. Given that I can already build DiligentCore and link it against my targets in effectively two lines of CMake (not including line breaks), I think this is a pretty approachable goal. Effectively, this would just require that all of Diligent can be built out of source. As a bonus, some of the features in this article could be included to make the engine even nicer to consume.

TheMostDiligent commented 10 months ago

Whether to use relative paths in the headers is really a rhetorical question. There are also multiple ways source code can be distributed. Currently, Diligent headers are organized in a way that after installing or cloning the repos, any header can be included into a project without adding any include directories, and it will compile. Some of our clients asked it to be this way. In particular for projects that don't use CMake, this approach simplified integration significantly and makes the project future-proof as adding new dependencies will not break the build.

Currently, all public headers use relative paths, so there will be the same issue with them. Changing headers in ImGuiDiligentRenderer.hpp will make it inconsistent with the rest of the API.

As a workaround for the relative paths problem, I can suggest adding a dummy path, which will make relative paths work, e.g.:

target_include_directories(Diligent-Imgui PRIVATE ${d_core_SOURCE_DIR}/_/_/_)

It is certainly a hack, but not too intrusive and it is only a single line.

QuantumFelidae commented 10 months ago

I understand, thanks for taking the time to write this up.

While I did not end up using the dummy path, it was a useful tool.

As such, I have written a very small CMake tool here which seems to do what I was looking for, pulling from remote repository and linking like so:

cmake_minimum_required (VERSION 3.26)

project ("Sample")
list(APPEND CMAKE_MODULE_PATH "<PATH>")
set(FETCH_DILIGENT_TOOLS TRUE)
include(FetchDiligent)

add_executable (main ${MAIN_SOURCES})

target_link_libraries(main PRIVATE 
                        Diligent-Imgui
                        Diligent-AssetLoader 
                        Diligent-GraphicsEngineVk-shared)

There is very little CMake in the implementation, but I figured I would share it for anyone who wanted something similar.


However, the target include located directly below it is not wrapped in such a conditional, and thus the headers from the provided imgui and the imgui alias are both provided.

I am not sure I got what causes the problem here - can you clarify?

If I understand the CMake correctly -- and please correct me if I am wrong!

If imgui is a preexisting target, using the local imgui is prevented with the following chunk in DiligentTools/Imgui/CMakeLists.txt

if(TARGET imgui)
    target_link_libraries(Diligent-Imgui PRIVATE imgui)
else()
    ... setup local imgui ...
endif()

However, directly beneath this is the chunk

target_include_directories(Diligent-Imgui
PUBLIC
    interface
    ${IMGUIZMO_QUAT_PATH}
    ${DILIGENT_DEAR_IMGUI_PATH} <----- 
PRIVATE
    include
)

target_link_libraries uses the defined INTERFACE_INCLUDE_DIRECTORIES of the library and will include them. This is populated for any package manager generated imgui target. As such, the second include below will make a second set of headers available. Because imgui is linked privately while the local headers are included publicly, the exposed headers in the transitive linkage from Diligent-Imgui to main will only be the local Diligent provided ones while the source will be the preexisting imgui target. Thus the header and the implementation can be different versions. This will lead to issues with API changes, and prevent newer versions from being used correctly.

Thank you for your time.

TheMostDiligent commented 10 months ago

As such, the second include below will make a second set of headers available.

You are correct, this does seem incorrect. I posted a fix

As such, I have written a very small CMake tool here which seems to do what I was looking for

Changing source directory for the fetch is actually a very good solution. Thanks for finding it - I will add this to readme.

QuantumFelidae commented 10 months ago

Great! Thank you for taking the time to assist me with this!