Conversation
Remove the mechanism to deliver GeoJson content directly to the client.
j9liu
requested review from
j9liu and
lilleyse
and removed request for
azrogers
|
|
||
| /** | ||
| * @brief Whether to translate vector feature data, e.g. GeoJSON content, to | ||
| * glTF, or leave it in its native format. | ||
| */ | ||
| bool translateVectorFeatures = true; |
There was a problem hiding this comment.
I believe this option can be removed now?
| @@ -1155,15 +1212,16 @@ TilesetJsonLoader::loadTileContent(const TileLoadInput& loadInput) { | |||
| } else { | |||
| // not a renderable content, then it must be external tileset | |||
There was a problem hiding this comment.
Maybe update this comment to something like:
| // not a renderable content, then it must be external tileset | |
| // Otherwise must be JSON content, either an external tileset or GeoJSON |
| ExternalContentInitializer&& externalContentInitializer, | ||
| const CesiumGeospatial::Ellipsoid& ellipsoid) { | ||
| const CesiumGeospatial::Ellipsoid& ellipsoid, | ||
| rapidjson::Document& tilesetJson) { |
There was a problem hiding this comment.
Since tilesetJson is later moved I think rvalue reference would make more sense here.
| rapidjson::Document& tilesetJson) { | |
| rapidjson::Document&& tilesetJson) { |
| const CesiumGeospatial::Ellipsoid& ellipsoid) { | ||
| const CesiumGeospatial::Ellipsoid& ellipsoid, | ||
| rapidjson::Document& tilesetJson) { | ||
| // create external tileset |
There was a problem hiding this comment.
This comment can be removed now that the parsing code has moved elsewhere
| const CesiumAsync::IAssetResponse* pResponse = pCompletedRequest->response(); | ||
| const auto& responseData = pResponse->data(); | ||
|
|
||
| rapidjson::Document tilesetJson; |
There was a problem hiding this comment.
Calling this variable tilesetJson might be misleading. Maybe simply json?
| using Point = std::array<double, 2>; | ||
| std::vector<std::vector<Point>> polygon; | ||
|
|
||
| for (const auto& ring : polygonIn) { | ||
| polygon.emplace_back(); | ||
| for (const auto& coord : ring) { | ||
| polygon.back().push_back(Point{{coord[0], coord[1]}}); | ||
| } | ||
| } |
There was a problem hiding this comment.
For performance, some of these vectors could be reserved in advance
| pbr.roughnessFactor = 1.0; | ||
| pbr.baseColorFactor[0] = 0.0; | ||
| pbr.baseColorFactor[1] = 0.0; | ||
| pbr.baseColorFactor[2] = 0.0; | ||
| material.doubleSided = true; | ||
| // International orange | ||
| material.emissiveFactor = {0xff / 255.0, 0x4f / 255.0, 0.0}; |
There was a problem hiding this comment.
Does setting the emissive have any impact on lighting in Unreal?
| pbr.roughnessFactor = 1.0; | ||
| pbr.baseColorFactor[0] = 0.0; | ||
| pbr.baseColorFactor[1] = 0.0; | ||
| pbr.baseColorFactor[2] = 0.0; |
There was a problem hiding this comment.
Also add pbr.baseColorFactor[3] = 1.0;?
There was a problem hiding this comment.
Never mind, 1.0 is the default for all channels. Still, might not hurt to be explicit.
| return bufferViewIndex; | ||
| } | ||
|
|
||
| int32_t GltfConverterImpl::makeAccessor( |
There was a problem hiding this comment.
These helper functions (makeBufferView, makeAccessor) could possibly be removed. Some areas call the helper functions but other areas just create the objects inline (which I think is clean enough).
There was a problem hiding this comment.
My intent is to use the helper functions throughout. The repetitive verbosity was driving me crazy .
| // GeoJson polygons contain an outer counter and possible inner contours. The | ||
| // first and last coordinates must be identical. An optimization would be to | ||
| // only include that coordinate once, but for now we will put both values into | ||
| // the big array of coordinates. |
There was a problem hiding this comment.
The implementation looks ok, but it would be good to get visual confirmation that holes are working. I found an example here: https://mapsam.com/geojson-ring-visual/
|
Oh and a meta comment, @timoore could you update the PR description now that the PR is further along? |
4 participants
Very early work