Skip to content

feat: support installing and finding filament via find_package #8267

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

aminya
Copy link
Contributor

@aminya aminya commented Nov 10, 2024

This allows installing the targets of Filament and finding it later via find_package(filament CONFIG). This makes usage of Filament very easy from other CMake projects.

For this to work, I had to differentiate the public include paths during build vs being used when filament is installed.

This is cherry-picked from my PR for allowing filament to be built via vcpkg:
microsoft/vcpkg#41916

@pixelflinger pixelflinger added the internal Issue/PR does not affect clients label Nov 11, 2024
@poweifeng
Copy link
Contributor

Closing due to this PR having been idle for a while. If you'd like to continue, would you mind first opening a feature request issue and we can have a discussion there to better understand the need?
(related PRs #8267 #8266 #8245)

@patrikhuber
Copy link

Hi @poweifeng,

The need is pretty clear and simple: Just having a proper filament-config/export.cmake file for the filament library. This is standard nowadays for all proper CMake/C++ libraries, and it makes it so much easier to use the library (filament in this case) in downstream projects. Not having it makes it a real hassle to integrate Filament into projects. Could this please be added?

Thanks a lot.

@poweifeng
Copy link
Contributor

poweifeng commented Mar 10, 2025

Hi @poweifeng,

The need is pretty clear and simple: Just having a proper filament-config/export.cmake file for the filament library. This is standard nowadays for all proper CMake/C++ libraries, and it makes it so much easier to use the library (filament in this case) in downstream projects. Not having it makes it a real hassle to integrate Filament into projects. Could this please be added?

Thanks a lot.

Thanks for the clarification. This a valid request. I think because it was introduced as part of a set of making vkpkg to work that we weren't sure how to proceed.

@poweifeng poweifeng reopened this Mar 10, 2025
@patrikhuber
Copy link

Hi @poweifeng,
The need is pretty clear and simple: Just having a proper filament-config/export.cmake file for the filament library. This is standard nowadays for all proper CMake/C++ libraries, and it makes it so much easier to use the library (filament in this case) in downstream projects. Not having it makes it a real hassle to integrate Filament into projects. Could this please be added?
Thanks a lot.

Thanks for the clarification. This a valid request. I think because it was introduced as part of a set of making vkpkg to work that we weren't sure how to proceed.

Thanks a lot for the reply and for being open to the improvements. I can see now how it wasn't "clear and simple", depending on the context one's in. Sorry if it came across as presumptuous.

I've had a closer look at the changes in this PR: I'm not a super-expert on CMake export targets but it looks to me like it's doing the minimum needed and it's doing it well following modern CMake best practices. This will be useful for anyone using Filament with CMake using find_package(filament CONFIG REQUIRED) which is sort of the best practice nowadays for most setups/people (I think filament is meant to be used as a submodule but this is not ideal for many setups). As a side-effect, it'll also help integration into vcpkg - but this PR is completely vcpkg-independent.

Btw I don't know the author of this PR, I'm just a "third party" who also had a painful experience integrating Filament into their CMake build in the past, and who would really like to see this improvement land, one way or another. :)

And thank you for all your work on this @aminya!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal Issue/PR does not affect clients
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants