Allow custom styling for VectorTilesRasterOverlay. - #1430
Conversation
…ative into vector-overlay-styling
| }; | ||
| } // namespace CesiumVectorData | ||
|
|
||
| /** @brief Hash implementation for \ref CesiumVectorData::ColorStyle. */ |
There was a problem hiding this comment.
Prefer @ref according to the style guide, and apply this to the other instances of \ref in this doc too.
| /** @brief Hash implementation for \ref CesiumVectorData::ColorStyle. */ | |
| /** @brief Hash implementation for @ref CesiumVectorData::ColorStyle. */ |
| * @brief Draws a set of points to the canvas. | ||
| * | ||
| * @param points The set of points to draw. | ||
| * @param style The @ref PointStyle to use when drawing the points. |
There was a problem hiding this comment.
| * @param style The @ref PointStyle to use when drawing the points. | |
| * @param styles The @ref PointStyle instances to use when drawing the points. |
There was a problem hiding this comment.
Also, it seems like styles.size() must equal points.size(), where style[i] is applied to points[i]. Perhaps we should call it out here.
| const std::vector<CesiumGeospatial::Cartographic>& points, | ||
| const std::vector<const CesiumVectorData::VectorStyle*>& styles) { | ||
| if (this->_finalized) { | ||
| return; | ||
| } | ||
|
|
||
| for (size_t i = 0; i < points.size(); i++) { | ||
| BLPoint point = radiansToPoint( | ||
| points[i].longitude, | ||
| points[i].latitude, | ||
| this->_bounds, | ||
| this->_context); | ||
| // clang-tidy does not understand that we *are* in fact checking these | ||
| // optionals | ||
| if (styles[i]->point.fill) { |
There was a problem hiding this comment.
It seems like there's some assumptions here:
styles.size() == points.size()styles[i] != nullptrfor allistyles[i].point.hasValue()
We should add if statements to guard for these, or at least add asserts.
| /** | ||
| * @brief An interface for providing styling information for vector features. | ||
| */ | ||
| class CESIUMVECTOROVERLAYS_API VectorStylingProvider { |
There was a problem hiding this comment.
Add a unit test for this, to confirm that a simple style can be successfully applied?
| const CesiumGltf::FeatureId* pFeatureIdSet = | ||
| pMeshFeatures != nullptr && !pMeshFeatures->featureIds.empty() | ||
| ? &pMeshFeatures->featureIds[0] | ||
| : nullptr; |
There was a problem hiding this comment.
This could be simplified using model.getSafe.
| }); | ||
|
|
||
| return {pContent, errors}; | ||
| // Apply per-element styling if the user specified a styling provider. |
There was a problem hiding this comment.
Move this comment down after the if (!pStylingProvider) ?
| const CesiumUtility::Result<int64_t> featureIdResult = | ||
| getFeatureId(pFeatureIdSet, model, primitive, startIndex); | ||
| errors.merge(featureIdResult.errors); | ||
| polylineFeatureIds.emplace_back( | ||
| featureIdResult.value.value_or(-1)); | ||
| pContent->polylines.emplace_back(std::move(polyline)); | ||
| polyline.clear(); | ||
| startIndex = -1; |
There was a problem hiding this comment.
Wonder if there's a nice way to reorganize this so there aren't two polyline.size() >=2 checks... but doesn't have to be in scope for this PR.
| for (const auto& style : result) { | ||
| if (style.has_value()) { | ||
| pContent->pointStyles.emplace_back( | ||
| &*pContent->uniqueStyles.insert(*style).first); |
There was a problem hiding this comment.
NIT: For readability help? (Same with the others in the file)
| &*pContent->uniqueStyles.insert(*style).first); | |
| &(*pContent->uniqueStyles.insert(*style).first)); |
| vectorizationResult) mutable { | ||
| if (vectorizationResult.errors.hasErrors()) { | ||
| if (vectorizationResult.value) { | ||
| delete *vectorizationResult.value; |
There was a problem hiding this comment.
I see this was already in the code, but for the future -- would be good to use smart pointers instead or otherwise document any specific reasons for why we're not using them.
| if (!pVectorContent->points.empty()) { | ||
| rasterizer.drawPoints( | ||
| pVectorContent->points, | ||
| pVectorContent->style.point); | ||
| pVectorContent->pointStyles); |
There was a problem hiding this comment.
NIT: why do points have their own multi-style override when polylines/polygons use for loops?
3 participants
This PR allows the user to specify a
VectorStylingProviderthat will be called to obtian styling information for the primitives loaded by theVectorTilesRasterOverlay. This allows per-element styling with a great degree of flexibility.