Bugfix/multi buffers #653
Bugfix/multi buffers #653
Conversation
Signed-off-by: Nicolas Märki <maerki@ubique.ch>
Signed-off-by: Nicolas Märki <maerki@ubique.ch>
Signed-off-by: Nicolas Märki <maerki@ubique.ch>
maerki
requested review from
stmitt,
bastianmorath,
maurhofer-ubique and
zimmermannubique
WalkthroughThe changes introduced in this pull request primarily enhance the functionality and structure of various classes related to map rendering and camera management. Key additions include new methods for retrieving scaling factors in multiple interfaces, improved buffer management in rendering classes, and enhancements to error handling and thread safety. The modifications span across multiple files, including both Java and C++ implementations, as well as Swift files, reflecting a comprehensive update to the rendering pipeline and camera interface. 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Signed-off-by: Nicolas Märki <maerki@ubique.ch>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 33
🧹 Outside diff range and nitpick comments (20)
ios/graphics/Shader/Metal/BaseShader.metal (1)
52-54: Consider performance implications of unused shader parameters.Even unused parameters in Metal shaders can consume precious register space and might affect shader compilation optimization. If these debug parameters are not actively used, removing them could lead to better shader performance.
shared/public/MapCameraInterface.h (1)
91-92: Add documentation for the new method.Please document the purpose of
getScalingFactor():
- What does the scaling factor represent?
- What are the expected return value ranges?
- When should this method be called?
ios/graphics/Model/Text/Text.swift (2)
Line range hint
13-13: Document thread safety implementation.The class is marked as
@unchecked Sendablebut lacks documentation explaining the thread safety guarantees and why manual implementation is needed.Add a documentation comment explaining the thread safety implementation:
+/// A thread-safe text rendering object that manages its own synchronization through internal locks. +/// Marked as `@unchecked Sendable` as thread safety is manually implemented using locks +/// for buffer and texture access. final class Text: BaseGraphicsObject, @unchecked Sendable {
Line range hint
155-156: Implement or document the emptyremoveTexture()method.The empty implementation might indicate missing cleanup logic for texture resources.
Would you like me to help implement proper texture cleanup logic? This should include:
- Proper texture resource cleanup
- Thread-safe texture state management
ios/graphics/Model/Icosahedron/Icosahedron.swift (1)
72-77: Consider documenting the buffer management strategy.The switch from direct byte setting to buffer management through
vpMatrixBuffersis a significant architectural change. Consider adding documentation that explains:
- The rationale behind using multiple buffers
- The lifecycle of these buffers
- Any performance considerations or threading implications
Also applies to: 126-131
ios/graphics/Model/Polygon/PolygonGroup2d.swift (1)
84-99: Consider implementing a safer buffer management abstraction.The current changes introduce better buffer management but still rely on low-level Metal buffer operations. Consider creating a higher-level abstraction for these buffer operations to:
- Encapsulate safety checks
- Provide type-safe access to buffer contents
- Handle synchronization
- Manage buffer lifecycle
This would make the code more maintainable and reduce the risk of memory-related issues.
Consider creating a generic buffer wrapper like:
class SafeMetalBuffer<T> { private let buffer: MTLBuffer private let lock = NSLock() func withSafeAccess(_ block: (UnsafeMutablePointer<T>) -> Void) { lock.lock() defer { lock.unlock() } guard let contents = buffer.contents().assumingMemoryBound(to: T.self) else { assertionFailure("Buffer access failed") return } block(contents) } }ios/graphics/Model/Line/LineGroup2d.swift (2)
Line range hint
125-131: Improve buffer handling safety and maintainability.Several improvements can be made to this section:
- Add null check for originOffsetBuffer
- Define buffer indices as constants for better maintainability
+private enum BufferIndex { + static let vertices = 0 + static let vpMatrix = 1 + static let originOffset = 5 + static let tileOrigin = 6 +} let originOffsetBuffer = originOffsetBuffers.getNextBuffer(context) -if let bufferPointer = originOffsetBuffer?.contents().assumingMemoryBound(to: simd_float4.self) { +guard let originOffsetBuffer = originOffsetBuffer, + let bufferPointer = originOffsetBuffer.contents().assumingMemoryBound(to: simd_float4.self) else { + return +} bufferPointer.pointee.x = Float(originOffset.x - origin.x) bufferPointer.pointee.y = Float(originOffset.y - origin.y) bufferPointer.pointee.z = Float(originOffset.z - origin.z) -} -encoder.setVertexBuffer(originOffsetBuffer, offset: 0, index: 5) -encoder.setVertexBuffer(tileOriginBuffer, offset: 0, index: 6) +encoder.setVertexBuffer(originOffsetBuffer, offset: 0, index: BufferIndex.originOffset) +encoder.setVertexBuffer(tileOriginBuffer, offset: 0, index: BufferIndex.tileOrigin)
Line range hint
14-14: Document thread-safety guarantees and lock usage.The class is marked as
@unchecked Sendablewhich indicates manual thread-safety verification. Consider:
- Adding documentation about thread-safety guarantees
- Documenting the lock usage pattern
- Consider using
NSRecursiveLockif recursive locking is neededAdd documentation like:
/// LineGroup2d handles 2D line rendering using Metal. /// Thread-safety: This class is thread-safe through internal locking. /// - Important: All public methods are protected by an internal lock. /// Recursive calls within the same thread are not supported. final class LineGroup2d: BaseGraphicsObject, @unchecked Sendable {ios/graphics/RenderingContext.swift (1)
23-25: Add documentation for frame managementThe
beginFrame()method lacks documentation explaining:
- Its purpose in the rendering pipeline
- When it should be called
- Why 1000 was chosen as the wrap-around value
- Thread-safety guarantees
Consider adding a documentation comment:
+/// Advances the frame counter for the rendering context. +/// This method should be called at the beginning of each new frame. +/// The frame ID wraps around to 0 after reaching 999 to prevent overflow. +/// - Note: This method must be called from the main thread or the Metal render thread. public func beginFrame() { frameId = (frameId + 1) % 1000 }djinni/map/core.djinni (1)
184-185: Add documentation for the new method.The new
get_scaling_factor()method lacks documentation explaining its purpose, expected return values, and usage context. This is particularly important as it's part of a public interface used across different platforms.Add documentation comment above the method:
+ # Returns the current scaling factor used for rendering transformations. + # The value affects how coordinates are transformed between screen space and map space. get_scaling_factor(): f64;ios/graphics/Model/Polygon/PolygonPatternGroup2d.swift (1)
Line range hint
103-123: Refactor scale factor handling for better maintainability.The current implementation has duplicated logic and could be simplified for better readability.
Consider this refactoring:
-var pixelFactor: Float = Float(screenPixelAsRealMeterFactor) - -if self.shader.fadeInPattern { - var scaleFactors = SIMD2<Float>([pixelFactor, pixelFactor]) - encoder.setVertexBytes(&scaleFactors, length: MemoryLayout<SIMD2<Float>>.stride, index: 2) - encoder.setVertexBytes(&posOffset, length: MemoryLayout<SIMD2<Float>>.stride, index: 3) - - scaleFactors = customScreenPixelFactor.x != 0 ? customScreenPixelFactor : SIMD2<Float>([pixelFactor, pixelFactor]) - encoder.setFragmentBytes(&pixelFactor, length: MemoryLayout<Float>.stride, index: 2) - encoder.setFragmentBytes(&scaleFactors, length: MemoryLayout<SIMD2<Float>>.stride, index: 3) -} else { - var scaleFactors = customScreenPixelFactor.x != 0 ? customScreenPixelFactor : SIMD2<Float>([pixelFactor, pixelFactor]) - encoder.setVertexBytes(&scaleFactors, length: MemoryLayout<SIMD2<Float>>.stride, index: 2) - encoder.setVertexBytes(&posOffset, length: MemoryLayout<SIMD2<Float>>.stride, index: 3) -} +let pixelFactor = Float(screenPixelAsRealMeterFactor) +let defaultScaleFactors = SIMD2<Float>([pixelFactor, pixelFactor]) +let scaleFactors = customScreenPixelFactor.x != 0 ? customScreenPixelFactor : defaultScaleFactors + +// Always set vertex bytes +encoder.setVertexBytes(&scaleFactors, length: MemoryLayout<SIMD2<Float>>.stride, index: 2) +encoder.setVertexBytes(&posOffset, length: MemoryLayout<SIMD2<Float>>.stride, index: 3) + +// Set additional fragment bytes only for fade-in patterns +if shader.fadeInPattern { + encoder.setFragmentBytes(&pixelFactor, length: MemoryLayout<Float>.stride, index: 2) + encoder.setFragmentBytes(&scaleFactors, length: MemoryLayout<SIMD2<Float>>.stride, index: 3) +}ios/graphics/Model/BaseGraphicsObject.swift (2)
26-26: Remove trailing semicolons.Swift style guidelines discourage the use of trailing semicolons.
Apply these changes:
-var initialMutable = simd_float4x4(1.0); +var initialMutable = simd_float4x4(1.0) -var initialMutable = simd_float4(0, 0, 0, 0); +var initialMutable = simd_float4(0, 0, 0, 0) -var initialMutable = simd_float1(0); +var initialMutable = simd_float1(0)Also applies to: 59-59, 92-92
🧰 Tools
🪛 SwiftLint
[Warning] 26-26: Lines should not have trailing semicolons
(trailing_semicolon)
136-139: Add documentation for public properties and clarify thread safety.The new public properties lack documentation explaining their purpose and thread-safety guarantees. This is especially important given the class is marked as
@unchecked Sendable.Consider adding documentation:
/// Buffer containing origin offset data for triple buffering. /// Thread-safe when accessed within the rendering cycle while holding the `lock`. public var originOffsetBuffers: MultiBufferFloat4 /// Buffer containing view-projection matrices for triple buffering. /// Thread-safe when accessed within the rendering cycle while holding the `lock`. public var vpMatrixBuffers: MultiBufferFloat4x4 /// Buffer containing model matrices for triple buffering. /// Thread-safe when accessed within the rendering cycle while holding the `lock`. public var mMatrixBuffers: MultiBufferFloat4x4ios/graphics/Model/Quad/Quad2dStretchedInstanced.swift (1)
Line range hint
124-147: Consider implementing a robust buffer management system.While the transition to using multiple buffers is a good architectural decision, consider implementing a dedicated buffer management system that:
- Provides safe buffer access patterns
- Implements proper error handling and recovery
- Manages buffer lifecycle and synchronization
- Validates buffer sizes and memory bounds
This would help centralize the buffer management logic and reduce the risk of memory-related issues.
Would you like assistance in designing this buffer management system?
shared/src/map/camera/MapCamera3d.h (1)
187-190: Consider documenting the camera calculation methods.These new protected methods handle core camera calculations but lack documentation about their purpose and relationships:
getCameraDistancegetCameraFieldOfViewgetCameraVerticalDisplacementgetCameraPitchConsider adding documentation to explain:
- The purpose of each method
- The units of measurement used
- The relationships between these calculations
ios/graphics/Shader/Metal/LineShader.metal (1)
85-87: Consider adding documentation for debug parameters.The parameter addition follows Metal's best practices and maintains proper buffer binding indices. However, adding documentation comments would help other developers understand:
- The purpose of these debug parameters
- Expected coordinate space (world space, screen space, etc.)
- When and how these parameters should be used for debugging
Add documentation above the function:
/// Debug parameters: /// @param debugTileOrigin Origin of the current tile for debugging visualization /// @param debugRenderOrigin Origin of the render space for debugging visualizationios/maps/MCMapView.swift (1)
29-29: LGTM! Triple buffering implementation looks good.The semaphore initialization with a value of 3 correctly implements triple buffering, which will help prevent screen tearing and ensure smooth rendering by allowing up to three frames to be in flight simultaneously.
Note that triple buffering increases memory usage compared to double buffering. Monitor memory usage in low-memory conditions, especially on devices with limited memory.
bridging/android/java/io/openmobilemaps/mapscore/shared/map/MapCameraInterface.kt (1)
83-83: Add documentation for the new method.The new
getScalingFactor()method lacks documentation explaining its purpose and usage. Consider adding a KDoc comment describing what the scaling factor represents and how it should be used.ios/graphics/Model/Text/TextInstanced.swift (1)
40-41: Initialize buffers with proper error handling.When initializing
originBuffersandaspectRatioBuffers, consider handling potential initialization failures. Although unlikely, ifMultiBufferFloat4orMultiBufferFloat1fail to initialize due to resource constraints, the absence of error handling could lead to unexpected behavior.shared/src/map/camera/MapCamera3d.cpp (1)
Line range hint
1628-1644: Potential Missing Return Statement Leading to Undefined BehaviorIn the
valueForZoomfunction, if none of the conditions are met within the loop, the function reaches a point where no return statement is executed, which leads to undefined behavior.Ensure that the function always returns a value. Consider adding a default return statement at the end or handling this case appropriately.
#if __cplusplus >= 202302L std::unreachable(); #else __builtin_unreachable(); +#endif + return values.back().value; // Added default return to prevent undefined behavior #endif
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (25)
bridging/android/java/io/openmobilemaps/mapscore/shared/map/MapCameraInterface.kt(2 hunks)bridging/android/jni/map/NativeMapCameraInterface.cpp(1 hunks)bridging/ios/MCMapCameraInterface+Private.mm(1 hunks)bridging/ios/MCMapCameraInterface.h(1 hunks)djinni/map/core.djinni(1 hunks)ios/graphics/Model/BaseGraphicsObject.swift(2 hunks)ios/graphics/Model/Icosahedron/Icosahedron.swift(2 hunks)ios/graphics/Model/Line/LineGroup2d.swift(3 hunks)ios/graphics/Model/Polygon/Polygon2d.swift(4 hunks)ios/graphics/Model/Polygon/PolygonGroup2d.swift(1 hunks)ios/graphics/Model/Polygon/PolygonPatternGroup2d.swift(2 hunks)ios/graphics/Model/Quad/Quad2d.swift(1 hunks)ios/graphics/Model/Quad/Quad2dInstanced.swift(4 hunks)ios/graphics/Model/Quad/Quad2dStretchedInstanced.swift(2 hunks)ios/graphics/Model/Text/Text.swift(1 hunks)ios/graphics/Model/Text/TextInstanced.swift(3 hunks)ios/graphics/RenderingContext.swift(1 hunks)ios/graphics/Shader/Metal/BaseShader.metal(1 hunks)ios/graphics/Shader/Metal/LineShader.metal(1 hunks)ios/maps/MCMapView.swift(4 hunks)shared/public/MapCameraInterface.h(1 hunks)shared/public/Tiled2dMapSourceImpl.h(44 hunks)shared/src/map/camera/MapCamera3d.cpp(62 hunks)shared/src/map/camera/MapCamera3d.h(9 hunks)shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSourceSymbolDataManager.cpp(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- shared/public/Tiled2dMapSourceImpl.h
🧰 Additional context used
🪛 SwiftLint
ios/graphics/Model/BaseGraphicsObject.swift
[Warning] 26-26: Lines should not have trailing semicolons
(trailing_semicolon)
[Warning] 59-59: Lines should not have trailing semicolons
(trailing_semicolon)
[Warning] 92-92: Lines should not have trailing semicolons
(trailing_semicolon)
🔇 Additional comments (23)
bridging/ios/MCMapCameraInterface.h (2)
101-101: LGTM! Method signature is consistent across platforms.
The addition of getScalingFactor method aligns well with the interface's purpose and maintains consistency with the corresponding implementations in Android/JNI.
Line range hint 1-1: Reminder: File is auto-generated.
The header indicates this file is auto-generated from core.djinni. Ensure that changes are made in the Djinni definition file rather than directly in this file to maintain consistency across regenerations.
✅ Verification successful
Based on the evidence gathered, I can now provide a final response:
Confirmed: File is auto-generated from core.djinni
The file MCMapCameraInterface.h is indeed auto-generated from core.djinni. This is verified by:
- The presence of the auto-generation notice at the top of the file
- The explicit source reference "This file was generated by Djinni from core.djinni"
- The matching interface definition in
core.djinni
Any modifications should be made in the source core.djinni file rather than directly in the generated header file to maintain consistency and prevent changes from being overwritten during the next code generation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of the Djinni source file and check if it contains the scaling factor definition
# Test: Look for the Djinni source file
fd -g "*.djinni" -x grep -l "getScalingFactor"
Length of output: 46
Script:
#!/bin/bash
# Let's first find all .djinni files in the repository
fd -e djinni
# Then let's check the content of MCMapCamera.djinni if it exists
fd -g "MCMapCamera.djinni" -X cat
# Also check for any references to the djinni file in build scripts or documentation
rg -i "djinni" -g "!*.{h,cpp,mm}" -A 3
Length of output: 411505
Script:
#!/bin/bash
# Let's check if the file is actually auto-generated by looking for core.djinni
fd -g "core.djinni" -X cat
# Also check if there are any references to MCMapCameraInterface in core.djinni
fd -g "core.djinni" -X grep -A 5 "MCMapCameraInterface"
# Let's also check the actual content of the file in question
cat bridging/ios/MCMapCameraInterface.h
Length of output: 14520
shared/public/MapCameraInterface.h (1)
Line range hint 1-1: Warning: This is an auto-generated file.
The file header indicates this is auto-generated from core.djinni. Please ensure that changes are made to the Djinni IDL file instead of directly modifying this header.
ios/graphics/Model/Text/Text.swift (1)
86-91: ```shell
#!/bin/bash
Let's try a broader search for matrix-related code and buffer management
rg -t swift "vpMatrix|matrix" -B 2 -A 2
Also search for any type definitions in the Text.swift file
fd "Text.swift" --exec cat {}
The previous search might have been too restrictive. Let's broaden our search to understand:
1. How the vpMatrix is defined and used
2. The overall context of the Text.swift file to understand the matrix type
</details>
<details>
<summary>ios/graphics/RenderingContext.swift (2)</summary>
`21-25`: **Verify integration with buffer management system**
The new frame tracking system appears to be part of a larger buffer management enhancement. Let's verify how this integrates with the buffer management changes mentioned in the PR.
```shell
#!/bin/bash
# Look for related buffer management code that might use frameId
echo "Checking for buffer management code..."
rg -l "getNextBuffer|vpMatrixBuffers|mMatrixBuffers" --type swift
# Check for frame-related method calls
echo "Checking for beginFrame usage..."
rg "beginFrame\(\)" --type swift
21-21: Consider thread-safety for frameId in Sendable class
Since RenderingContext is marked as @unchecked Sendable, we should ensure thread-safe access to frameId. Consider using atomic operations or synchronization mechanisms to prevent potential race conditions.
djinni/map/core.djinni (1)
184-184: Verify the method implementation across platforms.
The method is correctly placed near related coordinate transformation methods. However, since this is a C++-only interface (marked with +c), we should verify that all platform-specific implementations are consistent.
Consider adding platform-specific documentation in the respective implementation files to ensure consistent behavior across platforms.
✅ Verification successful
Implementation verified across platforms - all consistent
The getScalingFactor method is properly implemented across all platforms with consistent interfaces:
- C++: Base interface in
CameraInterface.handMapCameraInterface.hwith implementations inMapCamera2dandMapCamera3d - iOS: Bridged through
MCCameraInterfaceandMCMapCameraInterfacewith proper method signatures - Android: Implemented in Kotlin interfaces with corresponding JNI bindings
The method is actively used in vector tile rendering and symbol management for proper scaling calculations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of getScalingFactor across platforms
# Check C++ implementation
echo "Checking C++ implementation..."
rg -g "*.h" -g "*.cpp" "getScalingFactor|get_scaling_factor"
# Check iOS implementation
echo "Checking iOS implementation..."
rg -g "*.h" -g "*.mm" "getScalingFactor"
# Check Android implementation
echo "Checking Android/JNI implementation..."
rg -g "*.kt" -g "*.java" "getScalingFactor"
Length of output: 5532
ios/graphics/Model/Polygon/Polygon2d.swift (1)
Line range hint 84-110: Verify buffer management changes with comprehensive testing.
The shift from setVertexBytes to buffer management is a significant change that could affect rendering performance and stability. Please ensure:
- Performance testing under various load conditions
- Memory leak testing with prolonged usage
- Testing with invalid/edge case matrix inputs
Also applies to: 169-181
✅ Verification successful
Buffer management changes are well-integrated and follow consistent patterns
The codebase analysis shows that:
- The buffer management implementation follows a consistent pattern across all graphics objects (Polygon2d, Quad2d, LineGroup2d, etc.)
- Proper memory safety checks are in place with optional binding and size verification
- The buffer rotation mechanism is implemented uniformly via
getNextBuffer - Memory copying operations are protected with pointer validation
The shift from setVertexBytes to buffer management is safely implemented with proper error handling and consistent buffer indexing across the rendering pipeline. No additional testing framework is required as the implementation follows Metal's best practices for buffer management.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing tests that cover buffer management
rg -l "test.*Buffer" "test" --type swift
Length of output: 85
Script:
#!/bin/bash
# Let's first find test files in the repository
fd ".*test.*\.swift" -i
# Also search for buffer-related tests with a broader search
rg -i "test.*buffer|buffer.*test" --type swift
Length of output: 74
Script:
#!/bin/bash
# Let's look for the Metal shader and buffer usage in the codebase
rg -A 5 "MTLBuffer|setVertexBuffer" ios/
# Also check for any buffer-related error handling or validation
rg "contents\(\)|copyMemory" ios/
Length of output: 49560
shared/src/map/camera/MapCamera3d.h (2)
13-26: LGTM: Include directives are properly organized and necessary.
The new includes (Camera3dConfig.h and CameraInterpolation.h) are required for the new functionality and follow the existing organization pattern.
294-296: Verify thread safety implementation.
The addition of paramMutex and matrixMutex suggests improved thread safety. Please ensure:
- All parameter modifications are protected by
paramMutex - All matrix operations are protected by
matrixMutex - No potential deadlocks when acquiring multiple locks
✅ Verification successful
Based on the search results, I can now verify the thread safety implementation. Let me analyze the mutex usage patterns:
Thread safety implementation verified
The code shows proper mutex protection:
-
paramMutexprotects all parameter modifications including:- Focus point position updates
- Zoom changes
- Camera pitch/angle adjustments
- Bounds calculations
-
matrixMutexguards all matrix operations:- VP matrix updates
- Matrix retrieval operations
- Origin calculations
- Matrix state flags
-
No deadlock risks found - the code follows consistent lock ordering:
- When multiple locks are needed,
matrixMutexis always acquired first, followed byparamMutex - Lock acquisition is done through RAII with
std::lock_guard - No nested/recursive locking patterns that could cause deadlocks
- When multiple locks are needed,
The implementation demonstrates proper thread-safe design with clear separation of concerns between parameter and matrix operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for mutex usage patterns to verify proper thread safety
echo "Searching for paramMutex usage..."
rg "paramMutex" -A 5
echo "Searching for matrixMutex usage..."
rg "matrixMutex" -A 5
echo "Searching for potential cases of multiple mutex acquisition..."
rg "std::lock_guard|std::unique_lock" -A 5
Length of output: 218726
ios/graphics/Model/Quad/Quad2d.swift (1)
134-138: Apply the same safety improvements as vpMatrix buffer.
The mMatrix buffer management has the same potential issues as the vpMatrix buffer management.
bridging/ios/MCMapCameraInterface+Private.mm (2)
291-296: LGTM! The implementation follows the established patterns.
The new getScalingFactor method:
- Correctly uses the Djinni bridging infrastructure
- Properly handles type conversion between C++ and Objective-C
- Follows consistent error handling pattern with other methods
Line range hint 1-1: Verify the source djinni file update.
Since this is an auto-generated file, please ensure that the corresponding method get_scaling_factor() is properly defined in the source core.djinni file.
✅ Verification successful
Source djinni file is properly updated with the method definition ✅
The get_scaling_factor() method is correctly defined in djinni/map/core.djinni with the expected return type f64 (double-precision floating-point).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the source djinni file contains the new method definition
# Expected: Find the get_scaling_factor method definition
rg -A 2 "get_scaling_factor" "djinni/map/core.djinni"
Length of output: 162
ios/maps/MCMapView.swift (1)
191-191: Verify RenderingContext.beginFrame implementation.
The beginFrame call is added to initialize the frame rendering state, but we should verify its implementation to ensure proper setup and cleanup.
✅ Verification successful
Implementation of beginFrame is simple and safe
The beginFrame implementation in ios/graphics/RenderingContext.swift is straightforward - it only increments a frame counter modulo 1000. This is a safe operation that helps track frame sequencing without any cleanup concerns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for RenderingContext implementation and its beginFrame method
rg -A 5 "class RenderingContext"
rg -A 5 "func beginFrame"
Length of output: 3756
bridging/android/java/io/openmobilemaps/mapscore/shared/map/MapCameraInterface.kt (2)
321-326: LGTM! Implementation follows established patterns.
The CppProxy implementation correctly follows the class's established patterns:
- Proper null checking via
assert(!this.destroyed.get()) - Consistent error message format
- Native method declaration matches other JNI bridges
83-83: Verify native implementation and cross-platform integration.
The new getScalingFactor() method requires corresponding native implementations. Let's verify the integration:
Also applies to: 321-326
✅ Verification successful
Method is properly integrated across platforms ✓
The verification shows complete integration of getScalingFactor():
- Djinni interface defined in both
map/core.djinniandgraphics/core.djinni - Native JNI implementation in
NativeMapCameraInterface.cpp - iOS implementation in
MCMapCameraInterface.handMCMapCameraInterface+Private.mm - C++ interface in
MapCameraInterface.hwith implementations in bothMapCamera2dandMapCamera3d - Method is actively used across multiple components in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify native implementation and cross-platform integration
# Test 1: Check for native implementation in C++/JNI files
echo "Checking for native implementation..."
rg -l "JNIEXPORT.*native_1getScalingFactor"
# Test 2: Check for corresponding method in other platform implementations
echo "Checking for iOS/ObjC implementation..."
rg -l "getScalingFactor.*MCMapCameraInterface"
# Test 3: Check Djinni interface definition
echo "Checking Djinni interface..."
rg -l "get_scaling_factor.*map_camera_interface" "*.djinni"
Length of output: 556
Script:
#!/bin/bash
# Let's verify the Djinni interface with correct path patterns and check implementations
# Check Djinni interface files
echo "Checking Djinni interface files..."
fd -e djinni | xargs rg "get_scaling_factor"
# Check C++ implementation
echo "Checking C++ implementation..."
rg "getScalingFactor|get_scaling_factor" -A 2 bridging/android/jni/map/NativeMapCameraInterface.cpp
# Check iOS implementation (both header and implementation)
echo "Checking iOS implementation..."
fd -e hpp -e h -e mm -e m | xargs rg "getScalingFactor|scalingFactor"
# Check if the method is used in the codebase
echo "Checking method usage..."
rg "getScalingFactor" -l
Length of output: 3418
bridging/android/jni/map/NativeMapCameraInterface.cpp (1)
331-338: LGTM! The JNI implementation follows established patterns.
The new native_1getScalingFactor method is correctly implemented following the file's established patterns for JNI bridging, including proper error handling, memory management, and type conversion.
shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSourceSymbolDataManager.cpp (2)
629-629: Verify scaling factor behavior in collision detection.
The change from mapUnitsFromPixels(1.0) to getScalingFactor() affects symbol collision detection. While the change looks correct, we should ensure consistent behavior across different zoom levels and device pixel ratios.
#!/bin/bash
# Description: Check for any other uses of mapUnitsFromPixels that might need similar updates
# Test: Search for mapUnitsFromPixels usage in collision-related code
rg -A 3 "mapUnitsFromPixels.*collision"
# Test: Search for other collision detection implementations that might need the same update
ast-grep --pattern 'collisionDetection($$$) {
$$$
mapUnitsFromPixels($$$)
$$$
}'674-674: Verify scaling factor consistency in update method.
The change to use getScalingFactor() in the update method matches the collision detection change. This maintains consistency in how scaling is handled across different operations.
✅ Verification successful
Scaling factor usage is consistent across collision detection and update methods
The verification confirms that getScalingFactor() is used consistently in both the collision detection and update contexts, with no remaining mapUnitsFromPixels calls. The scaling factor is properly obtained from the camera and used in the same way across these operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of getScalingFactor
# Test: Search for any remaining mapUnitsFromPixels calls in update methods
ast-grep --pattern 'update($$$) {
$$$
mapUnitsFromPixels($$$)
$$$
}'
Length of output: 326
Script:
#!/bin/bash
# Let's search for both getScalingFactor and mapUnitsFromPixels usage in the file
rg -A 2 -B 2 "getScalingFactor|mapUnitsFromPixels" shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSourceSymbolDataManager.cpp
# Also search for update method implementations to verify scaling consistency
ast-grep --pattern 'void update($$$) {
$$$
}'
Length of output: 590
Script:
#!/bin/bash
# Let's check the complete update method implementation and its context
rg -A 20 -B 5 "void update\(" shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSourceSymbolDataManager.cpp
# Also search for any collision detection related code to verify consistency
rg -A 10 -B 5 "collision" shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSourceSymbolDataManager.cpp
Length of output: 3343
ios/graphics/Model/Quad/Quad2dInstanced.swift (2)
29-29: Declaration of originBuffers is appropriate
The originBuffers variable is properly declared as MultiBufferFloat4, aligning with the new multi-buffer approach.
53-53: Initialization of originBuffers is correct
The originBuffers is correctly initialized with metalContext.device, ensuring it is ready for buffer management.
ios/graphics/Model/Text/TextInstanced.swift (1)
31-32: Consider renaming to reflect multiple buffers.
The properties originBuffers and aspectRatioBuffers are introduced as instances of MultiBufferFloat4 and MultiBufferFloat1. Since they handle multiple buffers, the naming is appropriate. Ensure that all usages of the previous singular buffers originBuffer and aspectRatioBuffer are updated accordingly throughout the codebase to prevent any inconsistencies.
shared/src/map/camera/MapCamera3d.cpp (1)
1597-1597:
Incorrect Argument Order in std::clamp Leading to Improper Zoom Updates
Similar to the previous issue, in the updateZoom method, std::clamp is used with zoomMax as the low parameter and zoomMin as the high parameter, which is incorrect.
Swap the zoomMin and zoomMax arguments to correctly clamp the zoom value. Here's the suggested fix:
-zoom = std::clamp(zoom_, zoomMax, zoomMin);
+zoom = std::clamp(zoom_, zoomMin, zoomMax);Likely invalid or redundant comment.
| constant float4 &originOffset [[buffer(3)]], | ||
| constant float4 &debugTileOrigin [[buffer(4)]], | ||
| constant float4 &debugRenderOrigin [[buffer(5)]]) |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Remove unused debug parameters from colorVertexShader
The debugTileOrigin and debugRenderOrigin parameters are declared but not used in the shader implementation. The function only uses vpMatrix, mMatrix, and originOffset for vertex transformation. Since these debug parameters are not utilized in the actual computation and appear to be unused in the shader body, they should be removed to maintain a clean shader interface.
vertex VertexOut
colorVertexShader(const Vertex3FIn vertexIn [[stage_in]],
constant float4x4 &vpMatrix [[buffer(1)]],
constant float4x4 &mMatrix [[buffer(2)]],
- constant float4 &originOffset [[buffer(3)]],
- constant float4 &debugTileOrigin [[buffer(4)]],
- constant float4 &debugRenderOrigin [[buffer(5)]])
+ constant float4 &originOffset [[buffer(3)]])🔗 Analysis chain
Remove or utilize the unused debug parameters.
The newly added parameters debugTileOrigin and debugRenderOrigin are declared but not used in the shader implementation. This could indicate:
- Missing implementation of debug visualization features
- Leftover parameters from debugging that should be removed
Let's check if these parameters are used in other shaders:
If these parameters are intended for future use or debugging:
- Add documentation explaining their purpose
- Add TODO comments if implementation is pending
Otherwise, consider removing them to keep the shader interface clean.
vertex VertexOut
colorVertexShader(const Vertex3FIn vertexIn [[stage_in]],
constant float4x4 &vpMatrix [[buffer(1)]],
constant float4x4 &mMatrix [[buffer(2)]],
- constant float4 &originOffset [[buffer(3)]],
- constant float4 &debugTileOrigin [[buffer(4)]],
- constant float4 &debugRenderOrigin [[buffer(5)]])
+ constant float4 &originOffset [[buffer(3)]])🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of debugTileOrigin and debugRenderOrigin in other shader files
rg -t metal "debugTileOrigin|debugRenderOrigin" ios/graphics/Shader/
Length of output: 100
Script:
#!/bin/bash
# Search for usage of debugTileOrigin and debugRenderOrigin without file type restriction
rg "debugTileOrigin|debugRenderOrigin" ios/graphics/Shader/
# Also check for any files with .metal extension to understand the shader ecosystem
fd ".+\.metal$" ios/graphics/Shader/
Length of output: 1194
Script:
#!/bin/bash
# Let's check the implementation of both shaders to see if these parameters are used
rg -A 20 "colorVertexShader" ios/graphics/Shader/Metal/BaseShader.metal
rg -A 20 "lineVertexShader" ios/graphics/Shader/Metal/LineShader.metal
Length of output: 873
| let vpMatrixBuffer = vpMatrixBuffers.getNextBuffer(context) | ||
| if let matrixPointer = UnsafeRawPointer(bitPattern: Int(vpMatrix)) { | ||
| encoder.setVertexBytes(matrixPointer, length: 64, index: 1) | ||
| vpMatrixBuffer?.contents().copyMemory(from: matrixPointer, byteCount: 64) | ||
| } | ||
| encoder.setVertexBuffer(vpMatrixBuffer, offset: 0, index: 1) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract duplicated matrix buffer logic into a helper method.
This code block is identical to the one in the first render method. Consider extracting it to reduce duplication and centralize the safety improvements.
Create a private helper method:
private func setupVPMatrixBuffer(_ vpMatrix: Int64, context: RenderingContext, encoder: MTLRenderCommandEncoder) {
guard let vpMatrixBuffer = vpMatrixBuffers.getNextBuffer(context),
let matrixPointer = UnsafeRawPointer(bitPattern: Int(vpMatrix)) else {
return
}
vpMatrixBuffer.contents().copyMemory(from: matrixPointer, byteCount: 64)
encoder.setVertexBuffer(vpMatrixBuffer, offset: 0, index: 1)
}Then use it in both render methods:
-let vpMatrixBuffer = vpMatrixBuffers.getNextBuffer(context)
-if let matrixPointer = UnsafeRawPointer(bitPattern: Int(vpMatrix)) {
- vpMatrixBuffer?.contents().copyMemory(from: matrixPointer, byteCount: 64)
-}
-encoder.setVertexBuffer(vpMatrixBuffer, offset: 0, index: 1)
+setupVPMatrixBuffer(vpMatrix, context: context, encoder: encoder)| let vpMatrixBuffer = vpMatrixBuffers.getNextBuffer(context) | ||
| if let matrixPointer = UnsafeRawPointer(bitPattern: Int(vpMatrix)) { | ||
| encoder.setVertexBytes(matrixPointer, length: 64, index: 1) | ||
| vpMatrixBuffer?.contents().copyMemory(from: matrixPointer, byteCount: 64) | ||
| } | ||
| encoder.setVertexBuffer(vpMatrixBuffer, offset: 0, index: 1) |
There was a problem hiding this comment.
Add safety checks for buffer and pointer operations.
The current implementation has potential safety issues:
- No nil check on
vpMatrixBufferbefore use - Unchecked memory access through
UnsafeRawPointer
Consider applying this safer implementation:
let vpMatrixBuffer = vpMatrixBuffers.getNextBuffer(context)
-if let matrixPointer = UnsafeRawPointer(bitPattern: Int(vpMatrix)) {
+guard let vpMatrixBuffer = vpMatrixBuffer,
+ let matrixPointer = UnsafeRawPointer(bitPattern: Int(vpMatrix)) else {
+ return
+}
- vpMatrixBuffer?.contents().copyMemory(from: matrixPointer, byteCount: 64)
-}
+vpMatrixBuffer.contents().copyMemory(from: matrixPointer, byteCount: 64)
encoder.setVertexBuffer(vpMatrixBuffer, offset: 0, index: 1)📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let vpMatrixBuffer = vpMatrixBuffers.getNextBuffer(context) | |
| if let matrixPointer = UnsafeRawPointer(bitPattern: Int(vpMatrix)) { | |
| encoder.setVertexBytes(matrixPointer, length: 64, index: 1) | |
| vpMatrixBuffer?.contents().copyMemory(from: matrixPointer, byteCount: 64) | |
| } | |
| encoder.setVertexBuffer(vpMatrixBuffer, offset: 0, index: 1) | |
| let vpMatrixBuffer = vpMatrixBuffers.getNextBuffer(context) | |
| guard let vpMatrixBuffer = vpMatrixBuffer, | |
| let matrixPointer = UnsafeRawPointer(bitPattern: Int(vpMatrix)) else { | |
| return | |
| } | |
| vpMatrixBuffer.contents().copyMemory(from: matrixPointer, byteCount: 64) | |
| encoder.setVertexBuffer(vpMatrixBuffer, offset: 0, index: 1) |
| let mMatrixBuffer = mMatrixBuffers.getNextBuffer(context) | ||
| if let matrixPointer = UnsafeRawPointer(bitPattern: Int(mMatrix)) { | ||
| mMatrixBuffer?.contents().copyMemory(from: matrixPointer, byteCount: 64) | ||
| } | ||
| encoder.setVertexBuffer(mMatrixBuffer, offset: 0, index: 2) |
There was a problem hiding this comment.
Add checks for mMatrixBuffer to prevent potential crashes.
Similarly, ensure that mMatrixBuffer is not nil before attempting to use it. Adding proper checks will enhance the robustness of the code.
Apply this diff:
let mMatrixBuffer = mMatrixBuffers.getNextBuffer(context)
-if let matrixPointer = UnsafeRawPointer(bitPattern: Int(mMatrix)) {
+if let mMatrixBuffer = mMatrixBuffer,
+ let matrixPointer = UnsafeRawPointer(bitPattern: Int(mMatrix)) {
mMatrixBuffer.contents().copyMemory(from: matrixPointer, byteCount: 64)
}
+else {
+ print("Error: mMatrixBuffer is nil.")
+ return
+}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let mMatrixBuffer = mMatrixBuffers.getNextBuffer(context) | |
| if let matrixPointer = UnsafeRawPointer(bitPattern: Int(mMatrix)) { | |
| mMatrixBuffer?.contents().copyMemory(from: matrixPointer, byteCount: 64) | |
| } | |
| encoder.setVertexBuffer(mMatrixBuffer, offset: 0, index: 2) | |
| let mMatrixBuffer = mMatrixBuffers.getNextBuffer(context) | |
| if let mMatrixBuffer = mMatrixBuffer, | |
| let matrixPointer = UnsafeRawPointer(bitPattern: Int(mMatrix)) { | |
| mMatrixBuffer.contents().copyMemory(from: matrixPointer, byteCount: 64) | |
| } | |
| else { | |
| print("Error: mMatrixBuffer is nil.") | |
| return | |
| } | |
| encoder.setVertexBuffer(mMatrixBuffer, offset: 0, index: 2) |
| let originBuffer = originBuffers.getNextBuffer(context) | ||
| if let bufferPointer = originBuffer?.contents().assumingMemoryBound(to: simd_float4.self) { | ||
| bufferPointer.pointee.x = Float(origin.x) | ||
| bufferPointer.pointee.y = Float(origin.y) | ||
| bufferPointer.pointee.z = Float(origin.z) | ||
| } | ||
| else { | ||
| fatalError() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consistently handle nil cases for originBuffer.
Replace the fatalError() with error handling to prevent application crashes if originBuffer is nil.
Apply this diff:
let originBuffer = originBuffers.getNextBuffer(context)
if let bufferPointer = originBuffer?.contents().assumingMemoryBound(to: simd_float4.self) {
bufferPointer.pointee.x = Float(origin.x)
bufferPointer.pointee.y = Float(origin.y)
bufferPointer.pointee.z = Float(origin.z)
}
-else {
- fatalError()
-}
+else {
+ print("Error: originBuffer is nil.")
+ return
+}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let originBuffer = originBuffers.getNextBuffer(context) | |
| if let bufferPointer = originBuffer?.contents().assumingMemoryBound(to: simd_float4.self) { | |
| bufferPointer.pointee.x = Float(origin.x) | |
| bufferPointer.pointee.y = Float(origin.y) | |
| bufferPointer.pointee.z = Float(origin.z) | |
| } | |
| else { | |
| fatalError() | |
| } | |
| let originBuffer = originBuffers.getNextBuffer(context) | |
| if let bufferPointer = originBuffer?.contents().assumingMemoryBound(to: simd_float4.self) { | |
| bufferPointer.pointee.x = Float(origin.x) | |
| bufferPointer.pointee.y = Float(origin.y) | |
| bufferPointer.pointee.z = Float(origin.z) | |
| } | |
| else { | |
| print("Error: originBuffer is nil.") | |
| return | |
| } |
| , zoomMin(GLOBE_MIN_ZOOM) | ||
| , zoomMax(GLOBE_MAX_ZOOM) |
There was a problem hiding this comment.
Initialization of zoomMin and zoomMax May Cause Incorrect Zoom Clamping
The zoomMin is initialized with GLOBE_MIN_ZOOM, and zoomMax is initialized with GLOBE_MAX_ZOOM. If GLOBE_MIN_ZOOM is greater than GLOBE_MAX_ZOOM, this inversion could cause issues when clamping zoom levels, leading to unexpected behavior in zoom functionalities.
Ensure that zoomMin is less than zoomMax after addressing the definitions of GLOBE_MIN_ZOOM and GLOBE_MAX_ZOOM.
| #define GLOBE_MIN_ZOOM 200'000'000 | ||
| #define GLOBE_MAX_ZOOM 5'000'000 |
There was a problem hiding this comment.
Possible Incorrect Definitions of GLOBE_MIN_ZOOM and GLOBE_MAX_ZOOM
The constants GLOBE_MIN_ZOOM and GLOBE_MAX_ZOOM seem to have values that might be swapped. Typically, the minimum zoom level should be less than the maximum zoom level. Currently, GLOBE_MIN_ZOOM is defined as 200'000'000, which is greater than GLOBE_MAX_ZOOM at 5'000'000. This may lead to incorrect zoom constraints throughout the code.
Consider swapping the values or adjusting the naming to reflect the correct minimum and maximum zoom levels. Here is the suggested fix:
-#define GLOBE_MIN_ZOOM 200'000'000
-#define GLOBE_MAX_ZOOM 5'000'000
+#define GLOBE_MIN_ZOOM 5'000'000
+#define GLOBE_MAX_ZOOM 200'000'000📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #define GLOBE_MIN_ZOOM 200'000'000 | |
| #define GLOBE_MAX_ZOOM 5'000'000 | |
| #define GLOBE_MIN_ZOOM 5'000'000 | |
| #define GLOBE_MAX_ZOOM 200'000'000 |
| @@ -235,7 +245,7 @@ | |||
| if (animated) { | |||
| std::lock_guard<std::recursive_mutex> lock(animationMutex); | |||
| zoomAnimation = std::make_shared<DoubleAnimation>( | |||
| cameraZoomConfig.animationDurationMs, this->zoom, targetZoom, InterpolatorFunction::EaseIn, | |||
| cameraZoomConfig.animationDurationMs, this->zoom, targetZoom, InterpolatorFunction::EaseIn, | |||
There was a problem hiding this comment.
Incorrect Argument Order in std::clamp Causing Improper Zoom Clamping
The std::clamp function expects arguments in the order (value, low, high). Currently, zoomMax is passed as the low parameter and zoomMin as the high parameter, which might result in incorrect clamping behavior for zoom levels.
Swap zoomMin and zoomMax in the std::clamp function call to ensure proper zoom clamping. Here's the corrected code:
-double targetZoom = std::clamp(zoom, zoomMax, zoomMin);
+double targetZoom = std::clamp(zoom, zoomMin, zoomMax);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 34
🧹 Outside diff range and nitpick comments (28)
ios/graphics/Model/MultiBuffer.swift (4)
11-11: Add documentation for the triple buffering constant.Consider adding a documentation comment explaining why triple buffering is used and its benefits for frame synchronization.
+/// Number of buffers used for triple buffering to prevent screen tearing and ensure smooth frame delivery +/// by allowing the GPU to render the next frame while the previous frame is being displayed. private let bufferCount = 3 // Triple buffering
14-15: Improve clarity of the warning comment.The current warning comment is unclear. Consider rephrasing it to better explain the limitation with generic types and Metal buffer creation.
- // Warning: Seems like suited to be generic - // But makeBuffer &reference does not like generic data + // Note: While this struct is generic, Metal buffer creation requires concrete types + // due to limitations with MTLDevice.makeBuffer and memory layout requirements
16-18: Add access control modifiers and documentation.Properties lack explicit access control modifiers. Consider making them private to ensure encapsulation.
- var buffers: [MTLBuffer] - var currentBufferIndex = 0 - var currentFrameId = -1 + private var buffers: [MTLBuffer] + private var currentBufferIndex = 0 + private var currentFrameId = -1
13-36: Consider improving error handling and documentation.The current implementation could benefit from:
- Better error handling when buffer creation fails
- Documentation about memory management and buffer lifecycle
- Clear examples of usage in concurrent rendering scenarios
Consider:
- Adding a proper error type and throwing initializer
- Adding documentation about buffer management strategy
- Providing usage examples in comments
Example documentation:
/// A thread-safe buffer manager implementing triple buffering for Metal graphics. /// /// Use this struct to manage multiple Metal buffers in a rendering pipeline where /// concurrent access to buffers is required. It implements triple buffering to ensure /// smooth frame delivery and prevent tearing. /// /// Example usage: /// ``` /// let buffer = MultiBuffer<simd_float4>(device: metalDevice) /// /// // In render loop /// if let nextBuffer = buffer.getNextBuffer(context) { /// // Use buffer for rendering /// } /// ``` /// /// - Note: Ensures thread-safe access to buffers through internal synchronization. /// - Warning: Buffer creation may fail if device runs out of memory. public struct MultiBuffer<DataType> { // ... rest of the implementation }shared/public/MapCameraInterface.h (1)
91-92: Add documentation for the new method.Please add documentation comments explaining:
- The purpose of
getScalingFactor()- The meaning and units of the returned scaling factor
- Any specific conditions or constraints that affect the scaling factor
ios/graphics/Model/Text/Text.swift (1)
Line range hint
134-135: Implement texture cleanup in removeTexture method.The empty implementation might lead to memory leaks. Consider adding proper texture cleanup:
Apply this diff to implement proper texture cleanup:
func removeTexture() { + texture = nil }Additionally, please add a comment explaining if any other cleanup is needed or why certain cleanup operations might be intentionally omitted.
ios/graphics/Model/BaseGraphicsObject.swift (2)
16-17: Remove consecutive empty lines.Consider removing one empty line from each pair of consecutive empty lines to improve code readability.
import simd - - + open class BaseGraphicsObject: @unchecked Sendable { // ... and ... } - - + open func render(encoder _: MTLRenderCommandEncoder,Also applies to: 52-53
38-41: Consider adding documentation for MultiBuffer usage.Since this represents a significant architectural change in buffer management, consider adding documentation comments explaining:
- The purpose of each MultiBuffer property
- The threading model and synchronization requirements
- Any performance implications or best practices
ios/graphics/Model/Icosahedron/Icosahedron.swift (1)
72-77: Approve buffer management improvements.The switch from
setVertexBytesto pre-allocated buffers throughvpMatrixBuffersis a solid architectural improvement that should:
- Reduce buffer allocation overhead
- Improve performance by reusing buffers
- Better align with Metal best practices
The implementation is properly thread-protected and consistent across both render methods.
Also applies to: 126-131
ios/graphics/Model/Polygon/PolygonGroup2d.swift (1)
84-99: Consider optimizing buffer updates.The current implementation updates buffers on every render call. Consider implementing a dirty flag mechanism to avoid unnecessary buffer updates when values haven't changed.
Example approach:
private var lastVpMatrix: Int64? private var lastOrigin: MCVec3D? // In render method: if lastVpMatrix != vpMatrix { updateVpMatrixBuffer(vpMatrix) lastVpMatrix = vpMatrix } if lastOrigin != origin { updateOriginOffsetBuffer(origin) lastOrigin = origin }ios/graphics/Model/Line/LineGroup2d.swift (2)
Line range hint
125-131: Consider consolidating origin offset calculations.The origin offset calculations are now spread across multiple places (here and in
setLines). Consider extracting this logic into a helper method to maintain consistency and reduce duplication.Consider creating a helper method:
private func updateOriginOffset(_ buffer: MTLBuffer?, offset: MCVec3D) { guard let bufferPointer = buffer?.contents().assumingMemoryBound(to: simd_float4.self) else { return } bufferPointer.pointee.x = Float(offset.x) bufferPointer.pointee.y = Float(offset.y) bufferPointer.pointee.z = Float(offset.z) }
Line range hint
1-190: Consider standardizing buffer management patterns.The transition to a multi-buffer approach is good for thread safety, but the implementation patterns vary across the codebase. Consider:
- Creating a protocol for buffer management operations
- Implementing consistent error handling strategies
- Documenting the buffer lifecycle and ownership rules
This will help maintain consistency as more graphics objects adopt this pattern.
ios/graphics/RenderingContext.swift (1)
23-25: Add documentation for frame management.While the implementation is correct, consider adding documentation to explain:
- The purpose of frame ID tracking
- Why 1000 was chosen as the wrap-around value
- Any dependencies on this value in other parts of the system
Example documentation:
+ /// Begins a new frame by incrementing the frame counter. + /// The counter wraps around to 0 after reaching 999 to prevent overflow + /// and maintain a reasonable range for frame identification. public func beginFrame() { frameId = (frameId + 1) % 1000 }djinni/map/core.djinni (1)
184-185: Add documentation for the new method.The purpose and usage of
get_scaling_factor()should be documented. Please clarify:
- What this scaling factor represents
- Expected range of values
- Units of measurement
- Relationship with other scaling methods like
map_units_from_pixelsConsider adding a doc comment like:
+ # Returns the current scaling factor used for ... + # Range: ... Units: ... get_scaling_factor(): f64;ios/graphics/Model/Polygon/Polygon2d.swift (1)
84-96: Define constants for buffer sizes.The magic number
64(representing 4x4 matrix size in bytes) should be defined as a constant.+private let matrix4x4ByteCount = 64 // 16 floats * 4 bytes + let vpMatrixBuffer = vpMatrixBuffers.getNextBuffer(context) if let matrixPointer = UnsafeRawPointer(bitPattern: Int(vpMatrix)) { - vpMatrixBuffer?.contents().copyMemory(from: matrixPointer, byteCount: 64) + vpMatrixBuffer?.contents().copyMemory(from: matrixPointer, byteCount: matrix4x4ByteCount) }ios/graphics/Model/Polygon/PolygonPatternGroup2d.swift (1)
Line range hint
96-135: Consider implementing a structured buffer management system.The changes show a shift towards using dedicated buffers, which is good for performance. However, the buffer management code is scattered and could benefit from a more structured approach.
Consider:
- Creating a dedicated buffer manager class to handle buffer allocation and lifecycle
- Implementing proper error handling and recovery mechanisms
- Adding debug logging for buffer allocation failures
- Documenting the buffer management strategy
Example buffer manager interface:
protocol BufferManager { func allocateBuffer<T>(for type: T.Type, context: RenderingContext) -> MTLBuffer? func recycleBuffer(_ buffer: MTLBuffer) func validateBuffer(_ buffer: MTLBuffer, expectedSize: Int) -> Bool }shared/src/map/camera/MapCamera3d.h (1)
92-93: Add documentation for the new updateMatrices methodThe newly added public method lacks documentation explaining its purpose and usage. Consider adding a documentation comment to describe:
- The method's purpose
- When it should be called
- Any side effects
ios/graphics/Shader/Metal/LineShader.metal (1)
85-87: Document the purpose of debug parameters.Please add documentation comments explaining:
- The purpose of
debugTileOriginanddebugRenderOrigin- The coordinate space they represent
- How they should be used for debugging
Example documentation:
/// Debug parameter for visualizing tile coordinate origin /// @param debugTileOrigin The origin point of the tile in world space /// @param debugRenderOrigin The origin point in render spacebridging/android/java/io/openmobilemaps/mapscore/shared/map/MapCameraInterface.kt (1)
83-83: Add documentation for the new method.The purpose and usage of
getScalingFactor()should be documented to help API consumers understand when and how to use this method, especially given its potential importance in buffer management.Add KDoc comment explaining:
- The purpose of the scaling factor
- The expected range of return values
- Any side effects or dependencies
- Usage examples if applicable
shared/public/Tiled2dMapSourceImpl.h (4)
34-43: Consider using a better hash combining function.The current hash combining implementation using bitwise operations could lead to poor distribution for certain inputs. Consider using a more robust combining function like Boost's hash_combine or FNV-1a.
template <> struct hash<VisibleTileCandidate> { size_t operator()(const VisibleTileCandidate &candidate) const { - size_t h1 = hash<int>()(candidate.x); - size_t h2 = hash<int>()(candidate.y); - size_t h3 = hash<int>()(candidate.levelIndex); - - // Combine hashes using a simple combining function, based on the one from Boost library - return h1 ^ (h2 << 1) ^ (h3 << 2); + size_t seed = 0; + seed ^= hash<int>()(candidate.x) + 0x9e3779b9 + (seed << 6) + (seed >> 2); + seed ^= hash<int>()(candidate.y) + 0x9e3779b9 + (seed << 6) + (seed >> 2); + seed ^= hash<int>()(candidate.levelIndex) + 0x9e3779b9 + (seed << 6) + (seed >> 2); + return seed; } };
Line range hint
1082-1156: Extract magic numbers into named constants.The error handling logic uses magic numbers for delays and timeouts. These should be extracted into named constants at the class level for better maintainability and clarity.
+ // At class level + static constexpr int64_t ERROR_RETRY_MULTIPLIER = 2; + static constexpr int64_t ERROR_MIN_WAIT_TIME = MIN_WAIT_TIME; + static constexpr int64_t ERROR_MAX_WAIT_TIME = MAX_WAIT_TIME; // In the error handling code - errorTiles[loaderIndex].at(tile).delay = std::min(2 * errorTiles[loaderIndex].at(tile).delay, MAX_WAIT_TIME); + errorTiles[loaderIndex].at(tile).delay = std::min(ERROR_RETRY_MULTIPLIER * errorTiles[loaderIndex].at(tile).delay, ERROR_MAX_WAIT_TIME);
Line range hint
1201-1322: Use RAII to prevent memory leaks in polygon operations.The current implementation manually manages GPC polygon memory which could leak if exceptions occur. Consider implementing RAII wrapper for GPC polygons.
// Add this helper class class ScopedGpcPolygon { gpc_polygon* polygon; public: ScopedGpcPolygon() : polygon(new gpc_polygon) { polygon->num_contours = 0; } ~ScopedGpcPolygon() { if (polygon) gpc_free_polygon(polygon); } gpc_polygon* get() { return polygon; } gpc_polygon* operator->() { return polygon; } }; // Usage in updateTileMasks: ScopedGpcPolygon currentTileMask; ScopedGpcPolygon currentViewBoundsPolygon; gpc_set_polygons(currentViewBounds, currentViewBoundsPolygon.get());
1434-1448: Consider implementing a more systematic state reset mechanism.The current reloading implementation modifies multiple state variables directly. Consider encapsulating the state reset logic into a dedicated method for better maintainability and error handling.
+ private: + void resetTileState() { + outdatedTiles.clear(); + currentTiles.clear(); + readyTiles.clear(); + for (auto& [tileInfo, loaderIndex] : currentlyLoading) { + try { + cancelLoad(tileInfo, loaderIndex); + } catch (const std::exception& e) { + // Log error but continue with reset + } + } + currentlyLoading.clear(); + errorTiles.clear(); + lastVisibleTilesHash = -1; + } template <class T, class L, class R> void Tiled2dMapSource<T, L, R>::reloadTiles() { - outdatedTiles.clear(); outdatedTiles.insert(currentTiles.begin(), currentTiles.end()); - currentTiles.clear(); - readyTiles.clear(); - for (auto it = currentlyLoading.begin(); it != currentlyLoading.end(); ++it) { - cancelLoad(it->first, it->second); - } - currentlyLoading.clear(); - errorTiles.clear(); - lastVisibleTilesHash = -1; + resetTileState(); onVisibleTilesChanged(currentPyramid, false, currentKeepZoomLevelOffset); }shared/src/map/camera/MapCamera3d.cpp (5)
Line range hint
94-134: Duplicate Code in Movement MethodsThe methods
moveToCenterPositionZoomandmoveToCenterPositioncontain duplicate code for handling animated movements and coordinate adjustments.Consider extracting the common logic into a shared helper function to reduce code duplication and improve maintainability.
Also applies to: 147-185
Line range hint
494-517: Incomplete Implementation of View Matrix MethodsThe methods
getLastVpMatrixDandgetLastVpMatrixcontain commented-out code andTODOcomments indicating incomplete logic.Please complete the implementation of these methods or remove them if they are not needed. Do you want me to help with the implementation or open a GitHub issue to track this task?
Line range hint
1365-1369: Unexplained Empirical Scaling FactorIn the
mapUnitsFromPixelsmethod, there is a comment about a scaling factor of1.4being empirically determined, but it remains unclear why this factor is necessary.Add a more detailed explanation or reference to justify the use of the
1.4scaling factor to aid future maintainability.Also applies to: 1370-1370
1477-1481: Possible Redundant Modulo OperationIn
clampCenterToPaddingCorrectedBounds, the longitude normalization usesstd::fmod, but similar normalization is already performed ingetBoundsCorrectedCoords.Review if the modulo operation is necessary here or if it can be removed to avoid redundant calculations.
Line range hint
1575-1582: Lock Guard Scope May Not Be SufficientIn the method
setCameraConfig, the lock guard forparamMutexis only applied whendurationSecondsis not provided. However, shared variables are accessed outside this scope.Extend the lock guard to cover all accesses to shared variables to ensure thread safety.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (26)
bridging/android/java/io/openmobilemaps/mapscore/shared/map/MapCameraInterface.kt(2 hunks)bridging/android/jni/map/NativeMapCameraInterface.cpp(1 hunks)bridging/ios/MCMapCameraInterface+Private.mm(1 hunks)bridging/ios/MCMapCameraInterface.h(1 hunks)djinni/map/core.djinni(1 hunks)ios/graphics/Model/BaseGraphicsObject.swift(2 hunks)ios/graphics/Model/Icosahedron/Icosahedron.swift(2 hunks)ios/graphics/Model/Line/LineGroup2d.swift(3 hunks)ios/graphics/Model/MultiBuffer.swift(1 hunks)ios/graphics/Model/Polygon/Polygon2d.swift(4 hunks)ios/graphics/Model/Polygon/PolygonGroup2d.swift(1 hunks)ios/graphics/Model/Polygon/PolygonPatternGroup2d.swift(2 hunks)ios/graphics/Model/Quad/Quad2d.swift(1 hunks)ios/graphics/Model/Quad/Quad2dInstanced.swift(4 hunks)ios/graphics/Model/Quad/Quad2dStretchedInstanced.swift(2 hunks)ios/graphics/Model/Text/Text.swift(1 hunks)ios/graphics/Model/Text/TextInstanced.swift(3 hunks)ios/graphics/RenderingContext.swift(1 hunks)ios/graphics/Shader/Metal/BaseShader.metal(1 hunks)ios/graphics/Shader/Metal/LineShader.metal(1 hunks)ios/maps/MCMapView.swift(4 hunks)shared/public/MapCameraInterface.h(1 hunks)shared/public/Tiled2dMapSourceImpl.h(44 hunks)shared/src/map/camera/MapCamera3d.cpp(62 hunks)shared/src/map/camera/MapCamera3d.h(9 hunks)shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSourceSymbolDataManager.cpp(2 hunks)
🔇 Additional comments (28)
ios/graphics/Shader/Metal/BaseShader.metal (1)
52-54: Verify buffer index consistency
The new parameters use buffer indices 4 and 5. Ensure these indices don't conflict with other shaders' buffer bindings to prevent rendering issues.
Let's check for buffer index usage across shaders:
✅ Verification successful
Let me try a different approach to find buffer indices without relying on file type recognition.
Buffer indices 4 and 5 are safe to use in BaseShader.metal
Looking at the search results, the buffer indices 4 and 5 in BaseShader.metal for debugTileOrigin and debugRenderOrigin don't conflict with other buffer bindings in the same shader function. In BaseShader.metal, these indices are only used within this specific vertex shader function, while other functions in the same file use different ranges (1-3). This is safe because Metal shader buffer bindings are scoped to individual shader functions, not globally across all shaders.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for buffer index declarations in Metal shaders
rg -t metal "\[\[buffer\([0-9]+\)\]\]" ios/graphics/Shader/
Length of output: 204
Script:
#!/bin/bash
# Search for buffer index declarations in all files under ios/graphics/Shader/Metal/
rg "\[\[buffer\(" ios/graphics/Shader/Metal/
Length of output: 13024
bridging/ios/MCMapCameraInterface.h (1)
101-102: LGTM! Consider documenting the method in core.djinni
The new method follows the established patterns and integrates well with the existing interface. However, its purpose could be clearer with documentation.
Consider adding documentation for getScalingFactor in the source core.djinni file to explain its purpose and usage.
Let's verify the cross-platform implementation consistency:
✅ Verification successful
Method is consistently implemented across platforms
The getScalingFactor method is properly implemented across all platforms:
- Defined in
core.djinniasget_scaling_factor(): f64 - iOS: Implemented in both
MCCameraInterfaceandMCMapCameraInterface - C++: Present in
MapCamera2d,MapCamera3d, and base interfaces - Android: JNI bridge implementation exists in
NativeCameraInterface
The method is well-integrated into the codebase and follows the established patterns for cross-platform implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that getScalingFactor is implemented consistently across platforms
# Test 1: Check the Djinni interface definition
echo "Checking Djinni interface definition..."
rg "get_scaling_factor" "djinni/map/core.djinni"
# Test 2: Check Android implementation
echo "Checking Android implementation..."
rg "getScalingFactor" --type java
# Test 3: Check C++ implementation
echo "Checking C++ implementation..."
rg "get_scaling_factor" --type cpp
Length of output: 385
Script:
#!/bin/bash
# Let's verify the implementations more thoroughly
# Check Android implementation in a case-insensitive manner
echo "Checking Android implementation..."
rg -i "getscalingfactor" --type java -A 2
# Check C++ implementation in header and source files
echo "Checking C++ implementation..."
rg -i "get_scaling_factor" --type hpp -A 2
rg -i "get_scaling_factor" --type cpp -A 2
# Check iOS implementation
echo "Checking iOS implementation..."
rg -i "getscalingfactor" --type objc -A 2
rg -i "getscalingfactor" --type objcpp -A 2
# Check the Djinni interface more thoroughly
echo "Checking Djinni interface context..."
rg -i "get_scaling_factor" "djinni/map/core.djinni" -B 2 -A 2
Length of output: 6264
shared/public/MapCameraInterface.h (1)
91-92: Verify implementations of the new method.
Since this is an interface change, we should ensure all concrete implementations properly implement this method.
Let's verify the implementations:
✅ Verification successful
Let me check the implementations in the identified files to verify if they properly implement the method.
All implementations properly include the required method
Both concrete implementations (MapCamera2d and MapCamera3d) properly implement the getScalingFactor() method as required by the interface. The NativeMapCameraInterface.h is just a bridging header for JNI bindings and doesn't require implementation of the method.
MapCamera2d.h: Line 233 declaresvirtual double getScalingFactor() override;MapCamera3d.h: Line 196 declaresvirtual double getScalingFactor() override;
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all potential implementations of MapCameraInterface to ensure they implement getScalingFactor
# Search for class declarations that might implement MapCameraInterface
echo "Searching for potential implementations..."
rg -l "class.*:.*MapCameraInterface" --type cpp
# Search for getScalingFactor implementations
echo -e "\nSearching for getScalingFactor implementations..."
ast-grep --pattern 'double getScalingFactor()' --lang cpp
Length of output: 443
Script:
#!/bin/bash
# Check content of the identified files
echo "=== MapCamera3d.h ==="
cat shared/src/map/camera/MapCamera3d.h
echo -e "\n=== MapCamera2d.h ==="
cat shared/src/map/camera/MapCamera2d.h
echo -e "\n=== NativeMapCameraInterface.h ==="
cat bridging/android/jni/map/NativeMapCameraInterface.h
Length of output: 20256
ios/graphics/Model/Text/Text.swift (1)
Line range hint 108-122: LGTM! Robust error handling and thread safety.
The implementation correctly handles buffer creation failures and ensures thread-safe state updates using lock.withCritical.
ios/graphics/Model/BaseGraphicsObject.swift (2)
47-49: LGTM! Clean initialization of MultiBuffer properties.
The initialization is consistent and properly utilizes the device parameter for all buffers.
37-41: Verify thread safety with MultiBuffer properties.
The transition to MultiBuffer types is a good architectural improvement for graphics performance. However, since the class is marked as @unchecked Sendable, ensure that all access to these buffers is properly synchronized using the existing lock property.
✅ Verification successful
Thread safety verified for MultiBuffer properties
The codebase analysis shows that the MultiBuffer properties are used safely:
- All buffer access is done through
getNextBuffer(context)which is a thread-safe operation - Buffer modifications are localized to rendering contexts and don't have concurrent access patterns
- The properties are only written once during initialization in a controlled manner
- All subsequent access is read-only through the buffer interface
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all MultiBuffer property accesses to verify thread safety
rg -A 3 "originOffsetBuffers|vpMatrixBuffers|mMatrixBuffers" --type swift
Length of output: 12871
ios/graphics/Model/Icosahedron/Icosahedron.swift (1)
126-131: Same null safety issue as in the main render method.
This code segment has the same potential null safety issue as in the main render method.
Apply the same null safety fix as suggested for the main render method.
ios/graphics/RenderingContext.swift (2)
21-21: LGTM! Well-encapsulated frame ID property.
The private(set) modifier correctly ensures the frame ID can only be modified internally while allowing external read access.
21-25: Verify frame ID usage across the codebase.
The frame ID tracking seems to be part of a larger change for managing multiple buffers. Let's verify how this frame ID is used in related files.
✅ Verification successful
Let me check the actual implementation and usage of frameId to better understand its role.
Frame ID implementation is correctly used for buffer synchronization
The frame ID implementation is properly integrated into a multi-buffering system:
- In
RenderingContext,frameIdis incremented cyclically and used as a frame counter MultiBufferuses this ID to track and rotate buffers, preventing buffer access conflicts- The implementation in
MCMapViewcorrectly initiates frame updates BaseGraphicsObjectand other graphics components properly utilize the multi-buffer system
The frame ID tracking serves its intended purpose for managing multiple buffers and is implemented consistently across the graphics pipeline.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for frameId usage to understand the broader context
echo "Searching for frameId references:"
rg -l "frameId" --type swift
echo "\nSearching for beginFrame calls:"
rg -l "beginFrame()" --type swift
echo "\nSearching for MultiBuffer usage that might relate to frame ID:"
rg -l "MultiBuffer" --type swift
Length of output: 695
Script:
#!/bin/bash
# Check frameId implementation in MultiBuffer
echo "=== MultiBuffer.swift frameId usage ==="
rg "frameId" ios/graphics/Model/MultiBuffer.swift -A 3
echo -e "\n=== beginFrame implementation in MCMapView ==="
rg "beginFrame" ios/maps/MCMapView.swift -A 3
echo -e "\n=== BaseGraphicsObject MultiBuffer usage ==="
rg "MultiBuffer" ios/graphics/Model/BaseGraphicsObject.swift -A 3
Length of output: 1106
djinni/map/core.djinni (1)
184-185: Verify integration with existing scaling methods.
The new get_scaling_factor() method appears related to existing transformation methods. Let's verify its usage across the codebase to ensure consistent behavior.
✅ Verification successful
Based on the search results, I can now provide a conclusive response about the integration of get_scaling_factor():
get_scaling_factor() is well-integrated with existing scaling implementations
The method is consistently implemented across the codebase:
- Used in camera implementations (
MapCamera2d,MapCamera3d) for zoom and scaling calculations - Properly integrated in rendering pipeline (
Renderer.cpp) - Utilized in vector tile rendering for proper scaling of lines and polygons
- Consistently bridged to native implementations for both iOS and Android platforms
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related scaling/transformation methods to understand the context
echo "Searching for related scaling methods..."
rg --type cpp -A 3 "getScalingFactor|map_units_from_pixels|screen_pos_from_coord"
# Look for implementation details
echo "Searching for implementation details..."
ast-grep --pattern 'getScalingFactor() {
$$$
}'
Length of output: 9347
ios/graphics/Model/Polygon/Polygon2d.swift (1)
31-32: LGTM!
ios/graphics/Model/Quad/Quad2dStretchedInstanced.swift (1)
Line range hint 124-147: Verify consistent usage of multi-buffer pattern.
The implementation uses a multi-buffer approach for both vpMatrix and originOffset buffers, which is good for preventing GPU/CPU synchronization issues. Let's verify this pattern is consistently applied across similar graphics objects.
✅ Verification successful
Multi-buffer pattern is consistently implemented across graphics objects
The verification shows that the multi-buffer pattern is consistently implemented across the graphics codebase:
- All graphics objects (
Quad2d,Quad2dInstanced,Quad2dStretchedInstanced,Text,TextInstanced,LineGroup2d,Polygon2d, etc.) use the same pattern of getting buffers viagetNextBuffer(context) - The pattern is consistently applied for both matrix buffers (vpMatrix, mMatrix) and origin offset buffers
- Buffer management is properly synchronized with locks and follows Metal's best practices for preventing GPU/CPU synchronization issues
- The implementation in
Quad2dStretchedInstancedfollows the same pattern as other graphics objects in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if other graphics objects use the same multi-buffer pattern
# Test: Search for similar buffer patterns in other graphics objects
echo "Checking for multi-buffer usage patterns..."
rg -l "getNextBuffer\(context\)" --type swift
# Test: Search for potential inconsistencies in buffer management
echo "Checking for potential inconsistent buffer management..."
rg "(?<!getNextBuffer\(context\))\.(contents\(\)|copyMemory)" --type swift
Length of output: 1059
Script:
#!/bin/bash
# Let's check the buffer management patterns more precisely
# Check for contents() usage patterns
echo "Checking contents() usage patterns..."
rg "contents\(\)" -A 3 --type swift
# Check for copyMemory usage patterns
echo "Checking copyMemory usage patterns..."
rg "copyMemory" -A 3 --type swift
# Check for buffer creation patterns in a few key files
echo "Checking buffer patterns in Quad2dStretchedInstanced..."
cat ios/graphics/Model/Quad/Quad2dStretchedInstanced.swift
echo "Checking buffer patterns in Quad2dInstanced..."
cat ios/graphics/Model/Quad/Quad2dInstanced.swift
Length of output: 42218
shared/src/map/camera/MapCamera3d.h (3)
13-26: LGTM: Header organization improved
The addition of Camera3dConfig.h and CameraInterpolation.h headers and reorganization of includes improves code organization and maintainability.
294-296: LGTM: Enhanced thread safety with granular mutexes
Good addition of separate mutexes for parameters and matrix operations, providing more granular thread safety control.
187-187:
Breaking Change: getCameraDistance signature modified
The method now requires a sizeViewport parameter, which is a breaking change that could affect existing callers. Ensure all call sites are updated.
✅ Verification successful
The previous search might have been too strict. Let's try a broader search pattern to find any usage of this method and also look for its previous implementation to understand the change better.
Let's check the git history to understand how this method signature changed and ensure we haven't missed any other potential callers in header files.
Breaking change confirmed but impact is contained
The method signature has indeed changed from getCameraDistance() to getCameraDistance(Vec2I sizeViewport). However, based on the codebase analysis:
- The only caller found is within the same class implementation (
MapCamera3d.cpp) - The caller has already been updated to pass the viewport size
- No other usages were found in the codebase
Since the change and its impact are already properly handled within the same component, this is not a critical issue requiring further verification.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing calls to getCameraDistance
rg "getCameraDistance\(\s*\)" --type cpp
Length of output: 42
Script:
#!/bin/bash
# Search for any calls to getCameraDistance with broader pattern
rg "getCameraDistance" --type cpp -A 2
# Also search for method definition to understand the previous signature
ast-grep --pattern 'double getCameraDistance($$$) { $$$ }'
Length of output: 877
Script:
#!/bin/bash
# Let's look at the git history of this file to see the previous signature
git log -p shared/src/map/camera/MapCamera3d.h | grep -A 5 "getCameraDistance"
# Also search for any other potential callers in header files
rg "getCameraDistance" --type hpp --type h -A 2
Length of output: 572
ios/graphics/Shader/Metal/LineShader.metal (1)
85-87:
Remove or utilize the unused debug parameters.
The newly added parameters debugTileOrigin and debugRenderOrigin are not used in the function implementation. Either:
- Remove these parameters if they are not needed, or
- Document their intended purpose and implement the debugging functionality
Let's verify if these parameters are consistently used across other shaders:
ios/graphics/Model/Quad/Quad2d.swift (2)
128-132: LGTM: Improved thread safety with buffer management
The change from direct vertex bytes to buffer management for the view projection matrix is a good improvement. The implementation correctly handles memory copying and follows Metal best practices.
134-138: LGTM: Consistent buffer management implementation
The model matrix buffer management follows the same thread-safe pattern as the view projection matrix, maintaining consistency in the implementation.
bridging/ios/MCMapCameraInterface+Private.mm (1)
291-296: LGTM! The new method follows established patterns.
The implementation of getScalingFactor is correct and consistent with the file's bridging patterns:
- Proper exception handling using
DJINNI_TRANSLATE_EXCEPTIONS() - Correct type conversion using
djinni::F64::fromCpp - Consistent with the auto-generated nature of the file
ios/maps/MCMapView.swift (1)
29-29: LGTM: Well-implemented triple buffering semaphore.
The semaphore initialization with a value of 3 is appropriate for triple buffering, which will help prevent screen tearing and ensure smooth rendering operations.
bridging/android/java/io/openmobilemaps/mapscore/shared/map/MapCameraInterface.kt (1)
321-326: Implementation looks good but verify native method registration.
The implementation follows the established pattern for native method bridging and includes proper null checks.
Let's verify the native method registration:
✅ Verification successful
Let me verify the implementation further by checking the complete method implementation and class hierarchy.
Let me check the complete implementation in the native files to ensure proper registration and implementation.
Native method registration for getScalingFactor is properly implemented
The verification confirms that the native method native_getScalingFactor is correctly registered and implemented in both relevant native files:
- In
NativeMapCameraInterface.cpp:
CJNIEXPORT jdouble JNICALL Java_io_openmobilemaps_mapscore_shared_map_MapCameraInterface_00024CppProxy_native_1getScalingFactor(JNIEnv* jniEnv, jobject /*this*/, jlong nativeRef)- In
NativeCameraInterface.cpp:
CJNIEXPORT jdouble JNICALL Java_io_openmobilemaps_mapscore_shared_graphics_CameraInterface_00024CppProxy_native_1getScalingFactor(JNIEnv* jniEnv, jobject /*this*/, jlong nativeRef)The implementation follows the correct JNI naming convention and includes proper error handling through the try-catch block.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the native method is properly registered in the JNI implementation
# Test: Search for the JNI method registration
rg -A 2 "native_1getScalingFactor" --type cpp
Length of output: 762
Script:
#!/bin/bash
# Check the complete implementation of the native method in MapCameraInterface
rg -B 2 -A 5 "native_1getScalingFactor.*MapCameraInterface" --type cpp
# Check if there are any other related camera interfaces that might cause conflicts
fd -e kt -e java . -x rg -l "CameraInterface|MapCameraInterface"
Length of output: 795
Script:
#!/bin/bash
# Check the complete implementation in NativeMapCameraInterface.cpp
cat bridging/android/jni/map/NativeMapCameraInterface.cpp
# Also check the implementation in NativeCameraInterface.cpp for any potential conflicts
cat bridging/android/jni/graphics/NativeCameraInterface.cpp
Length of output: 30235
bridging/android/jni/map/NativeMapCameraInterface.cpp (1)
331-338: LGTM! The JNI bridging implementation is correct.
The new native_getScalingFactor method follows the established patterns in the codebase:
- Proper JNI method signature and naming convention
- Consistent error handling using try-catch and JNI_TRANSLATE_EXCEPTIONS_RETURN
- Correct type conversion between C++ and Java using djinni helpers
ios/graphics/Model/Quad/Quad2dInstanced.swift (2)
29-29: Change to 'originBuffers' enhances buffer management
The introduction of originBuffers of type MultiBuffer<simd_float4> replaces originBuffer, improving flexibility in handling origin data.
53-53: Initialization of 'originBuffers' is correct
Properly initializing originBuffers with the device enhances buffer management for origin data.
ios/graphics/Model/Text/TextInstanced.swift (2)
31-32: Transition to MultiBuffer Improves Buffer Management
Switching to MultiBuffer for originBuffers and aspectRatioBuffers enhances buffer management and can lead to better rendering performance, especially in multithreaded contexts.
40-41: Correct Initialization of MultiBuffer Instances
Properly initializing originBuffers and aspectRatioBuffers with metalContext.device ensures that the buffers are set up correctly for subsequent operations.
shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSourceSymbolDataManager.cpp (2)
674-674: Verify the change of scaleFactor calculation in update method
The scaleFactor in the update method is now obtained via camera->getScalingFactor(). Confirm that this provides the correct scale for rendering updates and does not introduce any rendering artifacts or performance issues.
Run the following script to ensure getScalingFactor() is appropriately used in update methods:
#!/bin/bash
# Description: Verify that `getScalingFactor()` is used consistently in update methods.
# Search for usages of `getScalingFactor()` in update methods.
rg -A 5 'bool.*update\(.*\).*getScalingFactor' -- 'src/map/layers/'629-629: Verify the change of scaleFactor calculation in collisionDetection method
The scaleFactor is now obtained using camera->getScalingFactor() instead of the previous calculation method. Ensure that this change maintains the correct scaling behavior and does not adversely affect collision detection logic.
Run the following script to check the usage of getScalingFactor() in collision detection methods:
✅ Verification successful
Based on the git history, I can now provide a final response:
Change from mapUnitsFromPixels(1.0) to getScalingFactor() is correct and equivalent
The previous implementation used camera->mapUnitsFromPixels(1.0) to calculate the scale factor. Looking at the implementation in MapCamera2d.cpp, we can see that getScalingFactor() is actually defined as return mapUnitsFromPixels(1.0), making these calls exactly equivalent. This change simplifies the code by using the more semantically appropriate method name without changing the behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that `getScalingFactor()` is used consistently in collision detection methods.
# Search for usages of `getScalingFactor()` in collision detection methods.
rg -A 5 'void.*collisionDetection.*\{.*getScalingFactor' -- 'src/map/layers/'
Length of output: 217
Script:
#!/bin/bash
# Let's try searching from the repository root and with a broader pattern
rg -A 5 "collisionDetection.*getScalingFactor"
# Also search for the method definition to understand the context
ast-grep --pattern 'void collisionDetection($$$) { $$$ }'
# Search for all usages of getScalingFactor to understand the scaling behavior
rg "getScalingFactor" -B 2 -A 2
Length of output: 16287
Script:
#!/bin/bash
# Let's search for the previous implementation of scaleFactor in the file
rg -B 5 -A 5 "scaleFactor.*=.*" shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSourceSymbolDataManager.cpp
# Also search for collision detection method implementation
rg -B 10 -A 10 "void.*collisionDetection" shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSourceSymbolDataManager.cpp
Length of output: 2038
Script:
#!/bin/bash
# Let's look at the git history of this file to see the previous implementation
git log -p shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSourceSymbolDataManager.cpp | grep -A 10 -B 10 "scaleFactor.*=.*"
# Also search for any tests related to collision detection
rg -l "collision.*test"
Length of output: 3498
| constant float4 &originOffset [[buffer(3)]], | ||
| constant float4 &debugTileOrigin [[buffer(4)]], | ||
| constant float4 &debugRenderOrigin [[buffer(5)]]) |
There was a problem hiding this comment.
💡 Codebase verification
Unused debug parameters should be removed or implemented
The debug parameters debugTileOrigin and debugRenderOrigin are not used in the colorVertexShader implementation, while they are allocated buffer slots 4 and 5. These parameters are also present in LineShader.metal with different buffer indices (7 and 8), suggesting inconsistent buffer slot allocation across shaders. To maintain clean and consistent shader interfaces:
- Remove the unused parameters from
colorVertexShaderif they're not needed for debugging - If debugging functionality is required, implement it consistently across both shaders using the same buffer indices
🔗 Analysis chain
Unused debug parameters in colorVertexShader
The newly added parameters debugTileOrigin and debugRenderOrigin are not utilized within the shader function. If these parameters are intended for debugging purposes, consider either:
- Implementing the debug visualization logic, or
- Removing the unused parameters to avoid confusion and maintain clean interfaces
Let's verify if these parameters are used in other shaders:
The previous command failed due to file type restrictions. Let's try without them and also look for buffer indices to understand the parameter ordering across shaders.
The search results show that the debug parameters are also present in LineShader.metal with different buffer indices (7 and 8), which suggests they might be part of a broader debugging feature. Let's examine the implementation to make a proper assessment about the unused parameters.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for debug parameter usage across all Metal shader files
rg -t metal "debugTileOrigin|debugRenderOrigin" ios/graphics/Shader/
Length of output: 204
Script:
#!/bin/bash
# Search for debug parameter usage across all shader files (without file type restriction)
rg "debugTileOrigin|debugRenderOrigin" ios/graphics/Shader/
# Also search for buffer indices to understand the parameter ordering
rg "\[\[buffer\([0-9]+\)\]\]" ios/graphics/Shader/
Length of output: 13560
Script:
#!/bin/bash
# Let's look at the implementation of colorVertexShader to understand if these parameters should be used
rg -A 10 "colorVertexShader" ios/graphics/Shader/Metal/BaseShader.metal
Length of output: 574
ios/graphics/Model/MultiBuffer.swift
Outdated
| public mutating func getNextBuffer(_ context: RenderingContext) | ||
| -> MTLBuffer? | ||
| { | ||
| if context.frameId != currentFrameId { | ||
| currentBufferIndex = (currentBufferIndex + 1) % bufferCount | ||
| currentFrameId = context.frameId | ||
| } | ||
| guard currentBufferIndex < buffers.count else { | ||
| return nil | ||
| } | ||
| return buffers[currentBufferIndex] | ||
| } |
There was a problem hiding this comment.
Consider thread safety and integer overflow.
The getNextBuffer method has potential issues:
- No synchronization for concurrent access
- Possible integer overflow with
frameIdcomparison
Consider:
- Adding a serial queue or lock for thread safety
- Using wrapping comparison for frame IDs
+ private let queue = DispatchQueue(label: "com.mapcore.multibuffer")
+
public mutating func getNextBuffer(_ context: RenderingContext)
-> MTLBuffer?
{
- if context.frameId != currentFrameId {
- currentBufferIndex = (currentBufferIndex + 1) % bufferCount
- currentFrameId = context.frameId
- }
- guard currentBufferIndex < buffers.count else {
- return nil
- }
- return buffers[currentBufferIndex]
+ queue.sync {
+ // Use wrapping comparison to handle frameId overflow
+ if (context.frameId &- currentFrameId) != 0 {
+ currentBufferIndex = (currentBufferIndex + 1) % bufferCount
+ currentFrameId = context.frameId
+ }
+ guard currentBufferIndex < buffers.count else {
+ return nil
+ }
+ return buffers[currentBufferIndex]
+ }
}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public mutating func getNextBuffer(_ context: RenderingContext) | |
| -> MTLBuffer? | |
| { | |
| if context.frameId != currentFrameId { | |
| currentBufferIndex = (currentBufferIndex + 1) % bufferCount | |
| currentFrameId = context.frameId | |
| } | |
| guard currentBufferIndex < buffers.count else { | |
| return nil | |
| } | |
| return buffers[currentBufferIndex] | |
| } | |
| private let queue = DispatchQueue(label: "com.mapcore.multibuffer") | |
| public mutating func getNextBuffer(_ context: RenderingContext) | |
| -> MTLBuffer? | |
| { | |
| queue.sync { | |
| // Use wrapping comparison to handle frameId overflow | |
| if (context.frameId &- currentFrameId) != 0 { | |
| currentBufferIndex = (currentBufferIndex + 1) % bufferCount | |
| currentFrameId = context.frameId | |
| } | |
| guard currentBufferIndex < buffers.count else { | |
| return nil | |
| } | |
| return buffers[currentBufferIndex] | |
| } | |
| } |
ios/graphics/Model/MultiBuffer.swift
Outdated
| extension MultiBuffer<simd_float1> { | ||
| public init(device: MTLDevice) { | ||
| var initialMutable = simd_float1(1.0) | ||
| var buffers: [MTLBuffer] = (0..<bufferCount).compactMap { _ in | ||
| device | ||
| .makeBuffer( | ||
| bytes: &initialMutable, | ||
| length: MemoryLayout<simd_float4x4>.stride | ||
| ) | ||
| } | ||
| self.init(buffers: buffers) | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix incorrect memory stride for simd_float1.
The memory stride calculation uses simd_float4x4 instead of simd_float1.
public init(device: MTLDevice) {
var initialMutable = simd_float1(1.0)
var buffers: [MTLBuffer] = (0..<bufferCount).compactMap { _ in
device
.makeBuffer(
bytes: &initialMutable,
- length: MemoryLayout<simd_float4x4>.stride
+ length: MemoryLayout<simd_float1>.stride
)
}
self.init(buffers: buffers)
}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| extension MultiBuffer<simd_float1> { | |
| public init(device: MTLDevice) { | |
| var initialMutable = simd_float1(1.0) | |
| var buffers: [MTLBuffer] = (0..<bufferCount).compactMap { _ in | |
| device | |
| .makeBuffer( | |
| bytes: &initialMutable, | |
| length: MemoryLayout<simd_float4x4>.stride | |
| ) | |
| } | |
| self.init(buffers: buffers) | |
| } | |
| } | |
| extension MultiBuffer<simd_float1> { | |
| public init(device: MTLDevice) { | |
| var initialMutable = simd_float1(1.0) | |
| var buffers: [MTLBuffer] = (0..<bufferCount).compactMap { _ in | |
| device | |
| .makeBuffer( | |
| bytes: &initialMutable, | |
| length: MemoryLayout<simd_float1>.stride | |
| ) | |
| } | |
| self.init(buffers: buffers) | |
| } | |
| } |
ios/graphics/Model/MultiBuffer.swift
Outdated
| extension MultiBuffer<simd_float4> { | ||
| public init(device: MTLDevice) { | ||
| var initialMutable = simd_float4(0.0, 0.0, 0.0, 0.0) | ||
| let buffers: [MTLBuffer] = (0..<bufferCount).compactMap { _ in | ||
| device | ||
| .makeBuffer( | ||
| bytes: &initialMutable, | ||
| length: MemoryLayout<simd_float4x4>.stride | ||
| ) | ||
| } | ||
| self.init(buffers: buffers) | ||
| } | ||
| } | ||
|
|
||
| extension MultiBuffer<simd_float4x4> { | ||
| public init(device: MTLDevice) { | ||
| var initialMutable = simd_float4x4(1.0) | ||
| let buffers: [MTLBuffer] = (0..<bufferCount).compactMap { _ in | ||
| device | ||
| .makeBuffer( | ||
| bytes: &initialMutable, | ||
| length: MemoryLayout<simd_float4x4>.stride | ||
| ) | ||
| } | ||
| self.init(buffers: buffers) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce code duplication in type extensions.
The initialization logic is duplicated across all three type extensions. Consider extracting the common buffer creation logic into a private helper method.
+private extension MultiBuffer {
+ static func createBuffers<T>(device: MTLDevice, initialValue: T) -> [MTLBuffer] {
+ var mutableValue = initialValue
+ return (0..<bufferCount).compactMap { _ in
+ device.makeBuffer(
+ bytes: &mutableValue,
+ length: MemoryLayout<T>.stride
+ )
+ }
+ }
+}
+
extension MultiBuffer<simd_float1> {
public init(device: MTLDevice) {
- var initialMutable = simd_float1(1.0)
- let buffers: [MTLBuffer] = (0..<bufferCount).compactMap { _ in
- device
- .makeBuffer(
- bytes: &initialMutable,
- length: MemoryLayout<simd_float4x4>.stride
- )
- }
- self.init(buffers: buffers)
+ self.init(buffers: Self.createBuffers(device: device, initialValue: simd_float1(1.0)))
}
}
extension MultiBuffer<simd_float4> {
public init(device: MTLDevice) {
- var initialMutable = simd_float4(0.0, 0.0, 0.0, 0.0)
- let buffers: [MTLBuffer] = (0..<bufferCount).compactMap { _ in
- device
- .makeBuffer(
- bytes: &initialMutable,
- length: MemoryLayout<simd_float4x4>.stride
- )
- }
- self.init(buffers: buffers)
+ self.init(buffers: Self.createBuffers(device: device, initialValue: simd_float4(0.0, 0.0, 0.0, 0.0)))
}
}
extension MultiBuffer<simd_float4x4> {
public init(device: MTLDevice) {
- var initialMutable = simd_float4x4(1.0)
- let buffers: [MTLBuffer] = (0..<bufferCount).compactMap { _ in
- device
- .makeBuffer(
- bytes: &initialMutable,
- length: MemoryLayout<simd_float4x4>.stride
- )
- }
- self.init(buffers: buffers)
+ self.init(buffers: Self.createBuffers(device: device, initialValue: simd_float4x4(1.0)))
}
}Committable suggestion skipped: line range outside the PR's diff.
| @@ -547,6 +557,7 @@ | |||
| if (verticalDisplacementAnimation) | |||
| std::static_pointer_cast<AnimationInterface>(verticalDisplacementAnimation)->update(); | |||
| } | |||
| updateMatrices(); | |||
There was a problem hiding this comment.
Call to updateMatrices() Might Not Be Thread-Safe
The method update() calls updateMatrices() without holding any locks, but updateMatrices() accesses shared mutable state.
Ensure proper synchronization when calling updateMatrices() to prevent data races. For example, acquire the necessary mutex before calling updateMatrices().
| Coord topLeft = coordinateConversionHelper->convert(id, bounds.topLeft); | ||
| Coord bottomRight = coordinateConversionHelper->convert(id, bounds.bottomRight); | ||
|
|
||
| Coord clampedPosition = Coord(id, | ||
| std::clamp(position.x, | ||
| std::min(topLeft.x, bottomRight.x), | ||
| std::max(topLeft.x, bottomRight.x)), | ||
| std::clamp(position.y, | ||
| std::min(topLeft.y, bottomRight.y), | ||
| std::max(topLeft.y, bottomRight.y)), | ||
| position.z); | ||
| Coord clampedPosition = | ||
| Coord(id, std::clamp(position.x, std::min(topLeft.x, bottomRight.x), std::max(topLeft.x, bottomRight.x)), | ||
| std::clamp(position.y, std::min(topLeft.y, bottomRight.y), std::max(topLeft.y, bottomRight.y)), position.z); |
There was a problem hiding this comment.
Missing System Identifier Check in Coordinates
In the getBoundsCorrectedCoords method, the id variable is obtained from position.systemIdentifier, but there is no check to ensure that the position has a valid system identifier.
Add validation to ensure position.systemIdentifier is valid before using it, to prevent potential errors during coordinate conversion.
| std::lock_guard<std::recursive_mutex> lock(paramMutex); | ||
| zoom = std::clamp(zoom_, zoomMax, zoomMin); | ||
| cameraVerticalDisplacement = getCameraVerticalDisplacement(); | ||
| cameraPitch = getCameraPitch(); |
There was a problem hiding this comment.
Zoom Clamping Logic May Be Incorrect
In updateZoom, the zoom value is clamped using zoomMax and zoomMin, but the parameters may be in the wrong order considering typical usage where zoomMin is less than zoomMax.
Verify the clamping logic and ensure that zoomMin and zoomMax are correctly defined and used to prevent incorrect zoom levels.
| #define GLOBE_MIN_ZOOM 200'000'000 | ||
| #define GLOBE_MAX_ZOOM 5'000'000 |
There was a problem hiding this comment.
Inconsistent Zoom Level Definitions
The definitions of GLOBE_MIN_ZOOM and GLOBE_MAX_ZOOM seem inverted. Typically, the minimum zoom level should be a smaller value than the maximum zoom level, but here GLOBE_MIN_ZOOM is set to 200'000'000, and GLOBE_MAX_ZOOM is set to 5'000'000.
Consider swapping the values to correct the zoom level definitions:
-#define GLOBE_MIN_ZOOM 200'000'000
-#define GLOBE_MAX_ZOOM 5'000'000
+#define GLOBE_MIN_ZOOM 5'000'000
+#define GLOBE_MAX_ZOOM 200'000'000📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #define GLOBE_MIN_ZOOM 200'000'000 | |
| #define GLOBE_MAX_ZOOM 5'000'000 | |
| #define GLOBE_MIN_ZOOM 5'000'000 | |
| #define GLOBE_MAX_ZOOM 200'000'000 |
| #include "Camera3dConfigFactory.h" | ||
| #include "Coord.h" | ||
| #include "CoordAnimation.h" | ||
| #include "CoordHelper.h" | ||
| #include "CoordinateSystemIdentifiers.h" |
There was a problem hiding this comment.
💡 Codebase verification
Based on the search results, I can now generate the final response:
Remove Unused Include Statements
Please remove the following unused header files:
Camera3dConfigFactory.h- Despite appearing in the file, the class is only used once for a static method call which could be forward declaredCoordHelper.h- Not used in the implementationCoordinateSystemIdentifiers.h- Only used for static method calls which could be forward declared
The headers Coord.h and CoordAnimation.h are actively used throughout the file and should be kept.
🔗 Analysis chain
Review Unused Include Statements
The new include statements added may be unnecessary if the corresponding classes or functions are not used in this file. Unused includes can lead to longer compile times and unnecessary dependencies.
Run the following script to check for unused includes:
Also applies to: 19-19, 31-31
Let me gather more information to verify if these headers are truly unused by checking for types, functions, or symbols from these headers that might be used without directly mentioning the header name.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for unused include statements in MapCamera3d.cpp
includes=("Camera3dConfigFactory.h" "CoordHelper.h" "CoordinateSystemIdentifiers.h" "Logger.h" "MapCamera3DHelper.h")
for include in "${includes[@]}"; do
# Check if the header file is used in MapCamera3d.cpp
usage=$(grep -r "${include}" shared/src/map/camera/MapCamera3d.cpp | grep -v "#include" | wc -l)
if [ "$usage" -eq 0 ]; then
echo "Header ${include} appears to be unused."
fi
done
Length of output: 1455
Script:
#!/bin/bash
# Search for potential usages of types/symbols from these headers
echo "=== Searching for Camera3dConfigFactory usage ==="
rg -l "Camera3dConfigFactory" shared/src/map/camera/MapCamera3d.cpp
echo -e "\n=== Searching for CoordHelper usage ==="
rg -l "CoordHelper" shared/src/map/camera/MapCamera3d.cpp
echo -e "\n=== Searching for coordinate system related symbols ==="
rg -A 3 "CoordinateSystem|COORDINATE_SYSTEM" shared/src/map/camera/MapCamera3d.cpp
echo -e "\n=== Checking Coord related usage ==="
rg "Coord[^(Helper)]" shared/src/map/camera/MapCamera3d.cpp
echo -e "\n=== Show the class definition to understand dependencies ==="
ast-grep --pattern 'class MapCamera3d {
$$$
}'
Length of output: 16846
Signed-off-by: Nicolas Märki <maerki@ubique.ch>
Signed-off-by: Nicolas Märki <maerki@ubique.ch>
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
ios/graphics/Model/BaseGraphicsObject.swift(2 hunks)ios/graphics/Model/MultiBuffer.swift(1 hunks)ios/graphics/Model/Polygon/Polygon2d.swift(4 hunks)ios/graphics/Model/Quad/Quad2d.swift(8 hunks)ios/graphics/Model/Quad/Quad2dInstanced.swift(10 hunks)ios/graphics/Model/Text/TextInstanced.swift(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ios/graphics/Model/MultiBuffer.swift
🔇 Additional comments (23)
ios/graphics/Model/BaseGraphicsObject.swift (7)
40-46: Add error handling for buffer initialization.
The initialization of buffers could fail silently. A previous review comment suggested implementing proper error handling, which is still applicable here.
49-58: LGTM! Well-structured method signature.
The method signature is properly formatted for readability and correctly marked as open for subclass implementation.
62-65: LGTM! Appropriate compute method signature.
The compute method is correctly marked as open with an empty default implementation for optional subclass override.
90-108: LGTM! Well-structured render method with proper error handling.
The method includes appropriate guard statements and correctly delegates to the internal render method.
111-116: LGTM! Proper use of Swift's any keyword and error handling.
The compute method correctly uses the any keyword for protocol existential and includes appropriate guard statements.
123-124: LGTM! Well-documented parameter defaults.
The method signature is properly formatted with appropriate default values for the mask parameters.
36-38: LGTM! Verify thread safety of buffer access.
The new MultiBuffer properties are well-typed for their respective purposes. The class is marked as @unchecked Sendable, so ensure that all buffer access is properly synchronized using the lock property.
✅ Verification successful
Let me verify the thread safety by checking the MultiBuffer implementation and lock usage.
Let me try a different search pattern to find the MultiBuffer implementation.
Buffer access is properly synchronized with OSLock
The implementation is thread-safe:
BaseGraphicsObjecthas anOSLockproperty that's used during the rendering cycleMultiBufferuses triple buffering (bufferCount = 3) to handle concurrent access- Buffer access is coordinated through
getNextBufferwhich manages the buffer rotation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for buffer access patterns to verify proper locking
rg -A 3 'originOffsetBuffers|vpMatrixBuffers'
Length of output: 10019
Script:
#!/bin/bash
# Check MultiBuffer implementation and lock usage
rg -A 5 'class MultiBuffer'
# Check lock usage in BaseGraphicsObject
rg -A 3 'lock\.' ios/graphics/Model/BaseGraphicsObject.swift
Length of output: 92
Script:
#!/bin/bash
# Search for MultiBuffer in all Swift files
fd -e swift -x cat {} | rg -A 10 'MultiBuffer'
# Check for any synchronization mechanisms in BaseGraphicsObject
rg -A 3 'NSLock|DispatchQueue|@synchronized|lock' ios/graphics/Model/BaseGraphicsObject.swift
Length of output: 4314
ios/graphics/Model/Polygon/Polygon2d.swift (5)
26-36: LGTM: New matrix buffer property and initialization.
The addition of mMatrixBuffers and its initialization follows the established pattern for buffer management in the class.
39-48: LGTM: Method signature formatting improvements.
The reformatting of the method signature improves readability while maintaining the same functionality.
91-103: Add safety checks and consolidate buffer management.
The past review comments about buffer validation and duplication are still valid. Additionally, we should verify the matrix size assumption of 64 bytes.
#!/bin/bash
# Verify the matrix size assumption
ast-grep --pattern 'struct $matrix_type {
$$$
}' | grep -A 10 'simd_float4x4'183-195: Extract duplicated buffer management code.
The buffer management code is duplicated from the render method. The past review comments about extracting this into a shared method are still valid.
217-219: LGTM: Method signature formatting improvements.
The reformatting of the method signature improves readability while maintaining the same functionality.
ios/graphics/Model/Quad/Quad2dInstanced.swift (6)
29-30: LGTM: Buffer management improvements
The transition to MultiBuffer types and their initialization is well-structured and thread-safe. This change aligns with the broader initiative to improve buffer management across the graphics module.
Also applies to: 56-62
89-98: LGTM: Well-structured render method signature
The render method signature is properly updated with all necessary parameters and maintains consistency with the parent class.
214-237: LGTM: Well-implemented mask rendering
The mask rendering implementation is properly structured with appropriate parameter handling and state management.
147-159:
Fix unsafe pointer casting in matrix buffer management
The casting of vpMatrix and mMatrix from Int64 to Int before creating UnsafeRawPointer could lead to pointer truncation on 32-bit platforms.
Apply this diff to fix the unsafe casting:
-if let matrixPointer = UnsafeRawPointer(bitPattern: Int(vpMatrix)) {
+if let matrixPointer = UnsafeRawPointer(bitPattern: vpMatrix) {-if let matrixPointer = UnsafeRawPointer(bitPattern: Int(mMatrix)) {
+if let matrixPointer = UnsafeRawPointer(bitPattern: mMatrix) {179-201:
Improve error handling for buffer allocation failures
Using fatalError() for buffer allocation failures will crash the application in production. Consider implementing more graceful error handling.
Apply this diff to improve error handling:
let originOffsetBuffer = originOffsetBuffers.getNextBuffer(context)
if let bufferPointer = originOffsetBuffer?.contents()
.assumingMemoryBound(to: simd_float4.self)
{
bufferPointer.pointee.x = Float(originOffset.x - origin.x)
bufferPointer.pointee.y = Float(originOffset.y - origin.y)
bufferPointer.pointee.z = Float(originOffset.z - origin.z)
} else {
- fatalError()
+ NSLog("Failed to get origin offset buffer")
+ return
}
let originBuffer = originBuffers.getNextBuffer(context)
if let bufferPointer = originBuffer?.contents().assumingMemoryBound(
to: simd_float4.self)
{
bufferPointer.pointee.x = Float(origin.x)
bufferPointer.pointee.y = Float(origin.y)
bufferPointer.pointee.z = Float(origin.z)
} else {
- fatalError()
+ NSLog("Failed to get origin buffer")
+ return
}252-270:
Improve error handling in frame setup
Using fatalError() for buffer allocation failures will crash the application. Consider implementing more graceful error handling.
Apply this diff to improve error handling:
guard
let verticesBuffer = device.makeBuffer(
bytes: vertices,
length: MemoryLayout<Vertex>.stride * vertices.count,
options: []),
let indicesBuffer = device.makeBuffer(
bytes: indices,
length: MemoryLayout<UInt16>.stride * indices.count, options: []
)
else {
- fatalError("Cannot allocate buffers")
+ NSLog("Failed to allocate vertex or index buffers")
+ return
}Likely invalid or redundant comment.
ios/graphics/Model/Text/TextInstanced.swift (2)
31-33: LGTM: Good architectural improvement using MultiBuffer
The transition from MTLBuffer to MultiBuffer for managing buffers is a solid improvement. This change should help with better buffer management and synchronization across frames.
Also applies to: 41-48
278-279: LGTM: Proper implementation of buffer copying
The use of copyOrCreate method for buffer management is well implemented and maintains thread safety through the lock mechanism.
Also applies to: 291-292
ios/graphics/Model/Quad/Quad2d.swift (3)
39-51: LGTM: Buffer management improvements
The addition of mMatrixBuffers and its initialization follows the established pattern for multi-buffer management, improving thread safety and rendering efficiency.
151-159: Previous review comment about error handling is still applicable
The error handling for the originOffset buffer remains minimal, with a non-descriptive fatalError().
365-368: LGTM: Clean interface method update
The method signature update maintains functionality while improving code formatting. The error handling includes a descriptive message, which is good practice.
| let vpMatrixBuffer = vpMatrixBuffers.getNextBuffer(context) | ||
| if let matrixPointer = UnsafeRawPointer(bitPattern: Int(vpMatrix)) { | ||
| vpMatrixBuffer?.contents().copyMemory( | ||
| from: matrixPointer, byteCount: 64) | ||
| } | ||
| if let mMatrixPointer = UnsafeRawPointer(bitPattern: Int(mMatrix)) { | ||
| encoder.setVertexBytes(mMatrixPointer, length: 64, index: 2) | ||
| encoder.setVertexBuffer(vpMatrixBuffer, offset: 0, index: 1) |
There was a problem hiding this comment.
Fix unsafe Int64 to Int casting for matrix pointers
The current implementation may cause issues on 32-bit systems due to unsafe casting of Int64 to Int.
Apply this diff to fix both matrix buffer implementations:
-if let matrixPointer = UnsafeRawPointer(bitPattern: Int(vpMatrix)) {
+if let matrixPointer = UnsafeRawPointer(bitPattern: UInt(bitPattern: vpMatrix)) {
vpMatrixBuffer?.contents().copyMemory(from: matrixPointer, byteCount: 64)
}
-if let matrixPointer = UnsafeRawPointer(bitPattern: Int(mMatrix)) {
+if let matrixPointer = UnsafeRawPointer(bitPattern: UInt(bitPattern: mMatrix)) {
mMatrixBuffer?.contents().copyMemory(from: matrixPointer, byteCount: 64)
}Also applies to: 119-124
| let originOffsetBuffer = originOffsetBuffers.getNextBuffer(context) | ||
| if let bufferPointer = originOffsetBuffer?.contents() | ||
| .assumingMemoryBound(to: simd_float4.self) | ||
| { | ||
| bufferPointer.pointee.x = Float(originOffset.x - origin.x) | ||
| bufferPointer.pointee.y = Float(originOffset.y - origin.y) | ||
| bufferPointer.pointee.z = Float(originOffset.z - origin.z) | ||
| } else { | ||
| fatalError() |
There was a problem hiding this comment.
Replace fatalError() with proper error handling
Using fatalError() for buffer handling can lead to app crashes. This should be replaced with graceful error handling.
Apply this diff to improve error handling for all buffer operations:
if let bufferPointer = originOffsetBuffer?.contents().assumingMemoryBound(to: simd_float4.self) {
bufferPointer.pointee.x = Float(originOffset.x - origin.x)
bufferPointer.pointee.y = Float(originOffset.y - origin.y)
bufferPointer.pointee.z = Float(originOffset.z - origin.z)
} else {
- fatalError()
+ print("Error: Failed to bind originOffsetBuffer")
+ return
}
if let bufferPointer = originBuffer?.contents().assumingMemoryBound(to: simd_float4.self) {
bufferPointer.pointee.x = Float(origin.x)
bufferPointer.pointee.y = Float(origin.y)
bufferPointer.pointee.z = Float(origin.z)
} else {
- fatalError()
+ print("Error: Failed to bind originBuffer")
+ return
}
if let bufferPointer = aspectRatioBuffer?.contents().assumingMemoryBound(to: simd_float1.self) {
bufferPointer.pointee = Float(context.aspectRatio)
} else {
- fatalError()
+ print("Error: Failed to bind aspectRatioBuffer")
+ return
}Also applies to: 151-159, 163-169
| guard | ||
| let verticesBuffer = device.makeBuffer( | ||
| bytes: vertecies, | ||
| length: MemoryLayout<Vertex>.stride * vertecies.count, | ||
| options: []), | ||
| let indicesBuffer = device.makeBuffer( | ||
| bytes: indices, | ||
| length: MemoryLayout<UInt16>.stride * indices.count, options: [] | ||
| ) | ||
| else { |
There was a problem hiding this comment.
Improve error handling in buffer allocation
The vertex and index buffer allocation should handle errors gracefully instead of using fatalError().
Apply this diff:
guard
let verticesBuffer = device.makeBuffer(
bytes: vertecies,
length: MemoryLayout<Vertex>.stride * vertecies.count,
options: []),
let indicesBuffer = device.makeBuffer(
bytes: indices,
length: MemoryLayout<UInt16>.stride * indices.count,
options: []
)
else {
- fatalError("Cannot allocate buffers")
+ print("Error: Failed to allocate vertex or index buffers")
+ return
}Committable suggestion skipped: line range outside the PR's diff.
| assert( | ||
| self.indicesCount * 2 == MemoryLayout<UInt16>.stride | ||
| * indices.count) |
There was a problem hiding this comment.
Fix incorrect assert condition
The current assert condition self.indicesCount * 2 == MemoryLayout<UInt16>.stride * indices.count appears to be incorrect. The left side multiplies the count by 2, while the right side already includes the stride (which is 2 bytes for UInt16). This will always fail.
Consider this fix:
-assert(self.indicesCount * 2 == MemoryLayout<UInt16>.stride * indices.count)
+assert(self.indicesCount == indices.count)📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert( | |
| self.indicesCount * 2 == MemoryLayout<UInt16>.stride | |
| * indices.count) | |
| assert( | |
| self.indicesCount == indices.count) |
Signed-off-by: Nicolas Märki <maerki@ubique.ch>
Signed-off-by: Nicolas Märki <maerki@ubique.ch>
Signed-off-by: Nicolas Märki <maerki@ubique.ch>
Signed-off-by: Nicolas Märki <maerki@ubique.ch>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
shared/src/map/camera/MapCamera3d.h (1)
92-94: Consider adding documentation for the complex return type.The
computeMatricesmethod returns a tuple of vectors and Vec3D. Consider adding documentation to explain:
- The purpose and content of each vector in the tuple
- When
onlyReturnResultshould be true/false- The meaning of the Vec3D component
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
shared/src/map/camera/MapCamera3d.cpp(63 hunks)shared/src/map/camera/MapCamera3d.h(9 hunks)
🔇 Additional comments (7)
shared/src/map/camera/MapCamera3d.h (3)
13-26: LGTM! Header organization is clean and logical.
The header file organization follows good practices with related headers grouped together.
49-50: LGTM! Method signatures improved with better parameter organization.
The updated signatures with optional parameters provide more flexibility while maintaining clear parameter intent.
Also applies to: 164-165
296-298: LGTM! Good thread safety improvement.
The addition of separate mutexes for parameters and matrices provides fine-grained thread safety control.
shared/src/map/camera/MapCamera3d.cpp (4)
1378-1380: LGTM: Thread-safe scaling factor implementation
The implementation correctly uses mutex locking to protect access to the cached scaling factor.
1464-1469: LGTM: Robust bounds correction implementation
The implementation properly handles:
- Coordinate system conversion
- Value clamping
- Position validation
1628-1636: LGTM: Robust interpolation implementation
The implementation includes:
- Proper bounds checking
- Handling of empty interpolation values
- Correct normalization of zoom values
37-38:
Fix inverted zoom level constants
The zoom level constants are defined incorrectly:
GLOBE_MIN_ZOOMis set to 200,000,000GLOBE_MAX_ZOOMis set to 5,000,000
This inversion could cause issues with zoom constraints and clamping.
Apply this fix:
-#define GLOBE_MIN_ZOOM 200'000'000
-#define GLOBE_MAX_ZOOM 5'000'000
+#define GLOBE_MIN_ZOOM 5'000'000
+#define GLOBE_MAX_ZOOM 200'000'000Likely invalid or redundant comment.
| , screenPixelAsRealMeterFactor(0.0254 / screenDensityPpi * mapCoordinateSystem.unitToScreenMeterFactor) | ||
| , focusPointPosition(CoordinateSystemIdentifiers::EPSG4326(), 0, 0, 0) | ||
| , cameraPitch(0) | ||
| , zoomMin(GLOBE_MIN_ZOOM) | ||
| , zoomMax(GLOBE_MAX_ZOOM) | ||
| , lastOnTouchDownPoint(std::nullopt) | ||
| , bounds(mapCoordinateSystem.bounds) | ||
| , origin(0, 0, 0) | ||
| , cameraZoomConfig(Camera3dConfigFactory::getBasicConfig()) | ||
| , coordinateConversionHelper(mapInterface->getCoordinateConverterHelper()) { |
There was a problem hiding this comment.
Fix member initialization order
The initialization list has potential undefined behavior as coordinateConversionHelper is used in earlier initializations (e.g., focusPointPosition, bounds) but initialized last.
Reorder the initialization list to match the declaration order in the class and ensure members are initialized before use.
| std::lock_guard<std::recursive_mutex> lock(matrixMutex); | ||
| return VectorHelper::clone(vpMatrix); | ||
| } |
There was a problem hiding this comment.
Improve thread safety in matrix updates
The matrix update methods use locks but there might be race conditions:
getVpMatrix()locks only the matrix mutexupdateMatrices()locks only the param mutex- Both methods access shared state
This could lead to inconsistent matrix state during rendering.
Consider using a single lock or implementing a double-checked locking pattern to ensure thread safety.
Also applies to: 390-392
| void MapCamera3d::setCameraConfig(const Camera3dConfig &config, std::optional<float> durationSeconds, | ||
| std::optional<float> targetZoom, const std::optional<::Coord> &targetCoordinate) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure proper cleanup of animation objects
The camera configuration update creates multiple animation objects but cleanup relies solely on animation completion callbacks. Consider:
- Adding explicit cleanup in destructor
- Using RAII patterns for animation lifecycle
- Handling animation cancellation during reconfiguration
Add a method to properly cleanup ongoing animations:
void cleanupAnimations() {
std::lock_guard<std::recursive_mutex> lock(animationMutex);
pitchAnimation = nullptr;
verticalDisplacementAnimation = nullptr;
zoomAnimation = nullptr;
coordAnimation = nullptr;
}Also applies to: 1523-1531, 1535-1543
|
|
||
| return { | ||
| (renderCoord.z * sin(renderCoord.y) * cos(renderCoord.x)) - origin.x, | ||
| (renderCoord.z * cos(renderCoord.y)) - origin.y, | ||
| (-renderCoord.z * sin(renderCoord.y) * sin(renderCoord.x)) - origin.z, | ||
| 1.0 | ||
| }; | ||
| return {(renderCoord.z * sin(renderCoord.y) * cos(renderCoord.x)) - origin.x, (renderCoord.z * cos(renderCoord.y)) - origin.y, | ||
| (-renderCoord.z * sin(renderCoord.y) * sin(renderCoord.x)) - origin.z, 1.0}; |
There was a problem hiding this comment.
💡 Codebase verification
Coordinate conversion lacks validation for edge cases
The coordinate conversion in MapCamera3d.cpp does not perform validation for:
- NaN or infinity value checks
- Handling of coordinate system bounds
- Ensuring numerical precision in trigonometric operations
🔗 Analysis chain
Add validation for coordinate conversion edge cases
The cartesian coordinate conversion lacks validation for edge cases:
- No checks for NaN or infinity values
- No handling of coordinate system bounds
- Potential loss of precision in trigonometric calculations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for coordinate validation patterns in similar code
rg -A 3 "isnan|isinf|std::numeric_limits" shared/src/map/camera/
Length of output: 66
Script:
#!/bin/bash
# Search for alternative coordinate validation patterns in shared/src/map/camera/
rg -A 3 "validate|assert|check|ensure" shared/src/map/camera/
Length of output: 16516
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
shared/src/map/camera/MapCamera3d.h (2)
92-94: Add documentation for matrix computation methodsThese methods appear to be critical for the camera's matrix calculations. Consider adding documentation to explain:
- The purpose and relationship between these methods
- The components of the returned tuple in
computeMatrices- When
onlyReturnResultshould be true/false
173-173: Document coordinate distance threshold logicThe
coordIsFarAwayFromFocusPointmethod needs documentation to explain:
- What constitutes "far away" (the threshold used)
- The impact on camera behavior
- The relationship with viewport-based camera distance calculation
Also applies to: 189-189
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
shared/src/map/camera/MapCamera3d.cpp(63 hunks)shared/src/map/camera/MapCamera3d.h(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- shared/src/map/camera/MapCamera3d.cpp
🔇 Additional comments (3)
shared/src/map/camera/MapCamera3d.h (3)
13-26: LGTM! Well-organized header includes
The header includes are logically grouped and the new additions (Camera3dConfig.h, CameraInterpolation.h) properly support the enhanced camera functionality.
49-50: LGTM! Improved method signature formatting
The reformatted signature enhances readability while maintaining the same functionality.
164-165: LGTM! Enhanced camera configuration flexibility
The updated signature provides better control over camera transitions with optional duration and target parameters.
| std::recursive_mutex paramMutex; | ||
| std::recursive_mutex matrixMutex; | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Potential deadlock risks identified with mutex locking order
- Multiple instances where
animationMutexandparamMutexare locked in nested scopes - Inconsistent locking order may lead to circular wait conditions
🔗 Analysis chain
Verify mutex locking order to prevent deadlocks
The addition of paramMutex and matrixMutex improves thread safety, but using multiple recursive mutexes requires careful consideration:
- Document the expected locking order to prevent deadlocks
- Consider if recursive mutexes are necessary or if regular mutexes would suffice
- Verify that nested locks are actually needed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential deadlock scenarios in mutex usage
echo "Searching for mutex lock patterns..."
rg -A 5 "std::lock_guard<std::recursive_mutex>" shared/src/map/camera/MapCamera3d.cpp
echo "Searching for nested lock scenarios..."
rg -B 5 -A 5 "lock_guard.*paramMutex|lock_guard.*matrixMutex" shared/src/map/camera/MapCamera3d.cpp
Length of output: 25714
Summary by CodeRabbit
New Features
getScalingFactor()across various classes, enhancing functionality for retrieving scaling factors relevant to rendering and camera adjustments.renderSemaphoreinMCMapViewfor improved rendering control through semaphore-based synchronization.Bug Fixes
Refactor
MultiBufferfor efficient data handling.MapCamera3d, enhancing clarity and maintainability.Tiled2dMapVectorSourceSymbolDataManagerfor improved rendering efficiency.