-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
[open3d] add Open3D port #41844
base: master
Are you sure you want to change the base?
[open3d] add Open3D port #41844
Conversation
[open3d] do not apply the patch Update vcpkg.json
Update vcpkg.json
I want to already write some thanks for trying to finally resolve this long-standing issue <3 |
3fecfa4
to
3fdbb43
Compare
You're welcome! I am excited about this. It's one of the most requested packages on vcpkg based on the sheer number of issues that have been opened for it. |
Can you set this PR to draft until it is ready for review (or until you actually want review comments). |
I am ready to get review comments. I successfully built this on Ubuntu locally, and the rest is fixing the issues of the CI environment. |
"tbb", | ||
"tinygltf", | ||
"tinyobjloader", | ||
"uvatlas", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uvatlas is failing to be used on Windows with an error regarding missing DirectX headers. Have you tested UvAtlas on Windows recently inside CI?
-- Configuring done (21.5s)
CMake Error at D:/installed/x64-windows/share/uvatlas/UVAtlas-targets.cmake:60 (set_target_properties):
The link interface of target "Microsoft::UVAtlas" contains:
Microsoft::DirectX-Headers
but the target was not found. Possible reasons include:
* There is a typo in the target name.
* A find_package call is missing for an IMPORTED target.
* An ALIAS target is missing.
Call Stack (most recent call first):
D:/installed/x64-windows/share/uvatlas/uvatlas-config.cmake:27 (include)
D:/a/_work/1/s/scripts/buildsystems/vcpkg.cmake:859 (_find_package)
3rdparty/find_dependencies.cmake:293 (find_package)
3rdparty/find_dependencies.cmake:1537 (open3d_find_package_3rdparty_library)
CMakeLists.txt:503 (include)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tested UvAtlas on Windows recently inside CI?
If there is no port using it, then CI doesn't test for such issues.
(That's why I add extra test ports more often today.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it seems there are two possible fixes:
- Modify upstream CMakeLists.txt
if(directxmath_FOUND)
message(STATUS "Using DirectXMath package")
target_link_libraries(${PROJECT_NAME} PUBLIC Microsoft::DirectXMath)
endif()
if(directx-headers_FOUND)
message(STATUS "Using DirectX-Headers package")
target_link_libraries(${PROJECT_NAME} PUBLIC Microsoft::DirectX-Headers)
target_compile_definitions(${PROJECT_NAME} PUBLIC USING_DIRECTX_HEADERS)
endif()
Use PRIVATE
instead of PUBLIC
- Modify this port to include
directx-headers
as another dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since both of these libraries are header only, I think making these PRIVATE
is probably the simplest fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I applied this patch in #41878
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use
PRIVATE
instead ofPUBLIC
Doesn't help for static linkage.
Since both of these libraries are header only, I think making these
PRIVATE
is probably the simplest fix.
PRIVATE
makes no sense for header-only libs. Everything is INTERFACE
Modify this port to include
directx-headers
as another dependency.
No. It is the responsibilit of uvatlas
's CMake config to call find_dependency()
as needed to resolve imported targets.
@@ -3,7 +3,7 @@ | |||
{ | |||
"name": "linux-install-packages", | |||
"parameters": { | |||
"packages": "git curl zip unzip tar at libxt-dev gperf libxaw7-dev cifs-utils build-essential g++ gfortran libx11-dev libxkbcommon-x11-dev libxi-dev libgl1-mesa-dev libglu1-mesa-dev mesa-common-dev libxinerama-dev libxxf86vm-dev libxcursor-dev yasm libnuma1 libnuma-dev libtool-bin libltdl-dev flex bison libbison-dev autoconf libudev-dev libncurses5-dev libtool libxrandr-dev xutils-dev dh-autoreconf autoconf-archive libgles2-mesa-dev ruby-full pkg-config meson nasm cmake ninja-build libxext-dev libxfixes-dev libxrender-dev libxcb1-dev libx11-xcb-dev libxcb-dri3-dev libxcb-present-dev libxcb-glx0-dev libxcb-util0-dev libxkbcommon-dev libxcb-keysyms1-dev libxcb-image0-dev libxcb-shm0-dev libxcb-icccm4-dev libxcb-sync-dev libxcb-xfixes0-dev libxcb-shape0-dev libxcb-randr0-dev libxcb-render-util0-dev libxcb-xinerama0-dev libxcb-xkb-dev libxcb-xinput-dev libxcb-cursor-dev libkrb5-dev libxcb-res0-dev libxcb-keysyms1-dev libxcb-xkb-dev libxcb-record0-dev python3-setuptools python3-mako python3-pip python3-venv python3-jinja2 nodejs libwayland-dev python-is-python3 guile-2.2-dev libxdamage-dev libxtst-dev haskell-stack golang-go wayland-protocols libbluetooth-dev" | |||
"packages": "git curl zip unzip tar at libxt-dev gperf libxaw7-dev cifs-utils build-essential g++ gfortran libx11-dev libxkbcommon-x11-dev libxi-dev libgl1-mesa-dev libglu1-mesa-dev mesa-common-dev libxinerama-dev libxxf86vm-dev libxcursor-dev yasm libnuma1 libnuma-dev libtool-bin libltdl-dev flex bison libbison-dev autoconf libudev-dev libncurses5-dev libtool libxrandr-dev xutils-dev dh-autoreconf autoconf-archive libgles2-mesa-dev ruby-full pkg-config meson nasm cmake ninja-build libxext-dev libxfixes-dev libxrender-dev libxcb1-dev libx11-xcb-dev libxcb-dri3-dev libxcb-present-dev libxcb-glx0-dev libxcb-util0-dev libxkbcommon-dev libxcb-keysyms1-dev libxcb-image0-dev libxcb-shm0-dev libxcb-icccm4-dev libxcb-sync-dev libxcb-xfixes0-dev libxcb-shape0-dev libxcb-randr0-dev libxcb-render-util0-dev libxcb-xinerama0-dev libxcb-xkb-dev libxcb-xinput-dev libxcb-cursor-dev libkrb5-dev libxcb-res0-dev libxcb-keysyms1-dev libxcb-xkb-dev libxcb-record0-dev python3-setuptools python3-mako python3-pip python3-venv python3-jinja2 nodejs libwayland-dev python-is-python3 guile-2.2-dev libxdamage-dev libxtst-dev haskell-stack golang-go wayland-protocols libbluetooth-dev libc++-dev libc++abi-dev" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filament that's used internally for the GUI of Open3D needs libc++. Can we get these libraries included?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAICT there can be only one C++ standard lib in the whole triplet, and it already comes with the C++ compiler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain how it depends on a particular libc++?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Open3D relies on Filament which is provided by Google. Filament apparently relies on libc++. However, I suspect if we're not building from the source, it should work without a libc++. I haven't tested on a system that lacks libc++ yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, filament. Shouldn't it be packaged separately? With its own set of patches? Does it build at all with gcc (x64-linux CI)?
Past attempt, related to past open3d attempts: #17369.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, in a perfect world, Filament can be potentially ported as well, but I'd like to be realistic and pragmatic. Open3D has been blocked for years. Improvements can be done step by step.
Right now, the official binaries are used. I will test if they need libc++ during runtime
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, by not porting Filament, the port might just run into the ABI issues that de-vendoring and building from source tries to avoid...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filament works if the libc++ is linked. There are no abi issues. Also, this is private to Open3D, so no worries regarding that.
I will work on porting filament, but that will take a lot of time, and I don't want that to delay Open3D more
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that changes to managed-image.json need an image update. This is not part of the PR build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Started the Filament port here
#41916
Do you intent do upstream the patches? Some review comments will be about minimizing the patches for vcpkg, but that's not the right thing for upstream. |
Yes, that's my goal. I've tried to keep the patches generic so that they can be upstreamed. I'd like to get this merged here, then focus on upstreaming. Some of the patches for fmt 10/11 are already merged and will be available in the next release (whenever it might be). I have marked them explicitly in the code so we know later. |
I see the barrier of making filament a proper port. Ant this in addition to open3d itself: (That's almost the same pain as with other googlish ports (chromium, skia, tensorflow), but this time using CMake.) |
Started the Filament port here |
Hi @aminya, The seacas build errors complain about missing My system: |
Hey, I haven't tried building on Windows yet. It should work on Linux. There are some linked PRs before I can finalize this. |
Is there any new progress? |
If the maintainers want everything to be built with vcpkg, it would take a long time to port all the dependencies, but if we're ready to compromise, this PR is ready to go Right now, blocked by this PR: |
Got it, I'll wait for your changes. |
This adds Open3D.
It's the successor of #17423 and #12199.
Fixes isl-org/Open3D#5570
Fixes #27032
Fixes #15980
Fixes #16104
Fixes #14751
Fixes #12236
Fixes: #11950
Fixes: #6158
Fixes: #9172
find_package
calls are REQUIRED, are satisfied byvcpkg.json
's declared dependencies, or disabled with CMAKE_DISABLE_FIND_PACKAGE_Xxx.vcpkg.json
matches what upstream says.vcpkg.json
matches what upstream says../vcpkg x-add-version --all
and committing the result.