CesiumGS / cesium-unreal

Bringing the 3D geospatial ecosystem to Unreal Engine
https://cesium.com/platform/cesium-for-unreal/
Apache License 2.0
921 stars 295 forks source link

Update FeatureId::propertyTable usage to fit with cesium-native changes #1431

Closed azrogers closed 4 months ago

azrogers commented 4 months ago

FeatureId::propertyTable is now no longer an std::optional, which requires a few small changes.

j9liu commented 4 months ago

Thanks @azrogers ! I'll wait for CI to pass before I merge.

j9liu commented 4 months ago

Unfortunately it seems like iOS isn't happy with the recent cesium-native changes :/

It seems like these are the culprits:

/Users/runner/work/cesium-unreal/cesium-unreal/extern/cesium-native/CesiumUtility/src/Uri.cpp:270:38: error: 'path' is unavailable: introduced in iOS 13.0
  if constexpr (std::filesystem::path::preferred_separator == '\\') {
                                     ^
In file included from /Users/runner/work/cesium-unreal/cesium-unreal/extern/cesium-native/CesiumUtility/src/Uri.cpp:9:
/Applications/Xcode_14.2.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS16.2.sdk/usr/include/c++/v1/filesystem:909:24: note: 'path' has been explicitly marked unavailable here
class _LIBCPP_TYPE_VIS path {
                       ^
/Users/runner/work/cesium-unreal/cesium-unreal/extern/cesium-native/CesiumUtility/src/Uri.cpp:322:38: error: 'path' is unavailable: introduced in iOS 13.0
  if constexpr (std::filesystem::path::preferred_separator == '\\') {
                                     ^
In file included from /Users/runner/work/cesium-unreal/cesium-unreal/extern/cesium-native/CesiumUtility/src/Uri.cpp:9:
/Applications/Xcode_14.2.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS16.2.sdk/usr/include/c++/v1/filesystem:909:24: note: 'path' has been explicitly marked unavailable here
class _LIBCPP_TYPE_VIS path {
                       ^
2 errors generated.
kring commented 4 months ago

I fixed the above by updating cesium-native to inclue CesiumGS/cesium-native#891, so merge that first.

kring commented 4 months ago

This PR now updates cesium-native to include CesiumGS/cesium-native#892 too.

csciguy8 commented 4 months ago

This looks ready to go. I'll merge unless there are objections...