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

[open3d] add Open3D port #41844

Draft
wants to merge 31 commits into
base: master
Choose a base branch
from
Draft

[open3d] add Open3D port #41844

wants to merge 31 commits into from

Conversation

aminya
Copy link
Contributor

@aminya aminya commented Oct 29, 2024

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

  • Changes comply with the maintainer guide.
  • The name of the port matches an existing name for this component on https://repology.org/ if possible, and/or is strongly associated with that component on search engines.
  • Optional dependencies are resolved in exactly one way. For example, if the component is built with CMake, all find_package calls are REQUIRED, are satisfied by vcpkg.json's declared dependencies, or disabled with CMAKE_DISABLE_FIND_PACKAGE_Xxx.
  • The versioning scheme in vcpkg.json matches what upstream says.
  • The license declaration in vcpkg.json matches what upstream says.
  • The installed as the "copyright" file matches what upstream says.
  • The source code of the component installed comes from an authoritative source.
  • The generated "usage text" is accurate. See adding-usage for context.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is in the new port's versions file.
  • Only one version is added to each modified port's versions file.

@aminya aminya marked this pull request as draft October 29, 2024 08:45
@MonicaLiu0311 MonicaLiu0311 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Oct 29, 2024
@aminya aminya marked this pull request as ready for review October 29, 2024 10:40
@CampingAvocado
Copy link

I want to already write some thanks for trying to finally resolve this long-standing issue <3

@aminya aminya force-pushed the open3d branch 2 times, most recently from 3fecfa4 to 3fdbb43 Compare October 31, 2024 07:28
@aminya
Copy link
Contributor Author

aminya commented Oct 31, 2024

I want to already write some thanks for trying to finally resolve this long-standing issue <3

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.

@dg0yt
Copy link
Contributor

dg0yt commented Oct 31, 2024

Can you set this PR to draft until it is ready for review (or until you actually want review comments).

@aminya
Copy link
Contributor Author

aminya commented Oct 31, 2024

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",
Copy link
Contributor Author

@aminya aminya Oct 31, 2024

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)

Copy link
Contributor

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.)

Copy link
Member

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:

  1. 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

  1. Modify this port to include directx-headers as another dependency.

Copy link
Member

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.

Copy link
Member

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Use PRIVATE instead of PUBLIC

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"
Copy link
Contributor Author

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?

Copy link
Contributor

@dg0yt dg0yt Oct 31, 2024

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.

Copy link
Contributor

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++?

Copy link
Contributor Author

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.

https://github.com/isl-org/Open3D/blob/0f06a149c4fb9406fd3e432a5cb0c024f38e2f0e/3rdparty/find_dependencies.cmake#L1363-L1376

Copy link
Contributor

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.

Copy link
Contributor Author

@aminya aminya Oct 31, 2024

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

Copy link
Contributor

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...

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor Author

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

@dg0yt
Copy link
Contributor

dg0yt commented Oct 31, 2024

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.

@aminya
Copy link
Contributor Author

aminya commented Oct 31, 2024

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.

@dg0yt
Copy link
Contributor

dg0yt commented Nov 2, 2024

I see the barrier of making filament a proper port.
I just warn that not making it a proper port could be a blocker for open3d.
https://github.com/google/filament/tree/main/third_party

Ant this in addition to open3d itself:
https://github.com/isl-org/Open3D/tree/main/3rdparty

(That's almost the same pain as with other googlish ports (chromium, skia, tensorflow), but this time using CMake.)

@aminya
Copy link
Contributor Author

aminya commented Nov 2, 2024

Started the Filament port here
#41916

@MonicaLiu0311 MonicaLiu0311 marked this pull request as draft November 15, 2024 08:06
@jordanlui
Copy link

Hi @aminya,
I've cloned your open3d fork and attempted to have vcpkg find open3d in a simple cmake test project, but vcpkg is failing to install seacas, which is a dependency of VTK.
Do you use a particular vcpkg baseline or configuration when you install this open3d port?

The seacas build errors complain about missing PYTHON_EXECUTABLE and CACHE variable _Python_EXECUTABLE_DEBUG, even though I have set these in Windows env vars and also tried setting them in CMakeLists.txt.

My system:
Win 11
VS: 2022/Community/VC/Tools/MSVC/14.33.31629

@aminya
Copy link
Contributor Author

aminya commented Dec 4, 2024

Hey, I haven't tried building on Windows yet. It should work on Linux. There are some linked PRs before I can finalize this.

@MonicaLiu0311
Copy link
Contributor

Is there any new progress?

@aminya
Copy link
Contributor Author

aminya commented Jan 16, 2025

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:
#41916

@MonicaLiu0311
Copy link
Contributor

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: #41916

Got it, I'll wait for your changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR!
Projects
None yet
6 participants