Self Masking for Vector Layers #584
Conversation
…e maxZoom in geoJson decoding
WalkthroughThe updates focus on enhancing stencil operations and masking logic across various OpenGL objects and layer descriptions in a graphics rendering context. A significant addition is the Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 10
Configuration used: CodeRabbit UI
Files selected for processing (46)
- android/src/main/cpp/graphics/objects/LineGroup2dOpenGl.cpp (1 hunks)
- android/src/main/cpp/graphics/objects/Polygon2dOpenGl.cpp (1 hunks)
- android/src/main/cpp/graphics/objects/PolygonGroup2dOpenGl.cpp (1 hunks)
- android/src/main/cpp/graphics/objects/PolygonPatternGroup2dOpenGl.cpp (1 hunks)
- android/src/main/cpp/graphics/objects/Quad2dInstancedOpenGl.cpp (1 hunks)
- android/src/main/cpp/graphics/objects/Quad2dOpenGl.cpp (1 hunks)
- android/src/main/cpp/graphics/objects/Quad2dStretchedInstancedOpenGl.cpp (1 hunks)
- android/src/main/cpp/graphics/objects/Text2dInstancedOpenGl.cpp (1 hunks)
- android/src/main/cpp/graphics/objects/Text2dOpenGl.cpp (1 hunks)
- bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/RenderPassConfig.kt (1 hunks)
- bridging/android/jni/graphics/NativeRenderPassConfig.cpp (1 hunks)
- bridging/android/jni/graphics/NativeRenderPassConfig.h (1 hunks)
- bridging/ios/MCRenderPassConfig+Private.mm (1 hunks)
- bridging/ios/MCRenderPassConfig.h (1 hunks)
- bridging/ios/MCRenderPassConfig.mm (1 hunks)
- djinni/graphics/core.djinni (1 hunks)
- ios/graphics/Model/BaseGraphicsObject.swift (1 hunks)
- ios/graphics/Model/Polygon/PolygonGroup2d.swift (3 hunks)
- ios/graphics/RenderingContext.swift (1 hunks)
- shared/public/BackgroundVectorLayerDescription.h (1 hunks)
- shared/public/LineVectorLayerDescription.h (1 hunks)
- shared/public/PolygonVectorLayerDescription.h (1 hunks)
- shared/public/RasterVectorLayerDescription.h (1 hunks)
- shared/public/RenderPassConfig.h (1 hunks)
- shared/public/SymbolVectorLayerDescription.h (1 hunks)
- shared/public/Tiled2dMapVectorLayer.h (1 hunks)
- shared/public/VectorLayerDescription.h (3 hunks)
- shared/src/map/layers/icon/IconLayer.cpp (1 hunks)
- shared/src/map/layers/line/LineLayer.cpp (1 hunks)
- shared/src/map/layers/polygon/PolygonLayer.cpp (1 hunks)
- shared/src/map/layers/text/TextLayer.cpp (1 hunks)
- shared/src/map/layers/tiled/raster/Tiled2dMapRasterLayer.cpp (1 hunks)
- shared/src/map/layers/tiled/vector/Tiled2dMapVectorLayer.cpp (1 hunks)
- shared/src/map/layers/tiled/vector/Tiled2dMapVectorLayerParserHelper.cpp (5 hunks)
- shared/src/map/layers/tiled/vector/geojson/geojsonvt/geojsonvt.hpp (2 hunks)
- shared/src/map/layers/tiled/vector/sourcemanagers/Tiled2dMapVectorSourceDataManager.cpp (1 hunks)
- shared/src/map/layers/tiled/vector/sourcemanagers/Tiled2dMapVectorSourceDataManager.h (1 hunks)
- shared/src/map/layers/tiled/vector/sourcemanagers/Tiled2dMapVectorSourceTileDataManager.cpp (1 hunks)
- shared/src/map/layers/tiled/vector/sublayers/background/Tiled2dMapVectorBackgroundSubLayer.cpp (1 hunks)
- shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSourceSymbolDataManager.cpp (2 hunks)
- shared/src/map/layers/tiled/vector/tiles/line/Tiled2dMapVectorLineTile.cpp (5 hunks)
- shared/src/map/layers/tiled/vector/tiles/line/Tiled2dMapVectorLineTile.h (1 hunks)
- shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonPatternTile.cpp (8 hunks)
- shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonPatternTile.h (2 hunks)
- shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonTile.cpp (5 hunks)
- shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonTile.h (1 hunks)
Additional comments: 54
bridging/ios/MCRenderPassConfig.h (1)
- 9-12: The addition of
isPassMaskedto the initializer and factory method is correctly implemented. However, ensure that all calls to these methods throughout the iOS codebase are updated to include the new parameter.bridging/ios/MCRenderPassConfig+Private.mm (1)
- 13-20: Correctly integrates
isPassMaskedinto the Objective-C to C++ conversion. No issues found.bridging/ios/MCRenderPassConfig.mm (2)
- 10-14: Initialization of
isPassMaskedin the constructor is correctly implemented.- 29-29: The inclusion of
isPassMaskedin thedescriptionmethod provides better debugging visibility. Good practice.bridging/android/jni/graphics/NativeRenderPassConfig.h (1)
- 28-30: Correctly added
field_isPassMaskedto support the newisPassMaskedfield in JNI conversions.bridging/android/jni/graphics/NativeRenderPassConfig.cpp (2)
- 16-17: Correctly handles the
isPassMaskedfield in thefromCppJNI conversion method.- 26-27: Correctly handles the
isPassMaskedfield in thetoCppJNI conversion method.shared/public/VectorLayerDescription.h (2)
- 30-30: The addition of
selfMaskedas a member variable is correctly implemented. Ensure that all instances ofVectorLayerDescriptionand its derived classes are updated accordingly.- 61-68: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [64-75]
The constructor correctly includes the
selfMaskedparameter. Ensure that all constructor calls are updated to pass this new parameter.djinni/graphics/core.djinni (1)
- 61-61: The addition of
is_pass_maskedto therender_pass_configrecord is correctly implemented. Ensure that all related Djinni-generated code is regenerated.shared/src/map/layers/tiled/vector/sourcemanagers/Tiled2dMapVectorSourceDataManager.cpp (1)
- 37-39: Correctly adds layer indexes to
selfMaskedLayersbased on theselfMaskedproperty. This logic ensures that self-masked layers are tracked separately.shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonTile.h (1)
- 52-52: The change in the
addPolygonsmethod parameter type fromunordered_maptovectorofvectorindicates a significant change in how polygons are managed and grouped. Ensure that the implementation logic in the corresponding.cppfile is updated to handle this new data structure efficiently.shared/src/map/layers/tiled/vector/tiles/line/Tiled2dMapVectorLineTile.h (1)
- 46-46: The modification in the
addLinesmethod signature reflects a change in how line data is structured and processed. Ensure that the implementation is updated to efficiently handle the newvectorofvectorstructure for line data.shared/public/BackgroundVectorLayerDescription.h (1)
- 68-68: The constructor call to
VectorLayerDescriptionwithinBackgroundVectorLayerDescriptioncorrectly passes the newselfMaskedparameter. This ensures that the background layer can also support self-masking features.ios/graphics/Model/BaseGraphicsObject.swift (2)
- 84-98: The
maskStencilStatemethod correctly sets up a stencil state for masking operations. The configuration aligns with typical stencil masking techniques.- 100-114: The
renderPassMaskStencilStatemethod is correctly implemented to support stencil operations specific to render passes. This addition enhances the flexibility in rendering operations.shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonPatternTile.h (1)
- 56-56: The change in the
addPolygonsmethod parameter type and the addition ofpolygonRenderOrderindicate enhancements in polygon pattern rendering and order management. Ensure that the implementation logic efficiently supports these changes.ios/graphics/Model/Polygon/PolygonGroup2d.swift (1)
- 20-27: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [84-114]
The modifications to handle
renderPassparameter and the introduction ofrenderPassStencilStatebased on theisPassMaskedproperty are correctly implemented. This change allows for more nuanced control over stencil operations during rendering.shared/src/map/layers/tiled/vector/sourcemanagers/Tiled2dMapVectorSourceDataManager.h (1)
- 87-87: The declaration of
std::unordered_set<int32_t> selfMaskedLayersis correctly added to track self-masked layers. This addition is crucial for managing rendering behavior based on self-masking properties.shared/public/PolygonVectorLayerDescription.h (1)
- 102-110: The constructor and
clonemethod correctly incorporate theselfMaskedparameter, ensuring that polygon layers can specify self-masking behavior. This change is consistent with the overall goal of enhancing rendering capabilities.ios/graphics/RenderingContext.swift (1)
- 88-88: The modification to include an
isPassMaskedboolean parameter in theRenderPassConfiginitialization within theclearStencilBuffermethod is a significant change. This adjustment implies a semantic change in how rendering passes are handled, particularly in relation to stencil operations. Ensure that:
- The default value of
falseforisPassMaskedin this context is appropriate and aligns with the intended behavior of stencil buffer clearing operations.- All calls to
RenderPassConfigthroughout the iOS codebase have been audited to ensure that the newisPassMaskedparameter is correctly set according to the desired rendering logic.Consider adding unit tests or integration tests that specifically verify the behavior of rendering operations with and without the
isPassMaskedflag set, to ensure that this change does not introduce unintended side effects in rendering logic.android/src/main/cpp/graphics/objects/LineGroup2dOpenGl.cpp (1)
- 112-112: The change to use a ternary operator for setting the stencil function based on
isMaskInversedin therendermethod simplifies the conditional logic, making it more concise. However, ensure that:
- The logic accurately reflects the intended behavior for both masked and unmasked rendering scenarios.
- This change is consistently applied across all similar instances in the codebase where stencil operations are determined by masking conditions.
This refinement improves code readability and maintainability. Just verify that it aligns with the overall rendering strategy and does not introduce any unintended side effects.
shared/src/map/layers/text/TextLayer.cpp (1)
- 193-193: The adjustment to the
RenderPassConfigconstructor within thegenerateRenderPassesmethod, addingfalseas an argument, is crucial for controlling rendering behavior. This change implies a specific handling of masking for text layers. To ensure this change is effectively integrated:
- Confirm that the addition of the
falseargument toRenderPassConfigaligns with the intended behavior for text layer rendering, particularly in relation to masking and stencil operations.- Review all instances where
RenderPassConfigis instantiated within the text layer rendering logic to ensure consistency and correctness.It would be beneficial to conduct thorough testing of text rendering under various conditions (with and without masking) to verify that the rendering behavior remains consistent with expectations and that no unintended side effects have been introduced.
android/src/main/cpp/graphics/objects/PolygonPatternGroup2dOpenGl.cpp (1)
- 144-158: The logic for calculating
stencilMask,validTarget, andzpassbased onisMaskedandrenderPass.isPassMaskedconditions is correct and follows the intended functionality for handling masking and rendering passes. However, ensure that the values used forstencilMaskandvalidTargetare consistent with the rest of the rendering pipeline and that they do not conflict with other stencil operations in the application.shared/src/map/layers/tiled/vector/sublayers/background/Tiled2dMapVectorBackgroundSubLayer.cpp (1)
- 79-79: The modification to the
RenderPassconstructor call to include a new boolean parameter (false) is correctly implemented. This change is necessary to support the new self-masking feature by explicitly setting the masking behavior of the render pass. Ensure that all other instances whereRenderPassis constructed throughout the codebase are reviewed and updated accordingly if needed to maintain consistency.android/src/main/cpp/graphics/objects/Text2dOpenGl.cpp (1)
- 212-226: The implementation for calculating
stencilMask,validTarget, andzpassin therendermethod based onisMaskedandrenderPass.isPassMaskedconditions is correct. This addition aligns with the objective of enhancing the rendering process to support self-masking for text elements. Ensure that the stencil operations defined here are compatible with the overall rendering strategy and do not interfere with other graphical elements.shared/public/Tiled2dMapVectorLayer.h (1)
- 108-108: The addition of the
selfMaskedboolean attribute to theTiled2dMapVectorLayerclass is correctly implemented. This attribute is essential for indicating whether the layer is self-masked or not, aligning with the PR's objective to introduce self-masking capabilities. Ensure that this new attribute is utilized appropriately in the layer's rendering logic and that any necessary logic to handle self-masking is implemented.shared/src/map/layers/line/LineLayer.cpp (1)
- 188-188: The addition of
falseas a parameter toRenderPassConfigconstructor in thegenerateRenderPassesmethod is a significant change. This boolean likely controls the new self-masking feature for the line layer. Ensure that all instances whereRenderPassConfigis instantiated have been updated to include this new parameter where necessary. This change aligns with the PR's objective to introduce self-masking capabilities across various vector layer types.android/src/main/cpp/graphics/objects/Quad2dInstancedOpenGl.cpp (1)
- 209-223: The logic for setting
stencilMask,validTarget, andzpassbased onisMaskedandrenderPass.isPassMaskedconditions, and the subsequent configuration ofglStencilFuncandglStencilOp, introduces a more nuanced control over the rendering process, specifically tailored to support self-masking features. This is a critical update for enhancing rendering capabilities with respect to self-masking. Ensure that the logic correctly handles all possible combinations ofisMaskedandrenderPass.isPassMaskedto avoid rendering artifacts or unexpected behavior.android/src/main/cpp/graphics/objects/Text2dInstancedOpenGl.cpp (1)
- 203-217: Similar to
Quad2dInstancedOpenGl, the adjustments made to stencil operations inText2dInstancedOpenGlare crucial for implementing self-masking features. The careful calculation ofstencilMask,validTarget, andzpassensures that text rendering can also benefit from self-masking, enhancing the visual quality and performance of text elements in vector layers. It's important to validate that these changes integrate well with the overall rendering pipeline and do not introduce side effects when rendering text with or without self-masking.shared/src/map/layers/polygon/PolygonLayer.cpp (1)
- 253-253: The modification to include
falseas an additional argument in theRenderPassConfigconstructor within thegenerateRenderPassesmethod forPolygonLayeraligns with the overarching goal of introducing self-masking capabilities. This change is consistent with similar updates in other layer types, ensuring a unified approach to handling self-masking across the rendering engine. It's essential to ensure that this new parameter is correctly utilized in all relevant contexts to fully leverage the self-masking feature.android/src/main/cpp/graphics/objects/Quad2dStretchedInstancedOpenGl.cpp (1)
- 208-222: The logic for setting
glStencilFuncandglStencilOpbased onisMaskedandrenderPass.isPassMaskedis correct and aligns with the PR's objective to enhance self-masking capabilities for vector layers. However, ensure that the values used forstencilMask,validTarget, andzpassare consistent with the intended masking behavior and OpenGL standards.shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonTile.cpp (5)
- 151-151: The replacement of
std::unordered_mapwithstd::vectorforstyleGroupNewPolygonsVectorintroduces a more sequential and ordered approach to handling style groups and polygons. This change could potentially improve cache locality and performance during rendering. Ensure that the logic for indexing and accessing these vectors correctly matches the intended grouping and rendering order of polygons.- 188-188: The initialization of
styleGroupNewPolygonsVectorwith an empty vector for each new style group is correct. This setup facilitates the addition of polygons to their respective style groups in a structured manner. Verify that the logic for appending polygons to these vectors is implemented correctly in subsequent parts of the code.- 211-223: The logic for appending polygon indices and vertices to the last element of
styleGroupNewPolygonsVector[styleGroupIndex]is sound. This approach allows for efficient grouping of polygon data by style group. However, ensure that the handling ofindexOffsetand the calculation of new sizes for vertices and indices are accurate to prevent buffer overflows or rendering artifacts.- 241-241: The call to
addPolygonswithstyleGroupNewPolygonsVectoras the argument is appropriate and aligns with the changes made to the polygon handling logic. This encapsulation of polygon addition logic enhances modularity and maintainability of the code. Confirm thataddPolygonsis implemented to correctly process the vector-based input.- 264-272: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [248-313]
The
addPolygonsmethod's adaptation to accept a vector of vectors (styleGroupNewPolygonsVector) instead of an unordered map is a significant change that aligns with the PR's objectives. This change simplifies the handling of style groups and polygons, potentially improving performance. Ensure that the iteration overstyleGroupNewPolygonsVectorand the subsequent setup of polygon group objects are correctly implemented to reflect the new data structure.shared/src/map/layers/tiled/vector/tiles/line/Tiled2dMapVectorLineTile.cpp (3)
- 288-300: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [232-313]
The transition from using
std::unordered_maptostd::vectorfor handling line style groups (styleGroupNewLinesVector) and line subgroups (styleGroupLineSubGroupVector) is consistent with the PR's goal of enhancing the handling of vector layer elements. This change likely improves data access patterns and simplifies the logic for managing line styles and groups. Ensure that the logic for populating these vectors, especially the handling of line subgroups and the transition between them when themaxNumLinePointslimit is reached, is correctly implemented to maintain the integrity of line rendering.
- 320-320: The call to
addLineswithstyleGroupNewLinesVectoras the argument correctly reflects the changes made to line handling logic. This approach, which now uses a vector of vectors, is expected to streamline the processing of line data according to their style groups. Confirm thataddLinesis implemented to accurately process the vector-based input, ensuring that line data is correctly grouped and rendered according to their styles.- 328-328: The adaptation of the
addLinesmethod to accept a vector of vectors (styleIdLinesVector) instead of an unordered map represents a significant change that aligns with the PR's objectives. This modification simplifies the handling of line styles and groups, potentially improving performance. Ensure that the iteration overstyleIdLinesVectorand the subsequent setup of line group objects are correctly implemented to reflect the new data structure.shared/src/map/layers/tiled/vector/sourcemanagers/Tiled2dMapVectorSourceTileDataManager.cpp (1)
- 70-73: The addition of the
selfMaskedboolean variable to theTiled2dMapVectorLayer::TileRenderDescriptionconstructor call is a critical update for implementing self-masking capabilities for vector layers. This change correctly uses the presence oflayerIndexinselfMaskedLayersto determine theselfMaskedstatus, which is a logical approach for enabling self-masking based on layer configurations. Ensure that all relevant documentation and tests are updated to reflect this new parameter's role in rendering logic.shared/src/map/layers/icon/IconLayer.cpp (1)
- 342-342: The modification to include an additional boolean argument (
false) in theRenderPassConfigconstructor call within thebuildRenderPassesmethod aligns with the broader implementation strategy for self-masking capabilities. By explicitly setting this argument tofalse, it indicates that icon layers do not utilize self-masking by default, which should be consistent with the design intentions. Ensure that this behavior is documented clearly, and consider if any scenarios might require this to be configurable for icon layers in the future.shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonPatternTile.cpp (2)
- 169-169: The transition from
std::unordered_maptostd::vectorfor handling style groups and polygons inTiled2dMapVectorPolygonPatternTile.cppis a significant change that could enhance performance due to the contiguous memory layout of vectors. This modification should make the management of polygons and their associated styles more efficient, especially if the number of style groups is known and relatively stable. Verify that this change does not introduce any regressions in the ordering or accessibility of polygon data, particularly in scenarios where dynamic modifications to style groups occur.- 271-273: In the
addPolygonsmethod, the use of a vector to pass polygon descriptions maintains consistency with the earlier change to vector-based management. This approach simplifies the handling of polygon data. However, ensure that the logic for adding, updating, or removing polygons accounts for the vector's indexing, which is inherently different from key-based access in unordered maps. This might require additional checks or updates to ensure that polygon data is correctly associated with their respective style groups without any mismatches.shared/public/SymbolVectorLayerDescription.h (2)
- 512-514: The constructor of
SymbolVectorLayerDescriptionnow includes a new parameterselfMasked. Ensure that all instantiations of this class throughout the codebase are updated to include this new parameter.- 520-520: The
clonemethod ofSymbolVectorLayerDescriptionnow correctly clones theselfMaskedproperty. This is a good practice for ensuring that cloned objects maintain the same state as their originals.shared/src/map/layers/tiled/raster/Tiled2dMapRasterLayer.cpp (3)
- 419-419: The addition of a boolean argument to the
RenderPassConfigconstructor withinstd::make_shared<RenderPass>calls is consistent with the PR's objective to support self-masking for vector layers. However, ensure that the hardcodedfalsevalue is intentional and correctly reflects the desired behavior for all instances where aRenderPassis created.Verification successful
The provided script output does not offer new information or insights into the use of the hardcoded
falsevalue for selfMasking in theRenderPassConfigconstructor. The output simply echoes the initial concern about needing manual verification to confirm if the hardcoded value aligns with the intended use cases for selfMasking. This indicates that the script executed was not designed to analyze the codebase or check for the usage ofRenderPassConfigacross different instances. Therefore, to move forward with the verification process, we need to conduct a more detailed analysis of the codebase to understand howRenderPassConfigis used and whether the hardcodedfalsevalue is appropriate in all instances.Given the limitations of the provided script output, I will proceed with generating a new verification strategy to analyze the usage of
RenderPassConfigwithin the codebase. This will involve searching for instances whereRenderPassConfigis instantiated to review the context in which the boolean argument is set.
The search results provide several instances where
RenderPassConfigis instantiated with a boolean argument for self-masking across different layers and contexts within the codebase. Notably, the hardcodedfalsevalue is consistently used in various layers, including tiled vector and raster layers, text, polygon, icon, and line layers. However, there is an exception in theTiled2dMapVectorLayer.cppfile, where the boolean argument is dynamically set based ondescription->selfMasked, indicating that self-masking can be enabled or disabled based on specific conditions.Given this information, it appears that the hardcoded
falsevalue for self-masking in theRenderPassConfigconstructor is intentional for most use cases, with flexibility provided in certain contexts to enable self-masking based on specific layer or rendering conditions. This approach suggests that the implementation is designed to default to no self-masking for most rendering passes, with the option to enable it where necessary, aligning with the PR's objective to support self-masking for vector layers selectively.Based on the analysis of the codebase and the usage patterns of
RenderPassConfig, the review comment is verified as the hardcodedfalsevalue seems to be intentionally used to reflect the desired behavior across the majority of instances where aRenderPassis created, with provisions for enabling self-masking where applicable.* 426-426: Similar to the previous comment, verify that the hardcoded `false` value for the `selfMasked` parameter in the `RenderPassConfig` constructor is intentional and aligns with the desired behavior for raster layers. * 431-431: Again, ensure that the hardcoded `false` value for the `selfMasked` parameter in the `RenderPassConfig` constructor is intentional and correctly reflects the desired behavior for all instances where a `RenderPass` is created without a specific mask.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify if the hardcoded false value for selfMasking is intentional in all cases. # Manual verification might be needed to confirm the logic aligns with the intended use cases. echo "Manual verification needed to confirm the logic aligns with the intended use cases for selfMasking."Length of output: 208
Script:
#!/bin/bash # Search for all instances of RenderPassConfig being instantiated to review the context of the boolean argument usage. rg "RenderPassConfig\(" --context 2Length of output: 18211
shared/src/map/layers/tiled/vector/Tiled2dMapVectorLayerParserHelper.cpp (3)
- 227-227: The addition of the
layerSelfMaskedboolean property to represent whether a layer is self-masked aligns with the PR's objectives. However, ensure that this property is correctly parsed from the metadata and utilized in all relevant layer descriptions.- 240-242: The parsing of the
selfMaskedproperty from the layer's metadata is a crucial step for enabling self-masking capabilities. Ensure that the metadata for each layer correctly specifies this property where applicable.- 370-377: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [313-395]
The inclusion of the
layerSelfMaskedparameter in the construction of layer descriptions is consistent with the PR's objective to support self-masking for vector layers. Ensure that all layer descriptions correctly utilize this parameter based on the layer's metadata.shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSourceSymbolDataManager.cpp (1)
- 721-727: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [724-736]
The introduction of the
selfMaskedboolean variable and its usage in theTiled2dMapVectorLayer::TileRenderDescriptionconstructor call is correctly implemented. The logic to determineselfMaskedbased on the presence of the index inselfMaskedLayersis sound. However, ensure that theselfMaskedLayersset is properly maintained elsewhere in the code to accurately reflect layers that should be self-masked.shared/src/map/layers/tiled/vector/Tiled2dMapVectorLayer.cpp (1)
- 632-632: Creating a
RenderPasswithRenderPassConfig(description->renderPassIndex, description->selfMasked)correctly respects theselfMaskedproperty. However, this is done within a condition that might not always be reached due to the logic in lines 626-628. Ensure that all scenarios where a layer's self-masking property should affect rendering are correctly handled.
| @@ -618,26 +618,26 @@ void Tiled2dMapVectorLayer::pregenerateRenderPasses() { | |||
| continue; | |||
| } | |||
| if (description->maskingObject != lastMask && !renderObjects.empty()) { | |||
| newPasses.emplace_back(std::make_shared<RenderPass>(RenderPassConfig(description->renderPassIndex), renderObjects, lastMask)); | |||
| newPasses.emplace_back(std::make_shared<RenderPass>(RenderPassConfig(description->renderPassIndex, false), renderObjects, lastMask)); | |||
There was a problem hiding this comment.
The creation of a RenderPass object with RenderPassConfig(description->renderPassIndex, false) seems to ignore the selfMasked property of the description object. This could potentially lead to incorrect rendering behavior where self-masking properties of layers are not respected.
- newPasses.emplace_back(std::make_shared<RenderPass>(RenderPassConfig(description->renderPassIndex, false), renderObjects, lastMask));
+ newPasses.emplace_back(std::make_shared<RenderPass>(RenderPassConfig(description->renderPassIndex, description->selfMasked), renderObjects, lastMask));Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| newPasses.emplace_back(std::make_shared<RenderPass>(RenderPassConfig(description->renderPassIndex, false), renderObjects, lastMask)); | |
| newPasses.emplace_back(std::make_shared<RenderPass>(RenderPassConfig(description->renderPassIndex, description->selfMasked), renderObjects, lastMask)); |
| if (description->isModifyingMask || description->selfMasked) { | ||
| if (!renderObjects.empty()) { | ||
| newPasses.emplace_back(std::make_shared<RenderPass>(RenderPassConfig(description->renderPassIndex), renderObjects, lastMask)); | ||
| newPasses.emplace_back(std::make_shared<RenderPass>(RenderPassConfig(description->renderPassIndex, false), renderObjects, lastMask)); |
There was a problem hiding this comment.
The creation of a RenderPass object with RenderPassConfig(description->renderPassIndex, false) immediately before potentially creating another RenderPass with description->selfMasked seems redundant and could lead to unnecessary render passes being created. Consider optimizing this logic to avoid creating an unnecessary render pass when description->isModifyingMask or description->selfMasked is true.
Consider restructuring this logic to only create a new RenderPass when necessary, potentially by checking the conditions first and then deciding on the RenderPassConfig parameters.
| } else { | ||
| renderObjects.insert(renderObjects.end(), description->renderObjects.begin(), description->renderObjects.end()); | ||
| lastMask = description->maskingObject; | ||
| lastRenderPassIndex = description->renderPassIndex; | ||
| } | ||
| } | ||
| if (!renderObjects.empty()) { | ||
| newPasses.emplace_back(std::make_shared<RenderPass>(RenderPassConfig(lastRenderPassIndex), renderObjects, lastMask)); | ||
| newPasses.emplace_back(std::make_shared<RenderPass>(RenderPassConfig(lastRenderPassIndex, false), renderObjects, lastMask)); |
There was a problem hiding this comment.
Similar to the comment on line 621, this creation of a RenderPass object ignores the selfMasked property for the last batch of render objects. This could lead to incorrect handling of self-masking properties in certain scenarios.
- newPasses.emplace_back(std::make_shared<RenderPass>(RenderPassConfig(lastRenderPassIndex, false), renderObjects, lastMask));
+ // Ensure to correctly handle the selfMasked property for the last batch of render objects.
+ // This might require tracking the selfMasked property of the last description or adjusting the logic to ensure it's correctly applied.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| newPasses.emplace_back(std::make_shared<RenderPass>(RenderPassConfig(lastRenderPassIndex, false), renderObjects, lastMask)); | |
| // Ensure to correctly handle the selfMasked property for the last batch of render objects. | |
| // This might require tracking the selfMasked property of the last description or adjusting the logic to ensure it's correctly applied. |
| @@ -5,4 +5,5 @@ package io.openmobilemaps.mapscore.shared.graphics | |||
|
|
|||
| data class RenderPassConfig( | |||
| val renderPassIndex: Int, | |||
| val isPassMasked: Boolean, | |||
There was a problem hiding this comment.
Missing default value for isPassMasked.
- val isPassMasked: Boolean,
+ val isPassMasked: Boolean = false,| @@ -8,9 +8,11 @@ | |||
|
|
|||
| struct RenderPassConfig final { | |||
| int32_t renderPassIndex; | |||
| bool isPassMasked; | |||
There was a problem hiding this comment.
The use of std::move for primitive types like int32_t and bool is unnecessary and does not have any effect.
- : renderPassIndex(std::move(renderPassIndex_))
- , isPassMasked(std::move(isPassMasked_))
+ : renderPassIndex(renderPassIndex_)
+ , isPassMasked(isPassMasked_)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| bool isPassMasked; | |
| bool isPassMasked; |
| GLuint stencilMask = 0; | ||
| GLuint validTarget = 0; | ||
| GLenum zpass = GL_KEEP; | ||
| if (isMasked) { | ||
| glStencilFunc(GL_EQUAL, isMaskInversed ? 0 : 128, 128); | ||
| glStencilOp(GL_KEEP, GL_KEEP, GL_KEEP); | ||
| stencilMask += 128; | ||
| validTarget = isMaskInversed ? 0 : 128; | ||
| } | ||
| if (renderPass.isPassMasked) { | ||
| stencilMask += 127; | ||
| zpass = GL_INCR; | ||
| } | ||
|
|
||
| if (stencilMask != 0) { | ||
| glStencilFunc(GL_EQUAL, validTarget, stencilMask); | ||
| glStencilOp(GL_KEEP, GL_KEEP, zpass); |
There was a problem hiding this comment.
The implementation of stencil operations in the render method introduces a more flexible handling of masking conditions. However, consider the following points for improvement:
- Ensure that the initial values of
stencilMaskandvalidTargetare appropriately chosen for all possible states ofisMaskedandrenderPass.isPassMasked. - The increment of
stencilMaskby 127 whenrenderPass.isPassMaskedis true might lead to unexpected behavior if bothisMaskedandrenderPass.isPassMaskedare true, as the total value could exceed the maximum value representable in the stencil buffer. Verify that this logic aligns with the intended stencil operations. - The use of
GL_INCRforzpasswhenrenderPass.isPassMaskedis true assumes that the stencil buffer is being used in a way that supports incremental updates. Ensure this matches the overall rendering strategy and that there's no risk of overflow in the stencil values which could lead to visual artifacts.
Consider adding comments to explain the rationale behind the chosen stencil operation values, especially the significance of 128 and 127, to improve maintainability and readability for future developers.
| GLuint stencilMask = 0; | ||
| GLuint validTarget = 0; | ||
| GLenum zpass = GL_KEEP; | ||
| if (isMasked) { | ||
| if (isMaskInversed) { | ||
| glStencilFunc(GL_EQUAL, 0, 255); | ||
| } else { | ||
| glStencilFunc(GL_EQUAL, 128, 255); | ||
| } | ||
| stencilMask += 128; | ||
| validTarget = isMaskInversed ? 0 : 128; | ||
| } | ||
| glStencilOp(GL_KEEP, GL_KEEP, GL_INCR); | ||
| if (renderPass.isPassMasked) { | ||
| stencilMask += 127; | ||
| zpass = GL_INCR; | ||
| } | ||
|
|
||
| if (stencilMask != 0) { | ||
| glStencilFunc(GL_EQUAL, validTarget, stencilMask); | ||
| glStencilOp(GL_KEEP, GL_KEEP, zpass); |
There was a problem hiding this comment.
The modifications to the render method for handling stencil operations based on masking conditions are well-intended for enhancing rendering flexibility. However, consider the following:
- The logic for calculating
stencilMaskandvalidTargetshould be thoroughly tested to ensure it behaves as expected under all combinations ofisMaskedandrenderPass.isPassMasked. - The choice of
128and127for modifyingstencilMaskvalues appears arbitrary without context. Ensure these values are documented to explain their significance and how they contribute to the intended stencil operations. - Using
GL_INCRforzpassassumes a specific usage pattern of the stencil buffer. Confirm that this approach is compatible with the overall rendering strategy and does not risk stencil buffer overflow, which could lead to rendering issues.
Adding detailed comments to explain the logic behind the chosen values and operations would greatly aid in the maintainability of this code. Additionally, consider edge cases where the combined stencil operations might not produce the expected results and adjust or document as necessary.
| bool multiselect, | ||
| bool selfMasked): | ||
| VectorLayerDescription(identifier, source, sourceId, minZoom, maxZoom, filter, renderPassIndex, interactable, multiselect, selfMasked), | ||
| style(style) {}; | ||
|
|
||
| std::unique_ptr<VectorLayerDescription> clone() override { | ||
| return std::make_unique<LineVectorLayerDescription>(identifier, source, sourceLayer, minZoom, maxZoom, | ||
| filter ? filter->clone() : nullptr, style, renderPassIndex, | ||
| interactable ? interactable->clone() : nullptr, multiselect); | ||
| interactable ? interactable->clone() : nullptr, multiselect, selfMasked); |
There was a problem hiding this comment.
The addition of the selfMasked boolean parameter to the LineVectorLayerDescription constructor and its inclusion in the clone method is a significant enhancement, allowing for more detailed control over layer rendering properties. Ensure that:
- The
selfMaskedparameter is correctly documented in the class header to explain its purpose and how it affects rendering behavior. - All usages of
LineVectorLayerDescriptionthroughout the codebase have been updated to account for the new parameter, ensuring that existing functionality is not inadvertently affected.
Consider implementing default values for the selfMasked parameter in scenarios where backward compatibility with existing code might be necessary. Additionally, unit tests should be updated or added to cover the new functionality introduced by this parameter.
| @@ -144,7 +144,7 @@ class RasterVectorLayerDescription: public VectorLayerDescription { | |||
| bool underzoom, | |||
| bool overzoom, | |||
| std::optional<::RectCoord> bounds): | |||
| VectorLayerDescription(identifier, source, "", minZoom, maxZoom, nullptr, renderPassIndex, interactable, false), | |||
| VectorLayerDescription(identifier, source, "", minZoom, maxZoom, nullptr, renderPassIndex, interactable, false, false), | |||
There was a problem hiding this comment.
The introduction of an additional boolean parameter in the RasterVectorLayerDescription constructor to initialize VectorLayerDescription with false, false for the last two arguments is a notable change. Ensure that:
- The purpose of these boolean parameters, especially the newly added one, is clearly documented in both the
RasterVectorLayerDescriptionandVectorLayerDescriptionclass headers. This will help developers understand their impact on layer behavior. - All instantiations of
RasterVectorLayerDescriptionthroughout the codebase have been reviewed to ensure that the correct values are being passed, maintaining the intended functionality.
Adding explicit documentation for the new parameter and its expected behavior would enhance code readability and maintainability. Consider reviewing related classes or methods that might be affected by this change to ensure consistency across the codebase.
| GLuint stencilMask = 0; | ||
| GLuint validTarget = 0; | ||
| GLenum zpass = GL_KEEP; | ||
| if (isMasked) { | ||
| glStencilFunc(GL_EQUAL, isMaskInversed ? 0 : 128, 128); | ||
| glStencilOp(GL_KEEP, GL_KEEP, GL_KEEP); | ||
| stencilMask += 128; | ||
| validTarget = isMaskInversed ? 0 : 128; | ||
| } | ||
| if (renderPass.isPassMasked) { | ||
| stencilMask += 127; | ||
| zpass = GL_INCR; | ||
| } | ||
|
|
||
| if (stencilMask != 0) { | ||
| glStencilFunc(GL_EQUAL, validTarget, stencilMask); | ||
| glStencilOp(GL_KEEP, GL_KEEP, zpass); |
There was a problem hiding this comment.
The modifications to the render method in Quad2dOpenGl for handling stencil operations based on masking conditions are well-implemented. However, ensure that:
- The logic for calculating
stencilMask,validTarget, andzpassis thoroughly tested, especially considering the potential for bothisMaskedandrenderPass.isPassMaskedto be true simultaneously. - The values
128and127used forstencilMaskadjustments are clearly documented, explaining their significance and ensuring that they do not inadvertently exceed the stencil buffer's capacity. - The choice of
GL_INCRforzpassis consistent with the overall rendering strategy and does not lead to stencil buffer overflow, which could cause rendering artifacts.
Adding comments to explain the rationale behind the chosen stencil operation values and ensuring that the implementation is robust against all possible combinations of masking conditions would improve code clarity and maintainability.
Summary by CodeRabbit