-
Notifications
You must be signed in to change notification settings - Fork 317
Add support for loading and interacting with GeoJSON data in Blueprint #1651
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @azrogers! Lots of small comments below, but this looks really good!
return {}; | ||
} | ||
|
||
return (ECesiumGeoJsonObjectType)InObject._pObject->getType(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given this conversion via cast, it would add a bit of safety to add static assertions that the enum values are equivalent. That way a change on either side (other than adding a new enum value at the end) would be flagged as a compiler error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you mean. How would we assert that the enums are the same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can find an example of it here:
https://github.com/CesiumGS/cesium-unreal/blob/v2.16.1/Source/CesiumRuntime/Private/Cesium3DTileset.cpp#L1015
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. I've added it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to jump on right after @kring 's review -- apparently I had started a review a while ago, but didn't see that I forgot to submit the comments until now when I was looking through Kevin's 🤦 I removed some duplicate feedback, so most of these are just doc / style fixes at this point
Source/CesiumRuntime/Private/Tests/CesiumVectorDocument.spec.cpp
Outdated
Show resolved
Hide resolved
#include "CesiumUtility/IntrusivePointer.h" | ||
#include "CesiumVectorData/GeoJsonDocument.h" | ||
#include "CesiumVectorData/GeoJsonObject.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
angle brackets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we've decided on what we're doing about this in Unreal. The Cesium Native style is de facto to use angle brackets (though we've yet to write it down anywhere), but our Unreal code uses quotes a lot more than angle brackets, and all the code from Epic uses quotes instead of angle brackets.
#include "CesiumGeoJsonDocument.h" | ||
#include "CesiumRuntime.h" | ||
|
||
#include "CesiumUtility/Result.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#include "CesiumUtility/Result.h" | |
#include <CesiumUtility/Result.h> |
@@ -0,0 +1,412 @@ | |||
#include "CesiumGeoJsonObject.h" | |||
|
|||
#include "CesiumGeospatial/Cartographic.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#include "CesiumGeospatial/Cartographic.h" | |
#include <CesiumGeospatial/Cartographic.h> |
#include "CesiumUtility/IntrusivePointer.h" | ||
#include "CesiumVectorData/GeoJsonDocument.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#include "CesiumUtility/IntrusivePointer.h" | |
#include "CesiumVectorData/GeoJsonDocument.h" | |
#include <CesiumUtility/IntrusivePointer.h> | |
#include <CesiumVectorData/GeoJsonDocument.h> |
There's a lot of "vector document" in a bunch of other places as well, including filenames and parameter names (which matter because they're shown as output in names in Blueprint). Can I bug you to do a pass over the code to replace all those occurrences, and then I'll look for any that might have been missed while I'm reviewing? |
@kring I believe I've caught all of them, let me know if you find any others. |
Thanks @azrogers! |
The companion to CesiumGS/cesium-native#1154. This PR adds blueprint functions for loading and interacting with GeoJSON data. In particular, it provides the ability to load GeoJSON from a string or from Cesium ion, the ability to query nodes, and the ability to obtain primitives as values usable in Unreal.
One note is that access to the properties of a vector node requires blueprint functionality for interacting with JSON objects. I elected to push this responsibility onto the JSON Blueprint Utilities plugin, which is an official Unreal plugin but not enabled by default. Returning the
FJsonObjectWrapper
struct from a method is supported without this plugin enabled, but users will have to enable that plugin if they want to be able to do anything with it.