Get Indexed Layers / Restrict Camera Visible Rect #592
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 introduce new functionalities across the map and camera interfaces, focusing on indexed layers and bounds restrictions. These changes enhance the map's capabilities by allowing better layer management and control over the visible map area. The introduction of 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 (
|
maurhofer-ubique
force-pushed
the
feature/indexed-layers-restrictive-camera-bounds
branch
from
8586653 to
2f77f6c
Compare
maurhofer-ubique
force-pushed
the
feature/indexed-layers-restrictive-camera-bounds
branch
from
2f77f6c to
dbd0a9f
Compare
![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: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
djinni/yaml/indexed_layer.yamlis excluded by:!**/*.yamldjinni/yaml/indexed_layer_interface.yamlis excluded by:!**/*.yaml
Files selected for processing (24)
- bridging/android/java/io/openmobilemaps/mapscore/shared/map/IndexedLayerInterface.kt (1 hunks)
- bridging/android/java/io/openmobilemaps/mapscore/shared/map/MapCamera2dInterface.kt (2 hunks)
- bridging/android/java/io/openmobilemaps/mapscore/shared/map/MapInterface.kt (2 hunks)
- bridging/android/jni/map/NativeIndexedLayerInterface.cpp (1 hunks)
- bridging/android/jni/map/NativeIndexedLayerInterface.h (1 hunks)
- bridging/android/jni/map/NativeMapCamera2dInterface.cpp (1 hunks)
- bridging/android/jni/map/NativeMapInterface.cpp (2 hunks)
- bridging/ios/MCIndexedLayerInterface+Private.h (1 hunks)
- bridging/ios/MCIndexedLayerInterface+Private.mm (1 hunks)
- bridging/ios/MCIndexedLayerInterface.h (1 hunks)
- bridging/ios/MCMapCamera2dInterface+Private.mm (1 hunks)
- bridging/ios/MCMapCamera2dInterface.h (1 hunks)
- bridging/ios/MCMapInterface+Private.mm (2 hunks)
- bridging/ios/MCMapInterface.h (2 hunks)
- djinni/map/core.djinni (3 hunks)
- shared/public/IndexedLayerInterface.h (1 hunks)
- shared/public/MapCamera2dInterface.h (1 hunks)
- shared/public/MapInterface.h (2 hunks)
- shared/src/map/MapScene.cpp (3 hunks)
- shared/src/map/MapScene.h (1 hunks)
- shared/src/map/camera/MapCamera2d.cpp (11 hunks)
- shared/src/map/camera/MapCamera2d.h (3 hunks)
- shared/src/map/layers/IndexedLayer.cpp (1 hunks)
- shared/src/map/layers/IndexedLayer.h (1 hunks)
Files skipped from review due to trivial changes (3)
- bridging/ios/MCIndexedLayerInterface+Private.h
- bridging/ios/MCIndexedLayerInterface.h
- shared/public/IndexedLayerInterface.h
Additional comments: 37
shared/src/map/layers/IndexedLayer.h (1)
- 1-26: LGTM!
shared/src/map/layers/IndexedLayer.cpp (1)
- 1-22: LGTM!
bridging/android/java/io/openmobilemaps/mapscore/shared/map/IndexedLayerInterface.kt (1)
- 1-41: LGTM!
bridging/android/jni/map/NativeIndexedLayerInterface.h (1)
- 1-47: LGTM!
bridging/ios/MCMapCamera2dInterface.h (1)
- 95-95: LGTM!
shared/public/MapCamera2dInterface.h (1)
- 88-88: LGTM!
bridging/android/jni/map/NativeIndexedLayerInterface.cpp (1)
- 1-59: LGTM!
bridging/ios/MCMapInterface.h (1)
- 59-59: LGTM!
bridging/ios/MCIndexedLayerInterface+Private.mm (1)
- 1-102: LGTM!
shared/src/map/MapScene.h (1)
- 50-50: LGTM!
shared/public/MapInterface.h (1)
- 59-59: LGTM!
bridging/ios/MCMapInterface+Private.mm (1)
- 155-160: LGTM!
djinni/map/core.djinni (3)
- 42-42: LGTM!
- 71-74: LGTM!
- 170-170: LGTM!
shared/src/map/camera/MapCamera2d.h (3)
- 134-134: LGTM!
- 183-183: LGTM!
- 223-223: LGTM!
bridging/ios/MCMapCamera2dInterface+Private.mm (1)
- 276-280: LGTM!
bridging/android/java/io/openmobilemaps/mapscore/shared/map/MapInterface.kt (2)
- 43-44: LGTM!
- 167-172: LGTM!
bridging/android/jni/map/NativeMapInterface.cpp (2)
- 9-9: LGTM!
- 163-171: LGTM!
shared/src/map/MapScene.cpp (1)
- 98-105: LGTM!
bridging/android/java/io/openmobilemaps/mapscore/shared/map/MapCamera2dInterface.kt (2)
- 81-82: The addition of
setBoundsRestrictWholeVisibleRectaligns with the PR objectives to enhance camera control functionalities by allowing the restriction of the camera's visible rectangle. Ensure corresponding implementations handle this new abstract method appropriately.- 300-305: The implementation of
setBoundsRestrictWholeVisibleRectin the CppProxy class correctly asserts object validity before calling the native method. This ensures the method is not called on a destroyed object, aligning with the error handling pattern used throughout the class.bridging/android/jni/map/NativeMapCamera2dInterface.cpp (1)
- 317-323: The addition of
native_setBoundsRestrictWholeVisibleRectcorrectly implements the JNI bridge for the newly addedsetBoundsRestrictWholeVisibleRectmethod in the Kotlin interface. The use of::djinni::Bool::toCppfor converting the JNI boolean parameter to C++ is consistent with the rest of the file.shared/src/map/camera/MapCamera2d.cpp (10)
- 76-82: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [79-98]
The use of structured bindings for
positionMapSystemandadjustedZoomfromgetBoundsCorrectedCoordsis a good practice for readability and maintainability. However, ensure that all calls tosetZoomwithin the class now correctly useadjustedZoomto maintain consistency.
- 111-117: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [114-133]
Similar to the previous comment, the structured bindings usage here is consistent and improves code clarity. It's important to verify that the animation logic correctly handles the adjusted position and zoom, especially in the completion handlers where the final state is set.
- 263-269: The adjustment of
coordAnimationandzoomAnimationend values based on padding corrections is a critical update for ensuring animations reflect the actual visible area considering padding. This change enhances the responsiveness and accuracy of camera movements. Ensure that similar logic is applied consistently wherever animations are used.- 279-285: This segment mirrors the adjustments made for
setPaddingLeft, applied tosetPaddingRight. Consistency in handling padding adjustments for all sides is crucial for maintaining a uniform user experience. Verify that padding values are validated to prevent negative or excessively large values that could distort the camera view.- 295-301: The adjustments for
setPaddingTopfollow the same pattern as for other padding setters, ensuring that the camera's view is correctly adjusted for padding at the top. Consistency in handling across different padding directions is maintained. Again, validate padding inputs.- 311-317: Adjustments in
setPaddingBottomare consistent with the handling of other padding directions. This ensures that the camera's view accounts for bottom padding correctly. It's important to maintain this consistency across all padding setters for a coherent user experience.- 821-823: The call to
getBoundsCorrectedCoordswithinsetBoundsensures that when the map bounds are updated, the camera's center position and zoom are also adjusted accordingly to remain within the new bounds. This is crucial for preventing the camera from displaying areas outside the defined map bounds.- 843-893: The
getBoundsCorrectedCoordsmethod's implementation to handle both scenarios ofconfig.boundsRestrictWholeVisibleRectbeing true or false is comprehensive. However, ensure that the calculations fortargetScaleDiffFactorXandtargetScaleDiffFactorYcorrectly account for the visible area's aspect ratio to prevent distortion. Additionally, verify that the adjustments made tonewPositionandnewZoomare accurate and maintain the intended view within bounds.- 927-929: The call to
getBoundsCorrectedCoordswithinclampCenterToPaddingCorrectedBoundsensures that any adjustments to the camera's position or zoom factor due to changes in padding or bounds are correctly applied. This method's invocation in various setters helps maintain the camera's view within the desired constraints.- 937-940: The addition of
setBoundsRestrictWholeVisibleRectallows for dynamic toggling of how bounds restrictions are applied to the visible rectangle. This flexibility is beneficial for different mapping scenarios. Ensure that this setting is correctly applied across all relevant camera adjustments to maintain consistent behavior.
zimmermannubique
approved these changes
Feb 8, 2024
zimmermannubique
deleted the
feature/indexed-layers-restrictive-camera-bounds
branch
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