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

Move Abseil code in s2geometry to an isolated namespace. #892

Merged
merged 1 commit into from
May 29, 2024

Conversation

kring
Copy link
Member

@kring kring commented May 27, 2024

This builds on #891 so merge that first.

This PR moves the Abseil functionality embedded in the s2geometry library into an isolated namespace (cesium_s2geometry_absl). This avoids linker errors when packaging an Unreal Engine 5.4 game that includes both the Cesium for Unreal and Pixel Streaming plugins, because the Pixel Streaming plugin also embeds some Abseil code.

Fixes CesiumGS/cesium-unreal#1420

@Immersiv-1
Copy link

Hi @kring, with the update. Is there a way we can integrate or access? Thanks

@kring
Copy link
Member Author

kring commented May 27, 2024

Base automatically changed from ios-fix to main May 28, 2024 14:00
@csciguy8 csciguy8 self-requested a review May 28, 2024 20:29
Copy link
Contributor

@csciguy8 csciguy8 left a comment

Choose a reason for hiding this comment

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

Thanks @kring !

In general, this seems ok, especially if we want a quick fix for users encountering this problem. I tested out with cesium-unreal, and all seems fine.

It does feel a little hacky to force absl to a different value using a compile definition. My only worry would be potential build problems in other situations that aren't obvious (although I can't think of any). Ideally, s2geometry would have some kind of official configuration to solve problems like this.

That said, I did notice that our s2geometry version is a little old (from March 2019). And there have been some later PRs related to Abseil that might help this issue (ex. this one, or this one). Could be worth updating, now or in the future.

@kring
Copy link
Member Author

kring commented May 29, 2024

I agree it could be worth upgrading @csciguy8, but I don't think either of those s2geometry PRs will help us. They would allow us to use an external Abseil, but I don't think that helps.

We can't supply our own Abseil, because Unreal will still have its own, and we'd still have the conflict.

We can't use Unreal's Abseil (even if using libraries from Unreal in cesium-native were easy, which it's not) because Unreal doesn't appear to include headers and an independent library for it. It's just baked into the WebRTC library. In theory we could figure out what version of Abseil Unreal uses, and use the headers for the same version. But we'll still have challenges at link time. In a packaged build where WebRTC and Cesium for Unreal are baked into a single executable, webrtc.lib includes Abseil, and so as long as we're compiled against the right version, everything is fine. However, in a regular Editor build, WebRTC and Cesium for Unreal are in separate DLLs, and the Abseil symbols are almost certainly not exported from the WebRTC DLL. So even if we made Cesium for Unreal depend on WebRTC (which we don't want to do), we'd still need to somehow link against Abseil in a non-packaged build in order to avoid undefined symbol linker errors.

So I think the approach in this PR is the best available. Using the preprocessor to rename a namespace is a little hacky, sure, but it's simple and effective, and tightly scoped to just s2geometry (the #define absl doesn't exist except while we're building s2geometry). Theoretical problems we might have with it in the future (such as an unrelated symbol named absl getting inadvertently renamed) are likely to manifest as compiler or linker errors, so it should be relatively safe as well.

@csciguy8 csciguy8 merged commit 3992963 into main May 29, 2024
24 checks passed
@csciguy8 csciguy8 deleted the abseil-conflict branch May 29, 2024 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Linker error when packaging when the Cesium and Pixel Streaming plugins are both enabled
3 participants