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

Add SampleHeightMostDetailed tileset load failure test #511

Merged
merged 3 commits into from
Sep 30, 2024
Merged

Conversation

kring
Copy link
Member

@kring kring commented Sep 27, 2024

No description provided.

@kring kring added this to the October 2024 Release milestone Sep 28, 2024
Copy link
Contributor

@j9liu j9liu 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 ! I confirmed that the changes result in the future completing and an exception being thrown.

I noticed a slight difference in behavior between this and CesiumGS/cesium-unreal#1526. In Unreal, during the std::move(future).catchImmediately call, we initialize the result even when the promise fails. So it's possible to iterate over that array of falses and do something like print a string. But accessing task.Result causes the rest of the script to stop working due to an exception.

I can catch it of course, and save the rest of the application, but was wondering if this behavior is built into the way C# Tasks work? Or if it was worth achieving symmetry with Unreal, if possible. Not a big concern though, this can be merged otherwise.

@kring
Copy link
Member Author

kring commented Sep 30, 2024

Hmm this is interesting. So in cesium-native, if the tileset can't be sampled at all, we reject the future. Unity tasks have a similar mechanism to future rejection, so that's what we do there, too.

But in Unreal, all we have is an OnHeightsSampled callback, there's no separate error pathway. So instead we call that OnHeightsSampled and indicate that every height failed to sample. As I was doing this, I actually thought that Unreal was the compromise.

I can kind of argue either way, though. The question is: when the tileset is completely broken, and so no height samples will ever succeed, should we report this is "every height sample failed", or should we report it as a more dramatic error? Given that the latter is not possible in Unreal (short of making the user supply a separate error callback), maybe there's value in consistency across game engines.

@kring
Copy link
Member Author

kring commented Sep 30, 2024

The more I think about it, the more I like the idea that there's just one error mechanism, rather than two. So I'll switch Native and Unity to work like Unreal, as you suggested.

@kring
Copy link
Member Author

kring commented Sep 30, 2024

Ok that's done and this is ready.

@kring kring merged commit 455f2e7 into main Sep 30, 2024
5 checks passed
@kring kring deleted the height-query branch September 30, 2024 23:08
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.

2 participants