CesiumGS / cesium-unity

Bringing the 3D geospatial ecosystem to Unity
https://cesium.com/platform/cesium-for-unity/
Apache License 2.0
358 stars 83 forks source link

Use vcpkg version of cesium-native #479

Closed kring closed 3 months ago

kring commented 3 months ago

There's a lot going on this PR. Here's a summary.

vcpkg

We're now using the vcpkg-ified version of cesium-native from CesiumGS/cesium-native#820.

Unity 2022.3.41f.

Previously we used 2021.3.13f1. This means we can actually compile and test all of our code, including CesiumCartographicPolygon which requires 2022.3. The plugin will still work with 2021.3 after building with 2022.3, but there is a chance we might miss compatibility problems with the older release. 2021.3 will be out of support as soon as Unity 6 is released, though, so it should be a very short-term problem.

This is probably a good change in general, but the immediate motivation for making the switch here was that 2021.3 uses an older version of the Android NDK that uses Clang 9, which doesn't have very good C++20 support. 2022.3 uses Clang 12 on Android, which is much better.

To use the new Unity version to do builds, I had to fix a longstanding bug where the build process was baking-in all code that came out of a Roslyn code generator. This was fine in 2021.3 because Reinterop was the only Roslyn code generator. In 2022.3 this is no longer true, and it was breaking the build.

C++ 20

The C++ code in Cesium for Unity (but not cesium-native) now target C++20. This is necessary to use swl-variant.

swl-variant

This is an alternate variant implementation that requires C++20 and has much better compile-time performance in terms of both CPU time and memory usage. Like I did with Unreal, I've only switched the variants in cesium-unity itself to use this implementation. cesium-native still uses std::variant, though it may be worthwhile to change that too at some point.

macOS builds now run on macOS 14 / ARM64

Previously we were stuck on macOS 12 and x64. With the reduced compile-time memory usage from switching to swl-variant, we're no longer prevented from using GitHub's Apple Silicon runners (which are weirdly memory constrained). This gets us onto the more modern platform (OS and processor architecture) and speeds up our builds.

Builds are faster

In main, builds took over 2 hours on Windows and almost an hour and a half on macOS. In this branch, total CI times on both platforms are cut approximately in half. This is a result of swl-variant, faster runners, and caching builds of third-party dependencies via vcpkg. I also modified the build process to immediately exit Unity after the native code finished building, saving a bunch of pointless work building a Player application that we don't actually need.

The improved build times are also a result of disabling Unity's audio system during our build. Incredibly, on macOS, Unity was doing...something...with audio for 20 minutes every time it started up, presumably because the GitHub runners can't actually do audio or somesuch. Disabling it massively reduced our macOS build times.

Native code build logs are uploaded as build artifacts

Previously, if the native code failed to compile on one of our many platforms, it was hard to figure out what went wrong. We had to add a bit of script to dump the log to the build output, and run the build again. Now, a build artifact is created with all of the native build logs, so they can easily be inspected if needed.

Consistent macOS and iOS version targetting

Previously, we were targeting macOS 12.7 in our Editor build of the plugin. And 10.13 for Player builds. Now, we're targetting 10.15 for both. We moved from 10.13 to 10.15 because 10.13 doesn't support std::filesystem::path.

~On iOS, we previously weren't attempting to specify a target version, so we were getting xcode's default (whatever that is). Now we're explicitly targetting iOS 12, which is the minimum version supported by Unity 2022.3.~ Update: While we weren't explicitly specifying a target before, we were (perhaps coincidentally) getting iOS 12 anyway.

Floating point comparisons

When we started running the tests on an Apple Silicon Mac, we initially had some test failures. Apple's ARM processors seem to be a bit less tolerant of testing floating point equality compare to x64 processors. So I added some epsilons to some comparisons in tests. I also changed CesiumFlyToController.DetectMovementInput to only allow movement of more than 1/1000 of a meter to cancel an in-progress flight.

CesiumIonSession crash

We have sometimes seen crashes related to CesiumIonSession. It became quite consistent when running the tests in Unity 2022.3. The crash was caused by a race condition where the managed CesiumIonSession was garbage collected (perhaps due to AppDomain unload) while it was still attempting to resume a previous session, resulting in a use-after-free. I fixed this by holding a reference to the managed object while requests are in flight, and checking when they complete that the managed object is still valid.

Fixes #432 (I think!)

kring commented 3 months ago

I'm struggling with the Android build, specifically with installing OpenSSL from vcpkg. Unity includes the Android NDK, which is installed to C:\Program Files\Unity\Hub\Editor\2021.3.13f1\Editor\Data\PlaybackEngines\AndroidPlayer\NDK by default. That path has a space in it, and OpenSSL's build process apparently can't handle that. It says:

