Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NGFX CMake Nabla build system integration #789

Merged
merged 2 commits into from
Dec 26, 2024

Conversation

AnastaZIuk
Copy link
Member

No description provided.

…creating interface `ngfx` interface target on success, allow for picking found versions
@AnastaZIuk AnastaZIuk mentioned this pull request Nov 20, 2024
@devshgraphicsprogramming
Copy link
Member

@keptsecret merge this into your branch

@keptsecret keptsecret merged commit f3c777e into ngfx-sdk-integration Dec 26, 2024
1 check failed
@keptsecret keptsecret deleted the ngfx-si-cmake branch December 26, 2024 03:11
Comment on lines +662 to +675
# NGFX
if(TARGET ngfx)
if(NBL_STATIC_BUILD)
target_link_libraries(Nabla INTERFACE ngfx)
else()
target_link_libraries(Nabla PRIVATE ngfx)
endif()

target_include_directories(Nabla PRIVATE $<TARGET_PROPERTY:ngfx,INTERFACE_INCLUDE_DIRECTORIES>)
target_compile_definitions(Nabla
PRIVATE NBL_BUILD_WITH_NGFX
PRIVATE $<TARGET_PROPERTY:ngfx,INTERFACE_COMPILE_DEFINITIONS>
)
endif()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AnastaZIuk why is there an ngfx target cmake knows about?

we dont do this for Vulkan, CUDA, Optix, we delay load from DLL and are not supposed to crash if not found

Copy link
Member Author

@AnastaZIuk AnastaZIuk Dec 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AnastaZIuk why is there an ngfx target cmake knows about?

because I introduce modern cmake there as I do on newBuildSystem branch, interface targets don't have build rules but you can interact with them

we dont do this for Vulkan, CUDA, Optix

it's actually not quite true, for example when you call find_package(Vulkan) you get imported targets if found which are similar to interface targets and because there is no existing ngfx module which I can request I wrote a simple one myself just to embed whole logic into single logical target (it could also be imported target just like vulkan targets - doesn't matter too much). The only difference is the module is integral part of Nabla's cmake build system and we don't explicitly call the find_package on it but have an option to make it available for configure scope if possible.

If the target is available it means:

  • someone enabled NBL_BUILD_WITH_NGFX
  • the SDK with the injection header has been found and passed a simple validation

we delay load from DLL and are not supposed to crash if not found

and what makes you think it will crash? We do delay load from DLL here as well & we have m_loaded member to make sure we can call the API only if we first loaded the DLL

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok thanks for explaining it to me

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but whats the deal with target_link_libraries and the ngfx being an INTERFACE instead of PRIVATE?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but whats the deal with target_link_libraries and the ngfx being an INTERFACE instead of PRIVATE?

the reason for

if(NBL_STATIC_BUILD)
  target_link_libraries(Nabla INTERFACE ngfx)
else()
  target_link_libraries(Nabla PRIVATE ngfx)
endif()

is that in static build mode each executable must link against the import library - using the INTERFACE qualifier ensures this linkage is propagated.

On the other hand when building Nabla as a DLL the PRIVATE qualifier is used to prevent any inheritance of ngfx linkage for targets linking Nabla since the import library is already linked directly to the DLL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants