Conversation
|
Thank you so much for the contribution @baruchInsert-tech! Just to confirm, would you like a review on this PR? I see that it's marked "Draft", so we weren't sure if it was still in progress. |
Hello! |
Hi @j9liu , is there any update on this PR review? |
|
Hi @asafMasa, sorry for the delay. Our team hasn't had bandwidth in the past few weeks, but I definitely want someone to take a look by next week at the latest. |
j9liu
left a comment
j9liu
left a comment
There was a problem hiding this comment.
Hi @baruchInsert-tech, sorry about the delay on this review!
I didn't get to everything today, but I think I got through most of the important parts. It looks like there's a lot of comments but it's mostly cleanup and questions about API design decisions that were made. I'm planning to do a second pass after this first one is addressed!
| /// </summary> | ||
| /// <param name="url">The URL to load the GeoJSON from.</param> | ||
| /// <returns>A task that resolves to the loaded CesiumGeoJsonDocument, or null if loading failed.</returns> | ||
| public static async Task<CesiumGeoJsonDocument> LoadFromUrlAsync(string url) |
There was a problem hiding this comment.
Do you have an example of how to use this async action? I've only had experience with Unity Coroutines, so I'm a little unfamiliar with using async/await directly. :o
There was a problem hiding this comment.
In Unity, you can use async/await directly in MonoBehaviour methods. For example:
async void Start()
{
var document = await CesiumGeoJsonDocument.LoadFromUrlAsync("https://example.com/data.geojson");
if (document != null && document.IsValid())
{
var root = document.GetRootObject();
// Style features, then assign to overlay
overlay.document = document;
}
}
Under the hood, LoadFromUrlAsync wraps the callback-based LoadFromUrl in a TaskCompletionSource, so the await resumes on the main thread once the native async load completes. it works the same as coroutines but with a more concise syntax.
…ent structure - TestCesiumGeoJsonObject: tests for child access, feature IDs, properties, and per-feature styling - TestCesiumVectorStyle: tests for CesiumColor32, line styles, polygon styles, and style configuration
…or direct feature pointers
…n = 1, FromUrl = 2.
j9liu
left a comment
j9liu
left a comment
There was a problem hiding this comment.
Thanks @baruchInsert-tech for the quick turnaround! I have another round of feedback but it's mostly cleanup.
Feel free to push additional commits instead of force pushing the branch; it helps us identify the diffs more easily for next time :)
…Polygon to a pure C# class with a Rings property
…instead of 0 for the fallback, matching Cesium for Unreal.
… CesiumGeoJsonFeature
|
@j9liu All new comments were addressed and done! |
There was a problem hiding this comment.
Thanks @baruchInsert-tech again for the quick changes! This is looking great, Just a few extra pointers:
- #665 is merged, so you can remove
CesiumColor32now. - The C++ files need some formatting to pass/run on CI.
- The
.metafiles for the new classes can be pushed to this branch. We include them in our history because it helps with version control and future compatibility issues.
We could probably merge this today once all these points are addressed. Thanks again!
@j9liu Ive changed and addressed all of the comments. |
j9liu
left a comment
j9liu
left a comment
There was a problem hiding this comment.
Thanks @baruchInsert-tech, this looks great! I'm just waiting for CI to pass before we merge, but I don't expect there to be any changes you have to do.
3 participants
Description
Issue number or link
Author checklist
CHANGES.mdwith a short summary of my change (for user-facing changes).Remaining Tasks
Testing plan