MAP-16: only render what’s visible #690
Conversation
Signed-off-by: Nicolas Märki <[email protected]>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes introduce new visibility management methods for render objects across multiple layers of the codebase. New abstract methods Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Interface as RenderObjectInterface (CppProxy)
participant JNI as JNI Bridge
participant Native as Native Layer
Client->>Interface: setHidden(flag)
Interface->>Interface: Assert object not destroyed
Interface->>JNI: Call native_setHidden(flag)
JNI->>Native: Execute native visibility update
Native-->>JNI: Return result
JNI-->>Interface: Pass result
Interface-->>Client: Acknowledge updated hidden state
sequenceDiagram
participant Renderer as Renderer
participant RenderObj as RenderObject
Renderer->>RenderObj: isHidden()
alt RenderObj is hidden
Renderer->>Renderer: Skip rendering operations
else RenderObj is visible
Renderer->>RenderObj: Perform rendering preparation
Renderer->>RenderObj: Render object
end
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
shared/src/graphics/RenderObject.cpp (1)
31-37: Implementation of visibility methods looks good.The implementation is straightforward and correctly manages the hidden state via the member variable.
Consider adding validation or logging in a debug build if this visibility state has implications for performance or rendering behavior.
shared/src/map/layers/tiled/vector/sourcemanagers/Tiled2dMapVectorSourceVectorTileDataManager.cpp (1)
147-150: Efficient filtering for out-of-zoom-range tilesThis condition optimizes performance by skipping the processing of tiles that fall outside of the layer's defined zoom range, with an exception for tiles that match the source's maximum zoom level.
Consider extracting this zoom range check into a separate method for better readability and reusability, especially if similar checks exist elsewhere in the codebase.
-if ((tile->tileInfo.tileInfo.zoomIdentifier < layer->minZoom || - tile->tileInfo.tileInfo.zoomIdentifier > layer->maxZoom) && tile->tileInfo.tileInfo.zoomIdentifier != layer->sourceMaxZoom) { - continue; -} +if (!isInZoomRange(tile->tileInfo.tileInfo.zoomIdentifier, layer)) { + continue; +} // Add this helper method to the class +bool Tiled2dMapVectorSourceVectorTileDataManager::isInZoomRange(int zoomIdentifier, const std::shared_ptr<VectorLayerDescription>& layer) { + return (zoomIdentifier >= layer->minZoom && + zoomIdentifier <= layer->maxZoom) || + zoomIdentifier == layer->sourceMaxZoom; +}shared/src/graphics/Renderer.cpp (2)
51-52: Initializepreparedmore clearly (optional).Defining the
preparedvariable is a good approach to ensure stencil and scissor operations are only applied once per pass. However, consider adding a brief comment or renaming the variable to clarify its purpose (e.g.,renderPreparationDone) for improved readability.
90-98: Symmetry in post-render logic.The conditional un-application of scissoring and stencil operations looks correct. Ensure other render states or resources influenced by
renderingContext->setupDrawFrame()also do not require cleanup if no object is rendered.shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonPatternTile.h (1)
81-81: Visibility state naming.Having a tile-level
isVisibleboolean is consistent, but note that theRenderObjectInterfaceusesisHidden()as the inverted concept. Consider aligning naming or clarifying if tile visibility differs from object-level visibility.shared/src/map/layers/tiled/vector/Tiled2dMapVectorLayerParserHelper.cpp (3)
362-371: Refactor magic defaults for line layers.This block sets local
int minZoom = 0, maxZoom = 24;by default and then overwrites them if a matching source is found. Consider logging or error-handling when the source is missing to avoid silent fallback.
435-445: Avoid duplicating min/max zoom logic for symbol layers.The approach of iterating through
sourceDescriptionsto find matchingminZoom/maxZoomis repeated in multiple blocks. A small helper function could eliminate duplication and improve maintainability.
470-478: Consistent min/max zoom for fill layers.Similar to line and symbol layers, adding an explicit error message or fallback logic if no matching source is found for fill layers could improve debugging and consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/RenderObjectInterface.kt(2 hunks)bridging/android/jni/graphics/NativeRenderObjectInterface.cpp(2 hunks)bridging/android/jni/graphics/NativeRenderObjectInterface.h(2 hunks)bridging/ios/MCRenderObjectInterface+Private.mm(2 hunks)bridging/ios/MCRenderObjectInterface.h(1 hunks)djinni/graphics/core.djinni(2 hunks)shared/public/RenderObject.h(1 hunks)shared/public/RenderObjectInterface.h(1 hunks)shared/src/graphics/RenderObject.cpp(1 hunks)shared/src/graphics/Renderer.cpp(2 hunks)shared/src/map/layers/tiled/vector/Tiled2dMapVectorLayerParserHelper.cpp(4 hunks)shared/src/map/layers/tiled/vector/sourcemanagers/Tiled2dMapVectorSourceVectorTileDataManager.cpp(1 hunks)shared/src/map/layers/tiled/vector/tiles/line/Tiled2dMapVectorLineTile.cpp(8 hunks)shared/src/map/layers/tiled/vector/tiles/line/Tiled2dMapVectorLineTile.h(2 hunks)shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonPatternTile.cpp(4 hunks)shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonPatternTile.h(1 hunks)shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonTile.cpp(3 hunks)shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonTile.h(2 hunks)shared/src/map/layers/tiled/vector/tiles/raster/Tiled2dMapVectorRasterTile.cpp(1 hunks)shared/src/map/layers/tiled/vector/tiles/raster/Tiled2dMapVectorRasterTile.h(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (35)
djinni/graphics/core.djinni (1)
61-62: Well-designed visibility control methods added to render_object_interface.The addition of
set_hiddenandis_hiddenmethods provides a clean way to control visibility of render objects across all platforms. This is a good design choice that maintains consistency with the interface's existing style.bridging/ios/MCRenderObjectInterface.h (1)
18-21: Auto-generated visibility methods look good.The Objective-C interface correctly includes the new visibility control methods that were defined in the Djinni specification.
shared/public/RenderObjectInterface.h (1)
22-24: LGTM: Pure virtual methods for visibility control.The pure virtual methods properly establish the contract that all implementing classes must provide visibility control functionality.
shared/public/RenderObject.h (2)
33-35: Added visibility control methodsThese methods provide a clean way to control the visibility of render objects, which aligns with the PR objective of only rendering what's visible.
42-42: Added hidden state memberThis boolean field correctly tracks the visibility state with a sensible default (visible).
shared/src/map/layers/tiled/vector/tiles/raster/Tiled2dMapVectorRasterTile.h (1)
55-55: Simplified visibility trackingReplacing an optional boolean with a direct boolean flag simplifies the state management and improves code readability.
shared/src/map/layers/tiled/vector/tiles/raster/Tiled2dMapVectorRasterTile.cpp (1)
69-77: Efficient visibility management implementationThis implementation properly updates the render object's visibility state when the zoom range changes and prevents unnecessary processing when the tile is out of the zoom range.
The early return when the tile is not visible is an excellent optimization that prevents unnecessary style calculations and updates.
shared/src/graphics/Renderer.cpp (2)
55-60: Confirm behavior with hidden objects.Skipping hidden objects early is beneficial for performance. Verify that external code does not rely on any side-effects (e.g., uniform updates, logging, or debugging) that might need to be run even for hidden objects.
61-75: Ensure correct setup on first visible object.Applying the scissor rect, stencil mask, and mask object rendering only once is a solid optimization. Just confirm that if a pass has all hidden objects, the logic of not calling these operations remains correct (i.e., no additional cleanup is necessary).
shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonPatternTile.h (1)
74-75: Consider safe usage ofrenderObjects.Storing render objects in a
std::vector<std::shared_ptr<RenderObjectInterface>>is straightforward. Confirm that no concurrency or reference cycle issues arise, and ensure the vector is kept in sync with tile lifecycle events (e.g., clearing or invalidation).shared/src/map/layers/tiled/vector/Tiled2dMapVectorLayerParserHelper.cpp (1)
213-246: Validate presence of tile arrays before indexing.When constructing
VectorMapSourceDescription, the code immediately doestileJson["tiles"].begin()->get<std::string>(). Confirm that eachtileJsondefinitely contains a non-emptytilesarray to avoid potential out-of-bounds or undefined behavior.bridging/android/jni/graphics/NativeRenderObjectInterface.h (2)
40-41: Functionality added to improve visibility management.The addition of these abstract methods enhances the RenderObjectInterface with visibility controls, which aligns with the PR objective of only rendering what's visible.
52-53: Method IDs correctly defined with proper JNI signatures.The JNI method IDs are correctly defined with proper signatures:
(Z)Vfor setHidden (boolean parameter, void return)()Zfor isHidden (no parameters, boolean return)shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonTile.h (2)
66-66: Added collection to track render objects.The addition of a vector to store render objects will enable direct management of visibility through the newly added RenderObjectInterface methods.
75-75: Simplified visibility tracking.Replacing
lastInZoomRange(std::optional) with a direct boolean flagisVisiblesimplifies the visibility state management and aligns with the new visibility control approach.shared/src/map/layers/tiled/vector/tiles/line/Tiled2dMapVectorLineTile.h (2)
63-63: Added collection to track render objects consistently.Similar to the polygon tile implementation, this new vector enables direct management of render objects' visibility. This maintains consistency in the visibility management approach across different tile types.
73-73: Simplified visibility tracking.Replacing the optional boolean with a direct
isVisibleflag simplifies the state management and makes the visibility control more straightforward, matching the approach used in other tile classes.bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/RenderObjectInterface.kt (2)
19-21: Added visibility control methods to the interface.These new abstract methods provide the necessary API for controlling render object visibility from Kotlin/Java code.
62-72: Properly implemented visibility methods in CppProxy.The implementation follows the established pattern:
- Checks if the object has been destroyed before proceeding
- Delegates to the native implementation
- Includes appropriate error handling
This ensures proper bridging between the Java and C++ layers.
shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonTile.cpp (4)
88-97: Add visibility state management for zoom-based conditional rendering.The code now properly controls the visibility state of render objects based on the zoom range. This optimization prevents render objects from being processed unnecessarily when they're not in the visible zoom range.
114-116: Simplified property retrieval logic.Removed conditional checks against
inZoomRangewhen retrieving properties. This is a good simplification since the early return above (line 95) already handles the zoom range check.Also applies to: 122-122
325-332: Centralized render object creation in addPolygons method.Moving the render object creation to the
addPolygonsmethod improves code organization by ensuring render objects are updated whenever polygons change, rather than deferring creation to when they're requested.
357-359: Simplified generateRenderObjects to return cached collection.Now that render objects are properly maintained when polygons are added or modified, the method can simply return the existing collection instead of creating new objects on each call.
shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonPatternTile.cpp (4)
91-100: Add visibility state management for zoom-based conditional rendering.This change mirrors the implementation in the
Tiled2dMapVectorPolygonTileclass, ensuring consistent behavior across different tile types. The early return when outside the zoom range is an effective optimization.
121-124: Simplified opacity calculation.Removed conditional checks against
inZoomRangewhen calculating opacity. This is appropriate since the early return above (line 98) already handles the zoom range check.
333-340: Centralized render object creation in addPolygons method.Similar to the change in
Tiled2dMapVectorPolygonTile, this ensures render objects are updated whenever polygons change. This approach provides better encapsulation of the render object lifecycle.
366-368: Simplified generateRenderObjects to return cached collection.Consistent with the changes in other tile types, this simplifies the method to return the existing collection rather than creating new objects on each call.
bridging/ios/MCRenderObjectInterface+Private.mm (2)
63-74: Add visibility state management methods to iOS interface.The implementation properly handles boolean type conversion between Objective-C and C++ and includes appropriate exception handling. These methods support the visibility management feature added to the tile rendering classes.
113-125: Add ObjcProxy implementations for visibility methods.The proxy implementation correctly bridges between Objective-C and C++, maintaining the same pattern as other methods in the class. This provides a consistent interface for visibility management across platforms.
shared/src/map/layers/tiled/vector/tiles/line/Tiled2dMapVectorLineTile.cpp (4)
90-100: Add visibility state management for zoom-based conditional rendering.This implementation mirrors the changes in the polygon tile classes, ensuring a consistent approach to visibility management across different tile types. The early return optimization prevents unnecessary processing when tiles are outside the visible zoom range.
124-125: Simplified property calculation logic.Removed conditional checks against
inZoomRangewhen retrieving various line properties (color, opacity, blur, width, dash array, offset). This is appropriate since the early return above (line 98) already handles the zoom range check.Also applies to: 134-135, 141-142, 156-157, 163-164, 196-197
410-417: Centralized render object creation in addLines method.This change ensures render objects are updated whenever lines are added or modified. Moving this logic to the
addLinesmethod provides better encapsulation of the render object lifecycle.
444-446: Simplified generateRenderObjects to return cached collection.Consistent with the changes in other tile types, this simplifies the method to return the existing collection rather than creating new objects on each call.
bridging/android/jni/graphics/NativeRenderObjectInterface.cpp (2)
50-65: Correctly implemented JavaProxy methods for visibility controlThe implementation of
setHiddenandisHiddenfollows the established pattern for JavaProxy methods in this file, with proper JNI environment handling, exception checking, and type conversion between C++ and Java.
110-125: JNI export functions properly implementedThe native JNI export functions for
setHiddenandisHiddenare correctly implemented, following the same pattern as other methods in this file. They properly handle reference conversion, exception translation, and type marshaling between Java and C++.
Summary by CodeRabbit
New Features
Refactor