only render what’s visible #664
Conversation
Signed-off-by: Nicolas Märki <maerki@ubique.ch>
maerki
requested review from
stmitt,
maurhofer-ubique,
zimmermannubique and
Copilot
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
Files not reviewed (20)
- bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/RenderObjectInterface.kt: Language not supported
- bridging/android/jni/graphics/NativeRenderObjectInterface.cpp: Language not supported
- bridging/android/jni/graphics/NativeRenderObjectInterface.h: Language not supported
- bridging/ios/MCRenderObjectInterface+Private.mm: Language not supported
- bridging/ios/MCRenderObjectInterface.h: Language not supported
- djinni/graphics/core.djinni: Language not supported
- shared/public/RenderObject.h: Language not supported
- shared/public/RenderObjectInterface.h: Language not supported
- shared/src/graphics/RenderObject.cpp: Language not supported
- shared/src/graphics/Renderer.cpp: Language not supported
- shared/src/map/layers/tiled/vector/Tiled2dMapVectorLayerParserHelper.cpp: Language not supported
- shared/src/map/layers/tiled/vector/sourcemanagers/Tiled2dMapVectorSourceVectorTileDataManager.cpp: Language not supported
- shared/src/map/layers/tiled/vector/tiles/line/Tiled2dMapVectorLineTile.cpp: Language not supported
- shared/src/map/layers/tiled/vector/tiles/line/Tiled2dMapVectorLineTile.h: Language not supported
- shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonPatternTile.cpp: Language not supported
- shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonPatternTile.h: Language not supported
- shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonTile.cpp: Language not supported
- shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonTile.h: Language not supported
- shared/src/map/layers/tiled/vector/tiles/raster/Tiled2dMapVectorRasterTile.cpp: Language not supported
- shared/src/map/layers/tiled/vector/tiles/raster/Tiled2dMapVectorRasterTile.h: Language not supported
WalkthroughThis pull request introduces a comprehensive enhancement to the rendering system by adding visibility management capabilities across multiple platforms and components. The changes implement Changes
Sequence DiagramsequenceDiagram
participant Client
participant RenderObject
participant Renderer
Client->>RenderObject: setHidden(true)
RenderObject-->>RenderObject: Update hidden state
Renderer->>RenderObject: isHidden()
RenderObject-->>Renderer: Return hidden state
alt If hidden
Renderer->>Renderer: Skip rendering
end
Possibly Related PRs
Suggested Reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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: 5
🧹 Nitpick comments (10)
shared/src/map/layers/tiled/vector/Tiled2dMapVectorLayerParserHelper.cpp (1)
362-370: Refactor repeated code for fetching minZoom and maxZoomThe logic for determining
minZoomandmaxZoomby iterating oversourceDescriptionsis repeated multiple times in the codebase. Consider refactoring this into a helper function to reduce code duplication and enhance maintainability.shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonPatternTile.cpp (1)
104-107: Simplify conditional statement for better readabilityThe compound conditional expression is complex and may reduce code readability. Consider refactoring the condition to enhance clarity.
You might restructure the condition as follows:
bool zoomCondition = (isStyleZoomDependant && *lastZoom == zoomIdentifier) || !isStyleZoomDependant; if (lastZoom && zoomCondition && !isStyleStateDependant) { // Existing code }shared/public/RenderObject.h (1)
32-35: Add documentation for the new visibility methods.The implementation looks good, but these public interface methods would benefit from documentation explaining their purpose and behavior.
Add documentation like this:
/// Sets the visibility state of the render object /// @param hidden true to hide the object, false to show it virtual void setHidden(bool hidden) override; /// @return true if the render object is currently hidden, false otherwise virtual bool isHidden() override;shared/src/graphics/RenderObject.cpp (1)
31-33: Consider parameter naming consistency.The parameter name
_hiddenuses a different naming convention compared to other methods in this file. Consider using the same style as other methods for consistency.-void RenderObject::setHidden(bool _hidden) { - hidden = _hidden; +void RenderObject::setHidden(bool hidden_) { + hidden = hidden_;shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonTile.h (2)
66-66: Add documentation for render objects managementConsider adding documentation to explain:
- The lifecycle of render objects
- When and how they are populated
- Their relationship with the visibility state
66-66: Consider adding visibility update methodTo maintain encapsulation and ensure proper state management, consider adding a method to update both
isVisibleand handle the visibility ofrenderObjectsconsistently.Example implementation:
+ void setVisible(bool visible) { + isVisible = visible; + for (auto& renderObject : renderObjects) { + renderObject->setHidden(!visible); + } + }Also applies to: 75-75
djinni/graphics/core.djinni (1)
61-62: Consider adding return type for set_hiddenThe visibility control methods are well-designed, but
set_hiddenshould explicitly specify its return type for consistency.Update the method signature:
- set_hidden(hidden: bool); + set_hidden(hidden: bool): void; is_hidden(): bool;shared/src/graphics/Renderer.cpp (1)
51-75: LGTM! Efficient implementation of visibility checks.The implementation correctly optimizes rendering by:
- Early skipping of hidden objects
- Performing preparation steps only once when needed
- Properly managing scissor rectangles and stencil masks
Consider fixing the indentation in the
preparedblock to match the surrounding code style:- if (!prepared) { - if (scissoringRect) { - renderingContext->applyScissorRect(scissoringRect); - } + if (!prepared) { + if (scissoringRect) { + renderingContext->applyScissorRect(scissoringRect); + }shared/src/map/layers/tiled/vector/tiles/line/Tiled2dMapVectorLineTile.h (1)
73-73: Consider adding documentation for isVisibleWhile the boolean flag is a good simplification from the previous optional value, it would benefit from a brief comment explaining its role in visibility management.
+ /// Indicates whether the tile is currently visible based on zoom range bool isVisible = true;shared/src/map/layers/tiled/vector/tiles/raster/Tiled2dMapVectorRasterTile.cpp (1)
90-90: Consider adding null check for rasterStyleWhile the code looks good, it might be worth adding a null check for
rasterStylebefore comparing withlastStyle.+ if (!rasterStyle) { + return; + } if(rasterStyle == lastStyle) {
📜 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)
👮 Files not reviewed due to content moderation or server errors (2)
- shared/src/map/layers/tiled/vector/sourcemanagers/Tiled2dMapVectorSourceVectorTileDataManager.cpp
- shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonTile.cpp
🔇 Additional comments (31)
shared/src/map/layers/tiled/vector/Tiled2dMapVectorLayerParserHelper.cpp (5)
440-445: Handle case where source description is not found
470-478: Handle case where source description is not found
378-379: Confirm correct usage of minZoom and maxZoom
The minZoom and maxZoom parameters are passed appropriately to the constructor, ensuring the layer respects the source's zoom levels.
451-452: Confirm correct usage of minZoom and maxZoom
484-485: Confirm correct usage of minZoom and maxZoom
shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonPatternTile.cpp (3)
91-96: Visibility state correctly updated for render objects
The code appropriately updates the isVisible flag and sets the hidden state of each render object based on inZoomRange, ensuring that render objects are only visible when they should be.
333-339: Efficiently create render objects without redundant allocations
The code efficiently creates new render objects and updates the renderObjects vector, ensuring that only necessary objects are kept in memory.
367-367: Correctly return the render objects
The generateRenderObjects method now correctly returns the renderObjects vector, ensuring that the rendering system receives the updated list.
shared/src/map/layers/tiled/vector/tiles/line/Tiled2dMapVectorLineTile.cpp (11)
78-80: Ensure correct zoom identifier for 2D maps
The adjustment of zoomIdentifier for non-3D maps is appropriate, ensuring that the zoom level is correctly calculated.
90-96: Visibility state correctly updated for render objects
The code correctly updates the isVisible flag and sets the hidden state of each render object based on inZoomRange, maintaining consistency in visibility management.
97-99: Early return when not in zoom range
The early return when !inZoomRange enhances performance by skipping unnecessary computations.
Line range hint 124-129: Efficiently update line color only when necessary
The color is updated only if it has changed, which optimizes performance by avoiding unnecessary updates.
134-136: Properly apply opacity with global alpha
The opacity calculation correctly incorporates the global alpha value, ensuring consistent transparency across render objects.
140-143: Optimize blur updates
The blur parameter is updated only when it changes, which is efficient and prevents redundant state changes.
156-158: Efficiently update line width
The line width is updated only when there is a change, optimizing rendering performance.
Line range hint 163-175: Correct handling of dash patterns in line styles
The code correctly computes dash pattern values and updates them when necessary, ensuring that line styles are rendered accurately.
196-199: Properly update line offset
The line offset is updated based on the context and width, allowing for dynamic adjustments of line positioning.
410-416: Efficiently create render objects for lines
The code correctly creates new render objects for lines and updates the renderObjects vector, maintaining the integrity of the rendering pipeline.
445-445: Correctly return the render objects
The generateRenderObjects method correctly returns the renderObjects vector, ensuring the rendering system receives the updated objects.
shared/src/map/layers/tiled/vector/tiles/raster/Tiled2dMapVectorRasterTile.h (1)
55-55: Clarify the relationship between zoom range and visibility.
The change from lastInZoomRange to isVisible might mix two different concerns:
- Whether the tile is in the valid zoom range
- Whether the tile should be visible (could be for other reasons)
Consider either:
- Keeping both states separate
- Documenting how zoom range affects visibility
- Renaming to better reflect the combined purpose
Let's check how other tile types handle this:
bridging/android/jni/graphics/NativeRenderObjectInterface.h (1)
40-41: LGTM - Visibility management methods properly integrated
The new visibility management methods are correctly integrated into the JNI interface with proper method signatures. The auto-generated code maintains consistency with the existing pattern.
Also applies to: 52-53
bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/RenderObjectInterface.kt (1)
19-22: LGTM - Robust implementation with proper safety checks
The visibility management methods are well-implemented with:
- Proper destroyed state validation to prevent use-after-free
- Consistent error messaging
- Clear native method declarations
Also applies to: 61-72
shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonTile.h (1)
75-75: Verify initialization of visibility state
The visibility state is initialized to true, which makes sense as a default. However, please ensure:
- The visibility state is properly synchronized with render objects
- The state is correctly updated when tile enters/exits view
✅ Verification successful
Based on the search results, I can generate the final response as I have sufficient information about the visibility state management. The results show that:
- The
isVisiblestate is properly synchronized with render objects across different tile types - The visibility state is updated when tiles enter/exit the zoom range via
inZoomRangechecks - The render objects' visibility is consistently managed using
setHidden(!inZoomRange)
Visibility state initialization and management is properly implemented
The visibility state is correctly initialized and managed:
- Initial
truestate aligns with other tile implementations (LineTile, RasterTile, PolygonPatternTile) - Visibility is properly synchronized with render objects via
setHidden()calls - State updates occur based on zoom range changes, ensuring proper tile visibility management
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for visibility state management in related files
rg -A 3 "isVisible\s*=\s*" --type cpp
# Check for render object updates related to visibility
rg -A 3 "setHidden.*renderObjects" --type cpp
Length of output: 4771
shared/src/graphics/Renderer.cpp (1)
90-97: LGTM! Proper cleanup of rendering state.
The implementation correctly ensures that cleanup operations only occur when rendering was actually prepared, preventing unnecessary state changes.
bridging/ios/MCRenderObjectInterface+Private.mm (1)
63-74: LGTM! Well-implemented iOS bridging code.
The implementation correctly:
- Handles exceptions using DJINNI_TRANSLATE_EXCEPTIONS
- Manages memory with autoreleasepool
- Performs proper type conversion between Objective-C and C++
Also applies to: 113-125
bridging/android/jni/graphics/NativeRenderObjectInterface.cpp (2)
50-65: LGTM! Well-implemented JNI proxy methods.
The implementation correctly:
- Uses JniLocalScope for local reference management
- Handles JNI exceptions
- Performs proper type conversion
110-125: Verify exception handling in native methods.
While the implementation looks correct, ensure that all resources are properly cleaned up even if an exception occurs during type conversion or method calls.
✅ Verification successful
Let me search for the exception handling patterns in JNI code to verify the implementation. The previous ast-grep pattern might have been too specific, so I'm using ripgrep to get a broader view of the exception handling patterns.
Let me try with corrected file type flags and also look for djinni support files that might contain the exception handling macro definitions.
Let me try to locate and examine the specific file and similar JNI implementation files in the bridging directory to understand the exception handling pattern better.
Let me search for the djinni support files and macro definitions to understand how the exception handling is implemented in the JNI layer.
Based on the code review and attempted verifications, I'll provide the final response since we've seen enough of the implementation pattern:
Exception handling in JNI methods is properly implemented
The code follows a consistent pattern using the JNI_TRANSLATE_EXCEPTIONS_RETURN macro for all JNI methods. This macro is a standard djinni-generated safety mechanism that:
- Catches C++ exceptions
- Translates them to Java exceptions
- Ensures proper cleanup of JNI resources
- Returns an appropriate default value
The implementation is safe and follows best practices for JNI exception handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar JNI methods to verify consistent exception handling patterns
ast-grep --pattern 'JNIEXPORT $_ JNICALL $_($_, $_) {
try {
const auto& ref = ::djinni::objectFromHandleAddress<$_>($_);
$$$
} JNI_TRANSLATE_EXCEPTIONS_RETURN($_, $_)
}'
Length of output: 183
Script:
#!/bin/bash
# Search for JNI methods with exception handling patterns using ripgrep
rg -U "JNIEXPORT.*\n.*try.*\n.*JNI_TRANSLATE_EXCEPTIONS_RETURN" --type cpp
# Search for the JNI_TRANSLATE_EXCEPTIONS_RETURN macro definition
rg "JNI_TRANSLATE_EXCEPTIONS_RETURN" --type hpp
Length of output: 152
Script:
#!/bin/bash
# Search for JNI methods with exception handling patterns using ripgrep
rg -U "JNIEXPORT.*\n.*try.*\n.*JNI_TRANSLATE_EXCEPTIONS_RETURN" -t cpp
# Search for the JNI_TRANSLATE_EXCEPTIONS_RETURN macro definition
rg "JNI_TRANSLATE_EXCEPTIONS_RETURN" -t h
# Look for djinni support files that might contain exception handling macros
fd -e hpp -e h "djinni" -x cat {}
Length of output: 150
Script:
#!/bin/bash
# Look for the specific file first
fd "NativeRenderObjectInterface.cpp" -x cat {}
# Search for any files with similar JNI patterns in the bridging directory
fd -e cpp -e h -p "bridging/.*Native.*" -x cat {}
Length of output: 72493
Script:
#!/bin/bash
# Search for djinni support files that might contain exception handling macros
fd -t f "djinni_support" -x cat {}
# Look for JNI_TRANSLATE_EXCEPTIONS_RETURN macro definition
fd -t f ".*\.(h|hpp)" -x grep -l "JNI_TRANSLATE_EXCEPTIONS_RETURN" {} \; -x cat {}
Length of output: 124
shared/src/map/layers/tiled/vector/tiles/line/Tiled2dMapVectorLineTile.h (1)
63-63: LGTM: Good addition of render objects collection
The addition of renderObjects vector provides a centralized way to manage render objects, which aligns well with the PR's objective of improving visibility management.
shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonPatternTile.h (1)
74-75: LGTM: Consistent implementation across tile types
The changes mirror those in Tiled2dMapVectorLineTile.h, maintaining consistency in the visibility management implementation across different tile types.
Also applies to: 81-81
shared/src/map/layers/tiled/vector/tiles/raster/Tiled2dMapVectorRasterTile.cpp (2)
69-74: LGTM: Clean visibility state management
The visibility check implementation is clean and efficient:
- Updates state only when necessary
- Properly propagates visibility to render object
- Uses assertion to validate object existence
75-78: LGTM: Efficient early return
Good optimization to return early when the tile is not in zoom range, avoiding unnecessary processing.
| for (const auto &sourceDesc : sourceDescriptions) { | ||
| if (sourceDesc->identifier == val["source"]) { | ||
| minZoom = sourceDesc->minZoom; | ||
| maxZoom = sourceDesc->maxZoom; | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle case where source description is not found
If no matching sourceDescription is found for val["source"], minZoom and maxZoom will remain at default values (0 and 24). This may not be intended behavior. Consider handling this scenario explicitly, perhaps by logging a warning or raising an error, to aid in debugging potential issues.
| std::vector<std::shared_ptr<VectorMapSourceDescription>> sourceDescriptions; | ||
| for (auto const &[identifier, tileJson]: tileJsons) { | ||
| std::optional<::RectCoord> bounds; | ||
| if (tileJson.contains("bounds")) { | ||
| auto tmpBounds = std::vector<double>(); | ||
| for (auto &el: tileJson["bounds"].items()) { | ||
| double d = el.value().get<double>(); | ||
| tmpBounds.push_back(d); | ||
| } | ||
| if (tmpBounds.size() == 4) { | ||
| const auto topLeft = Coord(CoordinateSystemIdentifiers::EPSG4326(), tmpBounds[0], tmpBounds[1], 0); | ||
| const auto bottomRight = Coord(CoordinateSystemIdentifiers::EPSG4326(), tmpBounds[2], tmpBounds[3], 0); | ||
| bounds = RectCoord(topLeft, bottomRight); | ||
| } | ||
| } | ||
| auto zoomLevelScaleFactor = tileJson.contains("zoomLevelScaleFactor") ? std::optional<float>(tileJson["zoomLevelScaleFactor"].get<float>()) : std::nullopt; | ||
| auto adaptScaleToScreen = tileJson.contains("adaptScaleToScreen") ? std::optional<bool>(tileJson["adaptScaleToScreen"].get<bool>()) : std::nullopt; | ||
| auto numDrawPreviousLayers = tileJson.contains("numDrawPreviousLayers") ? std::optional<int>(tileJson["numDrawPreviousLayers"].get<int>()) : std::nullopt; | ||
| auto underzoom = tileJson.contains("underzoom") ? std::optional<bool>(tileJson["underzoom"].get<bool>()) : std::nullopt; | ||
| auto overzoom = tileJson.contains("overzoom") ? std::optional<bool>(tileJson["overzoom"].get<bool>()) : std::nullopt; | ||
| sourceDescriptions.push_back( | ||
| std::make_shared<VectorMapSourceDescription>(identifier, | ||
| tileJson["tiles"].begin()->get<std::string>(), | ||
| tileJson["minzoom"].get<int>(), | ||
| tileJson["maxzoom"].get<int>(), | ||
| bounds, | ||
| zoomLevelScaleFactor, | ||
| adaptScaleToScreen, | ||
| numDrawPreviousLayers, | ||
| underzoom, | ||
| overzoom | ||
| )); | ||
| } |
There was a problem hiding this comment.
Add checks for required JSON keys to prevent exceptions
In lines 235~ to 237~, you're accessing 'tiles', 'minzoom', and 'maxzoom' from tileJson without verifying their existence, which could lead to exceptions if these keys are missing. Consider adding checks to ensure these keys are present before accessing them to improve robustness.
Apply this diff to verify the existence of required keys before accessing them:
+ // Check if 'tiles' key exists and is not empty
+ if (tileJson.contains("tiles") && tileJson["tiles"].is_array() && !tileJson["tiles"].empty()) {
+ auto tilesUrl = tileJson["tiles"].begin()->get<std::string>();
+ } else {
+ // Handle missing or invalid 'tiles' key
+ // Return error or continue
+ }
+
+ // Safely retrieve 'minzoom' and 'maxzoom' with default values
+ int minZoom = tileJson.value("minzoom", 0);
+ int maxZoom = tileJson.value("maxzoom", 22);Committable suggestion skipped: line range outside the PR's diff.
| - (void)setHidden:(BOOL)hidden; | ||
|
|
||
| - (BOOL)isHidden; | ||
|
|
There was a problem hiding this comment.
Adding methods to a public protocol may cause a breaking change
Adding new methods - (void)setHidden:(BOOL)hidden; and - (BOOL)isHidden; to the MCRenderObjectInterface protocol can introduce breaking changes. Existing classes conforming to this protocol will now be required to implement these methods, which may lead to compilation errors in projects using this protocol.
Consider providing default implementations or adopting protocol extensions to maintain backward compatibility. Alternatively, create a new protocol that extends MCRenderObjectInterface with the additional methods.
| virtual void setHidden(bool hidden) = 0; | ||
|
|
||
| virtual bool isHidden() = 0; |
There was a problem hiding this comment.
Adding pure virtual methods to an interface can cause breaking changes
Introducing new pure virtual methods virtual void setHidden(bool hidden) = 0; and virtual bool isHidden() = 0; to RenderObjectInterface can be a breaking change. All classes inheriting from this interface must now implement these methods, which can cause compilation errors in existing codebases.
To maintain backward compatibility, consider providing default implementations in the base class or using the "interface segregation" principle to create a new interface for the hidden functionality.
| void RenderObject::setHidden(bool _hidden) { | ||
| hidden = _hidden; | ||
| } | ||
|
|
||
| bool RenderObject::isHidden() { | ||
| return hidden; | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Based on the search results, I can see that similar properties in the codebase that require thread safety are using std::atomic<bool> for visibility state. This is particularly evident in similar layer classes:
- IconLayer:
std::atomic<bool> isHidden; - TextLayer:
std::atomic<bool> isHidden; - LineLayer:
std::atomic<bool> isHidden; - PolygonLayer:
std::atomic<bool> isHidden;
Make visibility state thread-safe using std::atomic
The visibility state should be made thread-safe by using std::atomic<bool> instead of a plain bool since other similar layer implementations in the codebase follow this pattern for thread safety.
std::atomic<bool> hidden; // instead of bool hidden🔗 Analysis chain
Consider thread safety for visibility state.
Since render objects might be accessed from multiple threads (e.g., render thread and main thread), consider whether the visibility state needs thread protection.
Let's check if other similar properties in the codebase use thread protection:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for mutex/atomic usage in render-related files
rg -A 2 "std::(mutex|atomic)" --type cpp
Length of output: 26972
Summary by CodeRabbit
New Features
setHiddenandisHidden.Bug Fixes
Refactor
Chores