Add scale-in option for vector layer polygons #585
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThe recent updates focus on enhancing shader logic and rendering techniques for Android and iOS platforms, introducing Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
zimmermannubique
force-pushed
the
feature/fading-pattern
branch
from
66ba5c3 to
52b50c3
Compare
zimmermannubique
approved these changes
Feb 5, 2024
|
@maurhofer-ubique Da wäre iOS auch noch gemacht. Könnte man mergen. |
![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: 8
Configuration used: CodeRabbit UI
Files selected for processing (53)
- android/src/main/cpp/graphics/objects/PolygonPatternGroup2dOpenGl.cpp (1 hunks)
- android/src/main/cpp/graphics/shader/PolygonPatternGroup2dShaderOpenGl.cpp (4 hunks)
- android/src/main/cpp/graphics/shader/PolygonPatternGroup2dShaderOpenGl.h (2 hunks)
- android/src/main/cpp/graphics/shader/ShaderFactoryOpenGl.cpp (1 hunks)
- android/src/main/cpp/graphics/shader/ShaderFactoryOpenGl.h (1 hunks)
- android/src/main/java/io/openmobilemaps/mapscore/graphics/GLThread.kt (1 hunks)
- bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/shader/ShaderFactoryInterface.kt (2 hunks)
- bridging/android/java/io/openmobilemaps/mapscore/shared/map/MapCamera2dInterface.kt (2 hunks)
- bridging/android/java/io/openmobilemaps/mapscore/shared/map/layers/tiled/vector/Tiled2dMapVectorLayerInterface.kt (2 hunks)
- bridging/android/jni/graphics/shader/NativeShaderFactoryInterface.cpp (3 hunks)
- bridging/android/jni/graphics/shader/NativeShaderFactoryInterface.h (2 hunks)
- bridging/android/jni/map/NativeMapCamera2dInterface.cpp (1 hunks)
- bridging/android/jni/map/layers/tiled/vector/NativeTiled2dMapVectorLayerInterface.cpp (2 hunks)
- bridging/ios/MCMapCamera2dInterface+Private.mm (1 hunks)
- bridging/ios/MCMapCamera2dInterface.h (1 hunks)
- bridging/ios/MCShaderFactoryInterface+Private.mm (2 hunks)
- bridging/ios/MCShaderFactoryInterface.h (1 hunks)
- bridging/ios/MCTiled2dMapVectorLayerInterface+Private.mm (2 hunks)
- bridging/ios/MCTiled2dMapVectorLayerInterface.h (2 hunks)
- djinni/graphics/shader/shader.djinni (1 hunks)
- djinni/map/core.djinni (1 hunks)
- djinni/map/layers/tiled/vector/tiled_vector_layer.djinni (1 hunks)
- ios/graphics/Model/Polygon/PolygonPatternGroup2d.swift (5 hunks)
- ios/graphics/Pipelines/PipelineLibrary.swift (5 hunks)
- ios/graphics/Shader/Metal/PolygonGroupShader.metal (2 hunks)
- ios/graphics/Shader/PolygonPatternGroupShader.swift (1 hunks)
- ios/graphics/Shader/ShaderFactory.swift (1 hunks)
- shared/public/MapCamera2dInterface.h (1 hunks)
- shared/public/PolygonVectorLayerDescription.h (2 hunks)
- shared/public/ShaderFactoryInterface.h (1 hunks)
- shared/public/Tiled2dMapVectorLayer.h (1 hunks)
- shared/public/Tiled2dMapVectorLayerInterface.h (2 hunks)
- shared/public/Tiled2dMapVectorTile.h (1 hunks)
- shared/src/map/camera/MapCamera2d.cpp (2 hunks)
- shared/src/map/camera/MapCamera2d.h (2 hunks)
- shared/src/map/layers/tiled/vector/Tiled2dMapVectorLayer.cpp (1 hunks)
- shared/src/map/layers/tiled/vector/Tiled2dMapVectorLayerParserHelper.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/sourcemanagers/Tiled2dMapVectorSourceTileDataManager.h (2 hunks)
- shared/src/map/layers/tiled/vector/sublayers/background/Tiled2dMapVectorBackgroundSubLayer.cpp (1 hunks)
- shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorInteractionManager.cpp (1 hunks)
- shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorInteractionManager.h (1 hunks)
- shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSourceSymbolDataManager.cpp (4 hunks)
- shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSourceSymbolDataManager.h (1 hunks)
- shared/src/map/layers/tiled/vector/tiles/line/Tiled2dMapVectorLineTile.cpp (3 hunks)
- shared/src/map/layers/tiled/vector/tiles/line/Tiled2dMapVectorLineTile.h (1 hunks)
- shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonPatternTile.cpp (3 hunks)
- shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonPatternTile.h (2 hunks)
- shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonTile.cpp (1 hunks)
- shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonTile.h (1 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 errors (1)
- shared/src/map/layers/tiled/vector/tiles/Tiled2dMapVectorLineTile.cpp (no review received)
Additional comments: 53
android/src/main/cpp/graphics/shader/ShaderFactoryOpenGl.cpp (1)
- 53-54: Implementation of
createPolygonPatternGroupShader(bool fadeInPattern)correctly passes thefadeInPatternparameter to the shader constructor. This change aligns with the header file modification.shared/public/Tiled2dMapVectorLayerInterface.h (2)
- 6-6: Include directive for "Coord.h" is correctly added to support the new
performClickmethod.- 70-70: The addition of the
performClickvirtual function with aCoordparameter is correctly implemented to handle click events within vector layers.bridging/ios/MCTiled2dMapVectorLayerInterface.h (2)
- 4-4: Import statement for "MCCoord.h" correctly added to support the new
performClick:method in the iOS bridging interface.- 75-75: The addition of the
performClick:method with anMCCoordparameter in theMCTiled2dMapVectorLayerInterfaceprotocol is correctly implemented to handle click events within vector layers on iOS.shared/public/PolygonVectorLayerDescription.h (3)
- 23-29: The addition of the
fadeInPatternparameter to thePolygonVectorStyleconstructor is correctly implemented to support fading in patterns for vector layer polygons.- 35-36: The addition of the
fadeInPatternparameter to the copy constructor of thePolygonVectorStyleclass is correctly implemented, ensuring that the fading pattern feature is correctly copied.- 83-84: The addition of the
fadeInPatternmember variable in thePolygonVectorStyleclass is correctly implemented to store the state of the fading in pattern feature.bridging/android/jni/graphics/shader/NativeShaderFactoryInterface.h (1)
- 42-42: The addition of the
fadeInPatternparameter to thecreatePolygonPatternGroupShadermethod in theNativeShaderFactoryInterfaceclass is correctly implemented to support fading in patterns for vector layer polygons on Android.djinni/map/core.djinni (1)
- 159-159: The addition of the
screen_pos_from_coordmethod to themap_camera_2d_interfaceis correctly implemented to support converting coordinates to screen positions, enhancing interactive capabilities.bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/shader/ShaderFactoryInterface.kt (1)
- 23-23: The addition of the
fadeInPatternparameter to thecreatePolygonPatternGroupShadermethod in theShaderFactoryInterfaceKotlin interface is correctly implemented to support fading in patterns for vector layer polygons on Android.djinni/map/layers/tiled/vector/tiled_vector_layer.djinni (1)
- 67-67: The addition of the
perform_clickmethod to theTiledVectorLayerinterface is correctly implemented to support handling click events within vector layers.ios/graphics/Shader/Metal/PolygonGroupShader.metal (3)
- 60-61: The addition of
posOffsetas a constant buffer parameter is correctly implemented. Ensure that the corresponding Metal shader invocation on the iOS side is updated to pass this new parameter.- 62-64: Correct calculation of
pixelPositionusing the newposOffsetparameter. This change aligns with the PR objectives to enhance visual representation by adjusting calculations based on real-world measurements.- 95-163: The new
polygonPatternGroupFadeInFragmentShaderfunction is correctly added to handle fragment shader operations with fading in patterns. Ensure that the corresponding iOS code invoking this shader is updated to use this new function where necessary.shared/src/map/camera/MapCamera2d.h (2)
- 23-23: The inclusion of "Vec2F.h" is correctly added to support the new
screenPosFromCoordmethod, ensuring the necessary types are available.- 124-125: The
screenPosFromCoordmethod is correctly declared. Ensure that its implementation correctly converts map coordinates to screen positions, considering the current camera zoom, rotation, and pan state.bridging/android/java/io/openmobilemaps/mapscore/shared/map/layers/tiled/vector/Tiled2dMapVectorLayerInterface.kt (2)
- 43-44: The
performClickmethod is correctly declared in theTiled2dMapVectorLayerInterface. Ensure that its implementation in both the native and proxy classes correctly handles click events on vector layers.- 132-136: The implementation of
performClickin theCppProxyclass is correctly added. Ensure that the native methodnative_performClickis correctly implemented to handle click events on vector layers.ios/graphics/Model/Polygon/PolygonPatternGroup2d.swift (3)
- 26-26: The
renderPassStencilStateproperty is correctly added to support different stencil states during rendering. Ensure that this property is properly utilized in rendering logic to handle masked passes.- 33-34: The
posOffsetproperty is correctly added. Ensure that its value is correctly calculated and set before rendering, based on vertex data.- 71-117: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [48-160]
The modifications in the
rendermethod to handle therenderPassparameter and additional logic for masked passes are correctly implemented. Ensure that the rendering logic correctly applies therenderPassStencilStateandposOffsetduring rendering operations.bridging/ios/MCTiled2dMapVectorLayerInterface+Private.mm (2)
- 9-9: The inclusion of "MCCoord+Private.h" is correctly added to support the new
performClickmethod, ensuring the necessary types are available for bridging.- 159-163: The
performClickmethod is correctly implemented in the Objective-C bridging layer. Ensure that it correctly translates theMCCoordparameter and calls the corresponding C++ method.android/src/main/cpp/graphics/objects/PolygonPatternGroup2dOpenGl.cpp (1)
- 169-172: The introduction of the
uScreenPixelAsRealMeterFactoruniform variable is correctly implemented. Ensure that the corresponding shader code on the Android side is updated to utilize this new variable for accurate scale-in effects.shared/src/map/layers/tiled/vector/sublayers/background/Tiled2dMapVectorBackgroundSubLayer.cpp (1)
- 45-45: Ensure the hardcoded
falsevalue passed tocreatePolygonPatternGroupShaderaligns with the intended shader behavior. Consider if this should be dynamically determined based on some condition.bridging/ios/MCShaderFactoryInterface+Private.mm (1)
- 88-90: The update to pass
fadeInPatternfrom Objective-C to C++ increatePolygonPatternGroupShaderis correctly implemented.android/src/main/cpp/graphics/shader/PolygonPatternGroup2dShaderOpenGl.cpp (4)
- 19-21: The constructor correctly initializes
fadeInPatternandprogramNamebased on the provided boolean. This dynamic naming strategy is a good practice for managing different shader behaviors.- 54-67: Ensure the conditional logic for
fadeInPatternin the vertex shader correctly implements the intended behavior for both fading in patterns and the default behavior. The separation of logic based onfadeInPatternis clear and well-structured.- 82-84: The fragment shader's conditional inclusion of uniforms based on
fadeInPatternis correctly implemented. This approach ensures that only necessary uniforms are declared, optimizing shader performance.- 109-168: The complex conditional logic in the fragment shader for handling
fadeInPatternis well-structured. However, ensure that the calculations forspacing,totalSize, and texture coordinates accurately achieve the desired visual effects, especially in edge cases.bridging/ios/MCMapCamera2dInterface+Private.mm (1)
- 250-255: The addition of
screenPosFromCoordmethod correctly bridges Objective-C to C++, enabling coordinate to screen position conversions. Ensure the underlying C++ logic accurately performs the conversion.android/src/main/java/io/openmobilemaps/mapscore/graphics/GLThread.kt (1)
- 393-393: Adding
requestRender()inonWindowResizeis correctly implemented to ensure the GL surface is updated immediately after a size change. This is crucial for responsive rendering.bridging/android/jni/map/layers/tiled/vector/NativeTiled2dMapVectorLayerInterface.cpp (2)
- 6-6: Including "NativeCoord.h" is necessary for the new
native_performClickfunction to work withNativeCoordobjects.- 160-166: The implementation of
native_performClickcorrectly translates the JNI environment and object references to call theperformClickmethod on theTiled2dMapVectorLayerInterface. The usage of::djinni_generated::NativeCoord::toCppfor parameter conversion is appropriate.shared/public/Tiled2dMapVectorLayer.h (1)
- 158-159: The addition of the
performClickmethod with aCoordparameter is consistent with the class's functionality to handle click events on vector layers. This method enhances interactive capabilities.bridging/android/jni/graphics/shader/NativeShaderFactoryInterface.cpp (3)
- 5-5: Including "Marshal.hpp" is necessary for the conversion functions used in the modified
createPolygonPatternGroupShadermethod.- 77-82: Modifying
createPolygonPatternGroupShaderto accept abool c_fadeInPatternparameter allows for the creation of shaders with optional fade-in patterns, aligning with the feature enhancements described.- 188-192: The JNI method
native_createPolygonPatternGroupShadercorrectly accepts ajboolean j_fadeInPatternand translates it for use in the modifiedcreatePolygonPatternGroupShadermethod. This change supports the new fade-in pattern functionality.shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonTile.cpp (2)
- 323-328: Refactoring the click handling logic into
performClickand checking for camera existence inonClickConfirmedimproves code modularity and clarity. This change ensures that click events are only processed when a camera is available.- 330-351: The implementation of
performClickcorrectly processes click events based on coordinates, enhancing the class's interactive capabilities. The method's logic for feature selection and multi-selection is consistent with expected behavior.bridging/android/java/io/openmobilemaps/mapscore/shared/map/MapCamera2dInterface.kt (1)
- 73-74: LGTM!
bridging/android/jni/map/NativeMapCamera2dInterface.cpp (1)
- 283-290: LGTM!
shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonPatternTile.cpp (2)
- 31-32: Initialization of
fadeInPatternin the constructor is correct and follows best practices.- 417-438: The
performClickmethod correctly implements logic for handling click events based on coordinates. It properly locks the map interface and checks for a strong selection delegate before proceeding, which is a good practice for thread safety and ensuring the presence of necessary components.shared/src/map/layers/tiled/vector/sourcemanagers/Tiled2dMapVectorSourceTileDataManager.cpp (1)
- 420-442: The
performClickmethod is implemented correctly, iterating over tiles and sub-tiles to handle click events based on coordinates. It properly checks for interactable layers and visibility before attempting to perform click actions, which is essential for accurate event handling.shared/src/map/layers/tiled/vector/Tiled2dMapVectorLayerParserHelper.cpp (1)
- 383-384: The addition of
fadeInPatternto thePolygonVectorStyleconstructor is implemented correctly. It checks for the presence offadeInPatternin the layout and correctly initializes the style parameter based on its value. This change supports the new functionality for fading in patterns as intended.shared/src/map/camera/MapCamera2d.cpp (3)
- 737-764: The method
screenPosFromCoordcorrectly calculates the screen position from map coordinates considering the current zoom, angle, and center position. However, ensure that the conversion logic aligns with the expected coordinate system and rendering context, especially regarding the angle's sign and the viewport size's impact on the final screen position.- 734-768: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [766-769]
The method
mapUnitsFromPixelscorrectly calculates the map units from pixels using thescreenPixelAsRealMeterFactorand the current zoom level. This implementation is efficient and straightforward.
- 19-25: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [770-771]
The method
getScalingFactoris a simple wrapper aroundmapUnitsFromPixels(1.0), effectively returning the scaling factor for a single pixel. This is a clear and efficient way to expose this functionality.shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSourceSymbolDataManager.cpp (2)
- 754-765: The
onClickConfirmedmethod correctly locks the shared pointer tothis, acquires necessary interfaces, and then callsperformClickwith the converted coordinates. Ensure that all necessary null checks are performed before using the objects.- 788-788: The use of a delegate to handle feature selection upon a click is a good practice, promoting decoupling and flexibility in handling different actions upon selection.
shared/src/map/layers/tiled/vector/Tiled2dMapVectorLayer.cpp (1)
- 1123-1133: The
performClickmethod correctly converts a coordinate to a screen position and then callsonClickConfirmedwith the screen position. This logic is sound for simulating a click event based on a map coordinate. However, ensure thatonClickConfirmedis designed to handle programmatically generated events in addition to user-generated events, as this could affect application behavior.
| @@ -19,6 +19,7 @@ | |||
| #include "Vec2D.h" | |||
| #include "Vec2DHelper.h" | |||
| #include "Vec2FHelper.h" | |||
| #include "Logger.h" | |||
There was a problem hiding this comment.
Including "Logger.h" without using any logging functionality in the provided changes. Ensure that logging is either implemented where necessary or remove the unused include.
Comment on lines
774
to
780
| return false; | ||
| } | ||
|
|
||
| Coord clickCoords = camera->coordFromScreenPosition(posScreen); | ||
| Coord clickCoordsRenderCoord = conversionHelper->convertToRenderSystem(clickCoords); | ||
| Coord clickCoordsRenderCoord = conversionHelper->convertToRenderSystem(coord); | ||
|
|
||
| double clickPadding = camera->mapUnitsFromPixels(16); | ||
| CircleD clickHitCircle(clickCoordsRenderCoord.x, clickCoordsRenderCoord.y, clickPadding); |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [767-788]
In performClick, the method efficiently filters out non-interactable layers and layers not included in the layers set. It then iterates over symbol groups to find a hit. Consider optimizing the loop for large numbers of symbol groups or tiles, potentially by spatial indexing or early exit strategies if a hit is found.
| @@ -760,8 +774,7 @@ | |||
| return false; | |||
| } | |||
|
|
|||
| Coord clickCoords = camera->coordFromScreenPosition(posScreen); | |||
| Coord clickCoordsRenderCoord = conversionHelper->convertToRenderSystem(clickCoords); | |||
| Coord clickCoordsRenderCoord = conversionHelper->convertToRenderSystem(coord); | |||
There was a problem hiding this comment.
The click padding is hardcoded to 16 pixels. Consider making this value configurable or adaptive to screen density and zoom levels to improve usability across different devices.
| @@ -21,6 +21,8 @@ class PolygonPatternGroup2dShaderOpenGl : public BaseShaderProgramOpenGl, | |||
| public std::enable_shared_from_this<ShaderProgramInterface> { | |||
|
|
|||
| public: | |||
| PolygonPatternGroup2dShaderOpenGl(bool fadeInPattern); | |||
There was a problem hiding this comment.
Constructor PolygonPatternGroup2dShaderOpenGl(bool fadeInPattern) introduces a boolean parameter to control fading in patterns. Ensure all shader instances are correctly initialized with this new parameter.
Comment on lines
+35
to
+36
| const bool fadeInPattern; | ||
| const std::string programName; |
There was a problem hiding this comment.
Member variable const bool fadeInPattern correctly stores the fading pattern state. However, const std::string programName should be initialized in the constructor or have a default value to ensure it's always in a valid state.
| override init() { | ||
| // MARK: - Variables | ||
|
|
||
| let fadeInPattern : Bool |
There was a problem hiding this comment.
Variable fadeInPattern correctly captures the fading pattern state. Ensure it is used consistently across the class to control shader behavior.
| } | ||
| guard pipeline == nil else { return } | ||
|
|
||
| let t : PipelineType = fadeInPattern ? .polygonPatternFadeInGroupShader : .polygonPatternGroupShader |
There was a problem hiding this comment.
Modification in setupProgram to conditionally set the pipeline based on fadeInPattern is correct. Verify that all shader pipelines correctly support this conditional logic.
| @@ -27,7 +27,7 @@ class ShaderFactoryOpenGl : public ShaderFactoryInterface { | |||
|
|
|||
| std::shared_ptr<PolygonGroupShaderInterface> createPolygonGroupShader() override; | |||
|
|
|||
| std::shared_ptr<PolygonPatternGroupShaderInterface> createPolygonPatternGroupShader() override; | |||
| std::shared_ptr<PolygonPatternGroupShaderInterface> createPolygonPatternGroupShader(bool fadeInPattern) override; | |||
There was a problem hiding this comment.
Method signature update to include bool fadeInPattern in createPolygonPatternGroupShader is correct. Ensure downstream usage of this method correctly passes the new parameter.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Refactor
Documentation