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

Update cesium-native, don't derive CesiumImpl from ReferenceCountedThreadSafe #547

Merged
merged 7 commits into from
Jan 31, 2025

Conversation

kring
Copy link
Member

@kring kring commented Jan 23, 2025

The only change here is to CesiumImpl, necessitated by a change to ReferenceCounted.

CesiumGS/cesium-native#1048 slightly changed how we do CRTP in ReferenceCounted. See a bit of discussion about it here:
CesiumGS/cesium-native#1048 (comment)

This is a good change, but unfortunately it breaks the way we were using it in Unity. In Unity, before this PR, we were deriving CesiumImpl from ReferenceCountedThreadSafe, like this:

template <typename TDerived>
class CesiumImpl : public CesiumUtility::ReferenceCountedThreadSafe<TDerived> {
  // ...
}

So essentially CesiumImpl is a CRTP class itself. It's essential that TDerived be given as the template parameter to ReferenceCountedThreadSafe because, when the reference count goes to zero, ReferenceCountedThreadSafe static casts to that type before deleting it. The destructor is not virtual (by design) so we must cast to the fully-derived type in order to properly delete the object.

Unfortunately, this doesn't work after CesiumGS/cesium-native#1048. CesiumImpl isn't allowed to call the private constructor in ReferenceCountedThreadSafe because it is not a friend; only TDerived is. We get compiler errors because the ReferenceCountedThreadSafe constructor is not accessible.

I went around in circles a bit on how to solve this. We could go back to the old CRTP approach with a protected (instead of private) constructor, and disable the clang-tidy check that got us here. Perhaps us a static_assert to achieve something close to the safety that the private constructor / friend approach was getting us (but I can't see how to get all the way there). We could add another template parameter to ReferenceCountedThreadSafe, so that separate classes can be specified as the "derived" and the "friend". But then couldn't that be used to subvert the safety that the private constructor is meant to provide?

In the end, I decided to leave cesium-native alone and change CesiumImpl to no longer derive from ReferenceCountedThreadSafe. This is a somewhat unusual case, so I guess it's worth a bit of duplication to get what we need here, plus the safety of the private constructor in more common scenarios.

@kring kring added this to the February 2025 Release milestone Jan 31, 2025
@kring
Copy link
Member Author

kring commented Jan 31, 2025

Merging this to unbreak main.

@kring kring merged commit 852ebae into main Jan 31, 2025
4 checks passed
@kring kring deleted the update-native branch January 31, 2025 12:16
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.

1 participant