-- Warning: Tools with embedded space may be handled incorrectly by configure:
   C:/Program Files/Unity/Hub/Editor/2021.3.13f1/Editor/Data/PlaybackEngines/AndroidPlayer/NDK/toolchains/llvm/prebuilt/windows-x86_64/bin/clang.exe
   C:/Program Files/Unity/Hub/Editor/2021.3.13f1/Editor/Data/PlaybackEngines/AndroidPlayer/NDK/toolchains/llvm/prebuilt/windows-x86_64/bin/clang++.exe
   C:/Program Files/Unity/Hub/Editor/2021.3.13f1/Editor/Data/PlaybackEngines/AndroidPlayer/NDK/toolchains/llvm/prebuilt/windows-x86_64/bin/llvm-nm.exe
   C:/Program Files/Unity/Hub/Editor/2021.3.13f1/Editor/Data/PlaybackEngines/AndroidPlayer/NDK/toolchains/llvm/prebuilt/windows-x86_64/bin/aarch64-linux-android-strip.exe
   C:/Program Files/Unity/Hub/Editor/2021.3.13f1/Editor/Data/PlaybackEngines/AndroidPlayer/NDK/toolchains/llvm/prebuilt/windows-x86_64/bin/llvm-objdump.exe
   C:/Program Files/Unity/Hub/Editor/2021.3.13f1/Editor/Data/PlaybackEngines/AndroidPlayer/NDK/toolchains/llvm/prebuilt/windows-x86_64/bin/aarch64-linux-android-ranlib.exe
   C:/Program Files/Unity/Hub/Editor/2021.3.13f1/Editor/Data/PlaybackEngines/AndroidPlayer/NDK/toolchains/llvm/prebuilt/windows-x86_64/bin/aarch64-linux-android-ar.exe
   C:/Program Files/Unity/Hub/Editor/2021.3.13f1/Editor/Data/PlaybackEngines/AndroidPlayer/NDK/toolchains/llvm/prebuilt/windows-x86_64/bin/ld.lld.exe
-- Warning: Arguments with embedded space may be handled incorrectly by configure:
   --gcc-toolchain=C:/Program Files/Unity/Hub/Editor/2021.3.13f1/Editor/Data/PlaybackEngines/AndroidPlayer/NDK/toolchains/llvm/prebuilt/windows-x86_64
   --sysroot=C:/Program Files/Unity/Hub/Editor/2021.3.13f1/Editor/Data/PlaybackEngines/AndroidPlayer/NDK/toolchains/llvm/prebuilt/windows-x86_64/sysroot

And then fails later with:

/bin/sh: line 1: C:/Program: No such file or directory
make[1]: *** [Makefile:4646: crypto/aes/libcrypto-lib-aes_cbc.o] Error 127

Usually a good strategy when dealing with broken software that can't handle spaces in paths is to use the old school 8.3 version of the path. So I tried setting the ANDROID_NDK_ROOT environment variable to C:/Progra~1/Unity/Hub/Editor/2021.3.13f1/Editor/Data/PlaybackEngines/AndroidPlayer/NDK. No more space. But that still doesn't work, because CMake "helpfully" rewrites it to the long path so it breaks in the same way. πŸ™„ CMake bug report here: https://gitlab.kitware.com/cmake/cmake/-/issues/25639

The original author closed it without a fix in CMake after they worked around it with some complicated code to create NTFS junction points. I'll probably end up doing the same.

By the way, this wasn't an issue before vcpkg because we avoided the need for an actual OpenSSL dependency by carving out the bits of s2geometry we actually need and leaving out the bits that depend on OpenSSL. Doing the same with vcpkg would be...tricky.

kring commented 3 months ago

Today's problem is that the Android build is failing only on CI. The reason appears to be that the GitHub runner has a recent version (v27) of the Android NDK installed, and the ANDROID_NDK_ROOT environment variable set accordingly. Because this environment variable is set, Unity doesn't override it with its own (older) copy of the Android NDK.

This alone wouldn't be a problem, except for this issue: https://github.com/android/ndk/issues/2032#issuecomment-2267532582

The linked issue says this will be addressed in NDK v28, but it's not completely clear this will fix the problem for us. Recent versions of the Android NDK's CMake toolchain require CMake 3.6, where's asyncplusplus only requires CMake 3.1. We're of course using a plenty-new CMake, but CMake has a whole policy system whereby newer versions behave like older ones based on the declared required version of the project. In any case, I'm going to try to force our builds to use Unity's copy of the NDK, which should fix the problem.

kring commented 3 months ago

Thanks for reviewing @j9liu, this is ready for another look.

kring commented 3 months ago

Final testing across all platforms:

❌ means I cannot test because I don't have access to the necessary hardware.

kring commented 3 months ago

I've done all the platform testing I can here, too, so this is ready.

j9liu commented 3 months ago

Thanks @kring ! Merging now 😎