Rasterize Point and MultiPoint in GeoJsonDocumentRasterOverlay#1400
Conversation
baruchInsert-tech
force-pushed
the
geojson-document-overlay-points
branch
from
270d0bb to
c884b03
Compare
Point and MultiPoint geometry was silently dropped by the quadtree build: the bounding-box visitor returned no rectangle for them, so they never reached the rasterizer (which already knows how to draw them via VectorRasterizer::drawGeoJsonObject -> drawPoints). Make the Point/MultiPoint visitor operators produce a degenerate bounding rectangle at each coordinate and pad the tile's pixel footprint by the point's diameter (plus outline width), mirroring the existing line-width padding. Points then flow through the existing machinery and are rasterized as filled/outlined circles using VectorStyle::point. Adds render tests for Point, outlined Point, and MultiPoint, and a CHANGES.md entry.
baruchInsert-tech
force-pushed
the
geojson-document-overlay-points
branch
from
c884b03 to
f1f83a5
Compare
|
Hi @baruchInsert-tech, sorry for the delay in getting around to this! Can you merge in |
baruchInsert-tech
force-pushed
the
geojson-document-overlay-points
branch
from
012a0b0 to
8056aa2
Compare
Done! Thank U! |
j9liu
left a comment
j9liu
left a comment
There was a problem hiding this comment.
Thanks for the PR @baruchInsert-tech! I can confirm that points now render as a result of this PR, though there is a slight issue with outline rendering (see the comments).
Let me know when these comments are addressed -- and please push the commits regularly, without --force, so that the diff is more obvious upon review. Thank you!
| // A point's pixel footprint is its radius (plus its outline width) in | ||
| // every direction. Encode that as an equivalent line width (the full | ||
| // diameter) so the shared pixel padding math below applies to it too. | ||
| activeLineStyle.width = 2.0 * this->pStyle->point.radius; | ||
| if (this->pStyle->point.outline) { | ||
| activeLineStyle.width += this->pStyle->point.outline->width; | ||
| } | ||
| activeLineStyle.widthMode = LineWidthMode::Pixels; |
There was a problem hiding this comment.
Is it correct to hardcode this as Pixels?
I noticed that if you set Width Mode to meters, and the meters is greater than the point radius, then the points appear like this:
This doesn't seem correct, though I can't tell from a glance which line of code is the culprit.
There was a problem hiding this comment.
I appreciate the new unit test / changes for this, but this sadly wasn't fixed...
However, I did test whether point outlines work for VectorTilesRasterOverlay, and it appears to be a problem there too. This does point to a more underlying mistake that can be out of scope for this PR. At the same time, we'd really like for a human to verify the results of any PR changes before we get to it, to avoid this back-and-forth!
There was a problem hiding this comment.
Thanks!
Given you flagged it as reasonably out of scope, I've narrowed this PR back to the verified change , Point/MultiPoint now rasterize and reverted the bounds-padding attempt, which only affected tile coverage and never touched the actual rendering. I opened a separate issue for the shared meters-mode point-outline bug in VectorRasterizer.
#1413
And point taken on verifying renders before pushing, that's on me; I'll eyeball the output next time rather than round-tripping through review.
Wrap the west/east lines to satisfy the column limit, matching the adjacent south/north lines. Resolves the Quick Checks formatting failure on PR CesiumGS#1400. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
j9liu
left a comment
j9liu
left a comment
There was a problem hiding this comment.
Looks good, thanks @baruchInsert-tech !
3 participants
Summary
CesiumVectorOverlays::GeoJsonDocumentRasterOverlaydid not actually rasterizePointandMultiPointgeometry, even though the changelog for v0.61.0 states that it can display points. This PR makes the document overlay genuinely render points, fixing that latent gap.The problem
The rasterizer already knows how to draw points:
VectorRasterizer::drawGeoJsonObjectroutesGeoJsonPoint/GeoJsonMultiPointtodrawPoints(..., style.point). The geometry just never got there.When
GeoJsonDocumentRasterOverlaybuilds its quadtree,addPrimitivesToDatarunsRectangleAndLineWidthFromObjectVisitorto compute a bounding rectangle for each object and only stores objects that produce one (if (rect)). The visitor'soperator()forGeoJsonPointandGeoJsonMultiPointwere empty, so points yielded no rectangle and were dropped before ever reaching the rasterizer.The fix
GeoJsonPoint/GeoJsonMultiPointvisitor operators so they build a (degenerate) bounding rectangle at each coordinate and bumpmaxLineWidthPixelsby the point's pixel footprint (2 * radius, plus the outline width when outlined), mirroring the existing line-width padding.Point/MultiPointbranch toQuadtreeGeometryData::calculateBoundingRectangleForTileSizeso per-tile bounds are padded by that same pixel footprint (otherwise points fell into the line branch and were sized bystyle.line).addPrimitivesToDatathat claimed points are intentionally not rasterized.Points then flow through the existing machinery and are rasterized as filled/outlined circles using
VectorStyle::point. No new drawing code was needed.Tests
Added three render tests to
TestGeoJsonDocumentRasterOverlay.cpp, each rasterizing an in-code document and asserting the expected colors appear:Pointrendered as a filled circle (fill color present).Pointwith an outline (both fill and outline colors present).MultiPoint(fill color present).All three pass locally (RelWithDebInfo), and
CesiumVectorOverlays+cesium-native-testsbuild clean.Changelog
Added a Fixes entry under the unreleased section. Note the v0.61.0 entry already (incorrectly) claimed the document overlay could display points; this PR makes that true.
issue #1403
@azrogers
@j9liu