Java bindings for offscreen map rendering #652
Conversation
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 SwiftLint (0.57.0)ios/maps/MCFontLoader.swiftWalkthroughThis pull request introduces significant enhancements to the OpenMobileMaps library, focusing on JVM and standalone rendering capabilities. Key changes include the addition of GitHub Actions workflows for Maven builds, refined build configurations, and improved JNI bindings. The project now supports offscreen rendering using OSMesa, primarily targeting Linux on amd64 architecture, with enhanced error handling, texture management, and cross-platform compatibility. Changes
Sequence DiagramsequenceDiagram
participant JVM as Java Runtime
participant NativeLib as Native Library
participant OSMesa as OSMesa Context
participant MapRenderer as Map Renderer
JVM->>NativeLib: Initialize Library
NativeLib->>OSMesa: Create Rendering Context
OSMesa-->>NativeLib: Context Created
NativeLib->>MapRenderer: Configure Renderer
MapRenderer->>OSMesa: Make Context Current
MapRenderer->>MapRenderer: Render Map
MapRenderer->>OSMesa: Read Pixel Data
OSMesa-->>MapRenderer: Pixel Buffer
MapRenderer-->>JVM: Rendered Image
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 69
🧹 Outside diff range and nitpick comments (57)
standalone/CMakeLists.txt (1)
1-3: Add standard CMake configuration and documentation.The file is missing some standard CMake practices that would improve its maintainability and clarity.
Consider adding these elements at the start of the file:
+cmake_minimum_required(VERSION 3.16) + +# Standalone test application for OpenMobileMaps offscreen rendering +project(mapscore-standalone + VERSION 1.0 + DESCRIPTION "Standalone test application for OpenMobileMaps with OSMesa support" + LANGUAGES CXX) + #### # mapscore-standalone ####.github/workflows/jvm_package_deploy.yml (1)
1-19: Consider adding workflow documentation.While the workflow is functional, it would benefit from adding comments to explain:
- The purpose of each APT package
- The expected format of tags that trigger the workflow
- The relationship with the reused workflow
Add documentation at the top of the file:
name: Maven build and deploy JVM +# This workflow builds and deploys the JVM package for OpenMobileMaps with OpenGL/OSMesa support +# Triggered on tag pushes and requires the following dependencies: +# - cmake, make, clang: Build tools +# - libgl-dev, libgles-dev: OpenGL development files +# - libosmesa6-dev: Off-screen rendering support +# +# The workflow reuses the Maven deployment workflow from workflows-backend repository on: push: tags🧰 Tools
🪛 actionlint
4-4: "push" section is scalar node but mapping node is expected
(syntax-check)
9-9: reusable workflow call "UbiqueInnovation/workflows-backend/.github/workflows/mvn_package_deploy.yml" at "uses" is not following the format "owner/repo/path/to/workflow.yml@ref" nor "./path/to/workflow.yml". see https://docs.github.com/en/actions/learn-github-actions/reusing-workflows for more details
(workflow-call)
🪛 yamllint
[error] 2-2: trailing spaces
(trailing-spaces)
jvm/readme.md (5)
1-8: Maintain consistent terminology throughout the documentThe document uses both "off-screen" and "offscreen". Choose one form and use it consistently.
Apply this change throughout the document:
-# Java bindings for off-screen use of openmobilemaps +# Java bindings for offscreen use of openmobilemaps🧰 Tools
🪛 LanguageTool
[uncategorized] ~5-~5: Do not mix variants of the same word (‘offscreen’ and ‘off-screen’) within a single text.
Context: ... For now, this only explicitly supports offscreen rendering via [OSMesa](https://docs.mes...(EN_WORD_COHERENCY)
24-27: Add language specifier to code blockFor better syntax highlighting and documentation consistency, specify the language for the code block.
-``` +```shell apt install --no-install-recommends maven cmake make clang libgl-dev libgles-dev libosmesa6-dev openjdk-21-jdk-headless<details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint</summary> 25-25: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> --- `45-48`: **Add language specifier to code block** For better syntax highlighting and documentation consistency, specify the language for the code block. ```diff -``` +```shell git submodule init git submodule update<details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint</summary> 45-45: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> --- `51-54`: **Enhance test execution documentation** The current note about "simple tests" could be more descriptive. Consider adding: 1. What types of tests are included 2. Where to find test results 3. How to run specific test categories Also, add the shell language specifier to the code block: ```diff -``` +```shell mvn clean install<details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint</summary> 52-52: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> --- `1-54`: **Consider enhancing documentation structure** To improve the documentation's completeness, consider adding these sections: 1. Troubleshooting guide for common issues 2. Contribution guidelines 3. Roadmap for supporting additional platforms 4. Examples of basic usage Would you like assistance in drafting these additional sections? <details> <summary>🧰 Tools</summary> <details> <summary>🪛 LanguageTool</summary> [uncategorized] ~5-~5: Do not mix variants of the same word (‘offscreen’ and ‘off-screen’) within a single text. Context: ... For now, this only explicitly supports offscreen rendering via [OSMesa](https://docs.mes... (EN_WORD_COHERENCY) </details> <details> <summary>🪛 Markdownlint</summary> 25-25: null Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 36-36: null Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 45-45: null Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 52-52: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </blockquote></details> <details> <summary>android/src/main/cpp/graphics/OpenGlHelper.h (1)</summary><blockquote> `57-57`: **Add documentation for the error checking macro.** The macro would benefit from documentation explaining its purpose and usage. Add documentation above the macro: ```diff +/** + * Macro for checking OpenGL errors with automatic file and line information. + * Usage: Call after any OpenGL operation that might generate errors. + * @return GLenum The last error code or GL_NO_ERROR if no errors occurred + */ #define glCheckError() glCheckError_(__FILE__, __LINE__)jvm/src/main/java/io/openmobilemaps/mapscore/graphics/BufferedImageTextureHolder.java (1)
8-13: Enhance class documentation and consider null safety.The class documentation should mention thread-safety characteristics. Also, consider adding null validation for the
imagefield.-/** Implementation of the TextureHolderInterface for BufferedImage and OpenGL. */ +/** + * Thread-safe implementation of the TextureHolderInterface for BufferedImage and OpenGL. + * This class manages the lifecycle of OpenGL textures created from BufferedImages, + * ensuring proper resource cleanup through reference counting. + */djinni/map/loader/loader.djinni (2)
35-39: Consider adding documentation for async methodsWhile the methods are self-explanatory, it would be helpful to document:
- The expected behavior when cancellation occurs
- Whether these methods can be called multiple times with the same URL
- Any timeout behavior
Add documentation comments above each async method:
+ # Asynchronously loads a texture from the given URL + # Returns a future that will complete with the texture result + # If a request for the same URL is already in progress, implementation + # may return the same future load_texture_async(url: string, etag: optional<string>): future<texture_loader_result>; + # Asynchronously loads data from the given URL + # Returns a future that will complete with the data result + # If a request for the same URL is already in progress, implementation + # may return the same future load_data_async(url: string, etag: optional<string>): future<data_loader_result>;
35-39: Consider splitting the loader interfaceThe current interface handles both texture and data loading, which might violate the Single Responsibility Principle. Consider:
- Splitting into separate
TextureLoaderInterfaceandDataLoaderInterface- Creating a composite loader that implements both interfaces
- Clarifying the behavior of
cancel()- does it cancel both texture and data loading operations?This would make the interfaces more focused and easier to implement correctly.
Would you like me to propose a detailed design for splitting these interfaces?
jvm/CMakeLists.txt (2)
23-48: Use SYSTEM includes for external dependencies.Mark external include directories as SYSTEM to suppress warnings from those headers and clarify dependency boundaries.
Apply this diff:
target_include_directories(mapscore_jni PRIVATE - ../external/djinni/support-lib/jni - ${JNI_INCLUDE_DIRS} + SYSTEM + ../external/djinni/support-lib/jni + ${JNI_INCLUDE_DIRS} + ${OSMESA_INCLUDE_DIRS} + PRIVATE ../bridging/android/jni/graphics ../bridging/android/jni/graphics/common # ... other internal includes ... - ${OSMESA_INCLUDE_DIRS} )
1-60: Consider restructuring for multi-platform support.Given the PR objectives mention potential future platform support, consider restructuring the CMake configuration to better handle multi-platform builds:
- Move platform-specific logic into separate platform configuration files
- Create a proper CMake package configuration for downstream consumers
- Add proper version information and compatibility checks
Consider creating these additional files:
cmake/platforms/linux.cmakecmake/platforms/windows.cmake(for future use)cmake/mapscore-config.cmake.incmake/mapscore-version.cmakeshared/src/map/coordinates/CoordinateSystemIdentifiers.cpp (1)
Line range hint
44-57: Consider enhancing error messages with supported values.The error handling implementation is good, but the error messages could be more helpful by including the list of supported identifiers/values. This would help developers quickly identify the issue without having to check the documentation.
Example enhancement for
fromCrsIdentifier:- throw std::invalid_argument("Unsupported CRS identifier: " + identifier); + throw std::invalid_argument("Unsupported CRS identifier: " + identifier + + ". Supported formats: EPSG:3857, EPSG:4326, EPSG:2056, EPSG:21781");And for
unitToMeterFactor:- throw std::invalid_argument("Unsupported coordinate system identifier: " + std::to_string(coordinateSystemIdentifier)); + throw std::invalid_argument("Unsupported coordinate system identifier: " + std::to_string(coordinateSystemIdentifier) + + ". Supported systems: EPSG:3857, EPSG:4326, EPSG:2056, EPSG:21781");Also applies to: 59-69
CMakeLists.txt (1)
18-69: Improve modularity and security of include directories.The current include structure reveals tight coupling between components and potentially exposes internal implementation details.
Consider:
- Organizing includes into logical modules
- Minimizing PUBLIC includes
- Using interface libraries for better encapsulation
- Adding INTERFACE includes for header-only dependencies
Example restructuring:
# Create interface libraries for external dependencies add_library(mapscore_external INTERFACE) target_include_directories(mapscore_external INTERFACE external/protozero/protozero/include/ external/vtzero/vtzero/include/ external/earcut/earcut/include/mapbox/ ) # Group related includes into components add_library(mapscore_graphics INTERFACE) target_include_directories(mapscore_graphics INTERFACE shared/src/graphics shared/src/graphics/helpers ) # Link components target_link_libraries(mapscore PRIVATE mapscore_external mapscore_graphics )jvm/src/main/cpp/jni/GlTextureHelper.cpp (1)
1-5: Consider adding headers for improved error handling.Consider adding these headers to enhance error handling capabilities:
+#include <sstream> +#include <stdexcept>android/src/main/cpp/graphics/shader/ColorShaderOpenGl.cpp (3)
32-32: Good addition of error checking, consider enhancing error handling further.The addition of
glCheckError()calls at critical shader setup points is valuable, especially for offscreen rendering where visual feedback isn't available. However, consider enhancing the error handling to be more robust.Consider adding specific error handling for program linking status:
glLinkProgram(program); // create OpenGL program executables glCheckError(); + GLint linkStatus = 0; + glGetProgramiv(program, GL_LINK_STATUS, &linkStatus); + if (linkStatus != GL_TRUE) { + GLint infoLen = 0; + glGetProgramiv(program, GL_INFO_LOG_LENGTH, &infoLen); + if (infoLen > 1) { + std::vector<char> infoLog(infoLen); + glGetProgramInfoLog(program, infoLen, nullptr, infoLog.data()); + // Log or handle the error appropriately + throw std::runtime_error("Shader linking failed: " + std::string(infoLog.data())); + } + }Also applies to: 35-35
Line range hint
89-98: Optimize alpha handling in fragment shader.The current fragment shader's alpha handling could be simplified. Setting alpha to 1.0 and then multiplying by the input alpha is redundant and might lead to unexpected blending results.
Consider simplifying the alpha handling:
return OMMVersionedGlesShaderCode(320 es, precision mediump float; uniform vec4 vColor; out vec4 fragmentColor; void main() { - fragmentColor = vColor; - fragmentColor.a = 1.0; - fragmentColor *= vColor.a; + fragmentColor = vec4(vColor.rgb, vColor.a); });
Line range hint
41-47: Optimize mutex usage in preRender method.The current mutex lock scope in
preRenderis unnecessarily small and uses an empty block. The synchronization can be simplified while maintaining thread safety.Consider restructuring the mutex usage:
int program = openGlContext->getProgram(programName); int mColorHandle = glGetUniformLocation(program, "vColor"); - { - glUniform4fv(mColorHandle, 1, &color[0]); - std::lock_guard<std::mutex> lock(dataMutex); - } + std::lock_guard<std::mutex> lock(dataMutex); + glUniform4fv(mColorHandle, 1, &color[0]);jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/FontJsonManifestReader.java (1)
18-24: Add input validation and specific exception handling.Consider these improvements:
- Add null check for the input stream
- Handle specific Jackson exceptions (JsonParseException, JsonMappingException) separately
Here's the suggested implementation:
static FontData read(InputStream stream) throws IOException, InvalidManifestException { + if (stream == null) { + throw new IllegalArgumentException("Input stream cannot be null"); + } try { return extractFontData(new ObjectMapper().readTree(stream)); - } catch (IllegalArgumentException e) { + } catch (JsonParseException e) { + throw new InvalidManifestException("Invalid JSON format: " + e.getMessage()); + } catch (JsonMappingException e) { + throw new InvalidManifestException("JSON mapping error: " + e.getMessage()); + } catch (IllegalArgumentException e) { throw new InvalidManifestException(e.getMessage()); } }shared/src/logger/Logger.cpp (2)
33-35: Add documentation for log level valuesThe LOG_LEVEL define would benefit from documentation explaining the available levels and their meanings.
Consider adding a comment block:
#ifndef LOG_LEVEL +/* Log levels: + * 0 = ERROR - Critical errors that need immediate attention + * 1 = WARNING - Important events that aren't errors + * 2 = DEBUG - Detailed information for debugging (default) + * 3 = INFO - General information about program execution + * 4 = TRACE - Very detailed information for tracing + */ #define LOG_LEVEL 2 // == LogDebug #endif
Line range hint
106-177: Improve code structure and error handlingSeveral improvements could enhance the reliability and maintainability of this code:
- The color formatting code is duplicated across each log level case
- The va_args handling in the Android section lacks error checking
- The priority value isn't bounds-checked
Consider these improvements:
- Extract color formatting to a helper function:
std::string getColorCode(int priority) { switch (priority) { case 0: return "\033[fg192,53,40;"; case 1: return "\033[fg211,84,0;"; // ... other cases default: return ""; } }
- Add va_args error handling:
va_list args; va_start(args, fmt); -__android_log_print(androidPrio, tag, fmt, args); +int result = __android_log_print(androidPrio, tag, fmt, args); va_end(args); +if (result < 0) { + // Handle error or fallback to stderr +}
- Add priority bounds checking:
void Logger::log(int prio, const char *tag, const char *fmt, ...) const { + if (prio < 0 || prio > 4) { + // Handle invalid priority level + return; + }shared/src/map/layers/objects/QuadMaskObject.cpp (1)
Line range hint
87-124: Remove commented-out code.The large block of commented-out code should be removed as it:
- Clutters the file
- May become outdated or misleading
- Is preserved in version control if needed for reference
Remove lines 87-124 to improve code maintainability.
shared/src/map/layers/objects/PolygonGroup2dLayerObject.cpp (4)
Line range hint
47-54: Document coordinate system assumptions and bounds.The method processes coordinates without explicit validation of ranges. For spherical coordinates (when
is3d=true), this could lead to unexpected results with invalid lat/lon values.Consider adding documentation and validation:
void PolygonGroup2dLayerObject::setVertices(const std::vector<std::tuple<std::vector<::Coord>, int>> & vertices, const std::vector<uint16_t> & indices, bool is3d) { + // Coordinates are expected to be in [format] system where: + // - For is3d=true: x=longitude [-180,180], y=latitude [-90,90] + // - For is3d=false: x,y in render coordinate system double avgX = 0.0f, avgY = 0.0f;
Line range hint
77-83: Consider parameterizing the sphere radius for 3D transformations.The spherical to Cartesian coordinate transformation currently uses a hardcoded radius of 1.0, which might limit flexibility for different use cases.
Consider making the radius configurable:
+ // Default radius for sphere projection + const double sphereRadius = 1.0; double rx = is3d ? 1.0 * sin(avgY) * cos(avgX) : avgX; - double ry = is3d ? 1.0 * cos(avgY) : avgY; - double rz = is3d ? -1.0 * sin(avgY) * sin(avgX) : 0.0; + double ry = is3d ? sphereRadius * cos(avgY) : avgY; + double rz = is3d ? -sphereRadius * sin(avgY) * sin(avgX) : 0.0;
Line range hint
47-100: Consider separating coordinate transformation logic.The current implementation combines coordinate system transformation logic with vertex buffer management. Consider extracting the transformation logic into a separate utility class to improve maintainability and reusability.
This would:
- Make the code more testable
- Allow reuse of transformation logic in other parts of the system
- Simplify the main class by reducing its responsibilities
Based on the code analysis, I can now provide a final response.
The review comment is valid but needs refinement.
The current implementation in PolygonGroup2dLayerObject directly creates SharedBytes from indices without validation, and the OpenGL implementation blindly trusts these indices when copying to the GPU buffer. This could lead to buffer overruns if indices reference vertices beyond the buffer bounds. However, the suggested fix should be modified to check against vertex count rather than raw buffer size:
+ if (!indices.empty()) { + size_t maxIndex = *std::max_element(indices.begin(), indices.end()); + if (maxIndex >= verticesBuffer.size() / 4) { // 4 components per vertex + LogError << "Invalid indices detected: index " << maxIndex << " out of vertex buffer bounds"; + return; + } + } auto i = SharedBytes((int64_t)indices.data(), (int32_t)indices.size(), (int32_t)sizeof(uint16_t)); auto v = SharedBytes((int64_t)verticesBuffer.data(), (int32_t)verticesBuffer.size(), (int32_t)sizeof(float));The validation should check against vertex count (buffer size / 4) since each vertex has 4 components, making the bounds check more precise.
🔗 Analysis chain
Line range hint
98-100: Consider adding error handling for invalid indices.The SharedBytes construction doesn't validate if indices are within bounds of the vertex buffer.
Add validation before creating SharedBytes:
+ if (!indices.empty() && indices.back() * 4 >= renderVertices.size()) { + LogError << "Invalid indices detected: index out of vertex buffer bounds"; + return; + } auto i = SharedBytes((int64_t)indices.data(), (int32_t)indices.size(), (int32_t)sizeof(uint16_t));
Let me gather more specific information about bounds checking in the codebase, particularly focusing on SharedBytes and index validation.
Let me check the implementation of PolygonGroup2dLayerObject to understand how indices are used.
Let me check how the indices are used in the OpenGL implementation to understand the potential impact of invalid indices.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Look for existing bounds checking in related files rg "SharedBytes.*bounds|range|check"Length of output: 45229
Script:
#!/bin/bash # Look for SharedBytes usage and index validation patterns rg -A 5 "SharedBytes.*indices" # Look for the specific file and surrounding context rg -l "PolygonGroup2dLayerObject"Length of output: 86168
Script:
#!/bin/bash # Look at the implementation of PolygonGroup2dLayerObject rg -A 10 "void PolygonGroup2dLayerObject::setVertices" shared/src/map/layers/objects/PolygonGroup2dLayerObject.cppLength of output: 1282
Script:
#!/bin/bash # Look at the OpenGL implementation for polygon rendering rg -A 10 "setVertices.*indices" android/src/main/cpp/graphics/objects/PolygonGroup2dOpenGl.cppLength of output: 609
shared/src/map/layers/objects/PolygonPatternGroup2dLayerObject.cpp (3)
Line range hint
33-47: Add input validation and documentation for coordinate averaging.The averaging logic lacks bounds checking and documentation explaining the mathematical approach.
Consider adding input validation and documentation:
void PolygonPatternGroup2dLayerObject::setVertices(const std::vector<std::tuple<std::vector<::Coord>, int>> & vertices, const std::vector<uint16_t> & indices, bool is3d) { + if (vertices.empty()) { + Logger::warn("PolygonPatternGroup2dLayerObject", "Empty vertices provided"); + return; + } + + // Calculate the centroid of all vertices to use as the origin for relative positioning. + // This helps maintain numerical precision for coordinates far from (0,0). double avgX = 0.0f, avgY = 0.0f; size_t totalPoints = 0;
Line range hint
71-76: Consider extracting coordinate transformation constants and logic.The transformation logic uses magic numbers and could benefit from named constants and a dedicated transformation method.
Consider refactoring:
+ // Constants for spherical projection + static constexpr double SPHERE_RADIUS = 1.0; + + // Helper function for 3D coordinate transformation + static Vec3D transformTo3D(double x, double y) { + return Vec3D( + SPHERE_RADIUS * sin(y) * cos(x), + SPHERE_RADIUS * cos(y), + -SPHERE_RADIUS * sin(y) * sin(x) + ); + } + - double rx = is3d ? 1.0 * sin(avgY) * cos(avgX) : avgX; - double ry = is3d ? 1.0 * cos(avgY) : avgY; - double rz = is3d ? -1.0 * sin(avgY) * sin(avgX) : 0.0; + Vec3D origin = is3d ? transformTo3D(avgX, avgY) : Vec3D(avgX, avgY, 0.0);
Line range hint
33-94: Consider performance implications for offscreen rendering.Given this is part of Java bindings for offscreen rendering, the coordinate transformation logic might impact performance for large datasets. Consider:
- Batch processing for large vertex sets
- SIMD optimizations for coordinate transformations
- Caching transformed coordinates when possible
shared/public/Vec2DHelper.h (4)
Line range hint
19-41: Consider adding equality operator and optimizing scalar multiplication.The operator overloads are well-implemented, but consider these improvements:
- Add
operator==for vector equality comparison in tests- Make scalar multiplication more efficient
Add these overloads:
inline bool operator==( const ::Vec2D& left, const ::Vec2D& right ) { return left.x == right.x && left.y == right.y; } // More efficient scalar multiplication with const reference inline Vec2D operator*( const double& val, const ::Vec2D& right) { return Vec2D(right.x * val, right.y * val); }
Line range hint
43-89: Add input validation and optimize basic geometric operations.The basic geometric operations (distance, midpoint, etc.) look good, but the
rotatemethod needs improvement:
M_PIisn't standard C++, usestd::numbers::pi(C++20) or define a constant- Consider caching the angle conversion factor
Suggested improvement:
static constexpr double PI = 3.14159265358979323846; static constexpr double DEG_TO_RAD = PI / 180.0; static inline ::Vec2D rotate(const ::Vec2D &p, const ::Vec2D &origin, double angleDegree) { const double angle = angleDegree * DEG_TO_RAD; const double sinAngle = std::sin(angle); const double cosAngle = std::cos(angle); // ... rest of the implementation }
Line range hint
90-125: Add safety checks and optimize convex hull implementation.The convex hull implementation could be improved:
- Add empty vector check
- Pre-allocate vector capacity
- Consider using std::vector::reserve to avoid reallocations
Suggested improvements:
static inline std::vector<size_t> convexHull(std::vector<Vec2D>& points) { if (points.empty()) { return {}; } std::vector<size_t> hull; hull.reserve(points.size()); // Avoid reallocations // ... rest of the implementation }
Line range hint
126-185: Add safety checks and optimize minimum area rectangle calculation.The minimum area rectangle implementation needs several improvements:
- Add empty vector check
- Add division by zero check in normalize
- Consider caching trigonometric calculations
Add these safety checks at the start:
static inline Quad2dD minimumAreaEnclosingRectangle(std::vector<Vec2D>& points) { if (points.empty()) { return Quad2dD({0, 0}, {0, 0}, {0, 0}, {0, 0}); } const std::vector<size_t> &hull = Vec2DHelper::convexHull(points); if (hull.empty()) { return Quad2dD({0, 0}, {0, 0}, {0, 0}, {0, 0}); } // ... rest of the implementation } static inline ::Vec2D normalize(const ::Vec2D &vector) { const double length = std::sqrt(Vec2DHelper::squaredLength(vector)); if (length < std::numeric_limits<double>::epsilon()) { return vector; // Return original vector if too small to normalize } return vector / length; }jvm/pom.xml (2)
1-16: Consider platform-specific compiler configuration.The POM uses modern versions of Java (21) and Kotlin (1.9.21), which is good. However, hardcoding
clang++as the C++ compiler might limit portability.Consider making the C++ compiler configurable via profiles or system properties to support different platforms in the future, as mentioned in the PR objectives.
- <CMAKE_CXX_COMPILER>clang++</CMAKE_CXX_COMPILER> + <CMAKE_CXX_COMPILER>${env.CXX_COMPILER:-clang++}</CMAKE_CXX_COMPILER>
98-130: Improve native library organization.The current configuration copies both debug and release .so files to the same directory without clear distinction. This could lead to confusion about which library is being used at runtime.
Consider:
- Using different directories for debug and release builds
- Adding a suffix to debug libraries
- Creating a mechanism to select the appropriate library based on build type
Example structure:
native/ release/ libmapscore_jni_*.so debug/ libmapscore_jni_*_debug.soThis can be achieved by modifying the resources plugin configuration:
<targetPath>native/${build.type}/</targetPath>You'll also need to update the library loading code to handle this directory structure.
jvm/src/test/java/io/openmobilemaps/mapscore/OffscreenMapRendererTest.java (3)
32-44: Consider enhancing error handling in loadResource method.The error handling could be more specific and informative:
- Separate handling for FileNotFoundException vs IOException
- Include the resource path in error messages
private static String loadResource(String resourceName) { try { try (InputStream resource = OffscreenMapRendererTest.class.getResourceAsStream("/" + resourceName)) { if (resource == null) { - fail("Resource not found: " + resourceName); + fail("Test resource not found in classpath: " + resourceName); } return new String(resource.readAllBytes()); } - } catch (IOException e) { - return fail("Failed to load resource", e); + } catch (IOException e) { + return fail(String.format("Failed to load test resource '%s': %s", resourceName, e.getMessage()), e); } }
65-67: Address TODOs with concrete implementation plan.The TODOs raise valid concerns about the image comparison approach. Consider:
- Using a perceptual diff library like java-image-diff
- Configuring test output directory via system property
Would you like me to help implement these improvements or create GitHub issues to track them?
104-110: Consider making test configuration more flexible.The debug mode and log level could be made configurable via system properties or test configuration file.
@BeforeAll public static void setUp() { - System.setProperty("io.openmobilemaps.mapscore.debug", "true"); - Logger.getLogger("").setLevel(Level.CONFIG); + System.setProperty("io.openmobilemaps.mapscore.debug", + System.getProperty("test.debug.enabled", "true")); + Logger.getLogger("").setLevel( + Level.parse(System.getProperty("test.log.level", "CONFIG"))); MapsCore.initialize(); }shared/src/map/layers/text/TextLayer.cpp (1)
223-225: Add error logging for font loading failures.The null check is a good defensive programming practice to prevent crashes. However, silently skipping texts with failed font loading could mask issues.
Consider adding error logging to help diagnose font loading failures:
auto fontData = fontLoader->loadFont(text->getFont()).fontData; if(!fontData) { + Logger::error("TextLayer", "Failed to load font for text: " + text->getText().begin()->text); continue; }shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSymbolObject.cpp (1)
135-135: LGTM! Consider enhancing error logging.The additional check for
fontResult->status == LoaderStatus::OKimproves robustness by ensuring only successfully loaded fonts are used. This is a good defensive programming practice.Consider enhancing the error logging in the else block to include the specific font loading status for better diagnostics:
- LogError << "font (" << fontString << ") could not be loaded"; + LogError << "font (" << fontString << ") could not be loaded. Status: " << static_cast<int>(fontResult ? fontResult->status : LoaderStatus::ERROR);jvm/src/main/java/io/openmobilemaps/mapscore/MapsCore.java (2)
34-37: Enhance exception messages with OS and architecture details for better diagnosticsIncluding the detected OS and architecture in the exception messages aids in diagnosing issues when the native library fails to load. This additional context can be invaluable during troubleshooting.
Apply this diff to enhance the exception messages:
if (lib == null) { throw new RuntimeException( - "Could not load native mapscore library from resource path " - + libResourcePath); + "Could not load native mapscore library from resource path " + + libResourcePath + " (os=" + os + ", arch=" + arch + ")"); } ... throw new RuntimeException( - "Could not load native mapscore library from resource path " + libResourcePath, + "Could not load native mapscore library from resource path " + libResourcePath + " (os=" + os + ", arch=" + arch + ")", e);Also applies to: 41-44
27-28: Adjust logging level for library loading messageThe use of
logger.configmay not display the log message in typical configurations since the default logging level isINFO. Consider usinglogger.infoto ensure the message is visible without additional configuration.Apply this diff to change the logging level:
-logger.config("Loading native library from resource path " + libResourcePath); +logger.info("Loading native library from resource path " + libResourcePath);jvm/src/main/java/io/openmobilemaps/mapscore/graphics/util/OSMesa.java (2)
32-35: Add@paramdocumentation forwidthandheightin the constructorThe constructor
OSMesa(int width, int height, int numSamples)lacks@paramtags forwidthandheightin the Javadoc comment. Including these will improve documentation completeness and help other developers understand the parameters' purposes.
65-72: Document parameters in themakeCurrentmethodThe method
makeCurrent(int width, int height, int numSamples)is missing@paramdocumentation for its parameters. Adding@paramtags forwidth,height, andnumSampleswill enhance readability and maintain consistency across the codebase.jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/FontLoader.java (1)
48-48: Make base DPI value configurableThe base DPI value
160.0is hardcoded in the calculation ofdpFactor. Since this code is intended for non-Android Java environments, consider making the base DPI value configurable or adding documentation to explain why160.0is appropriate in this context.jvm/src/main/cpp/jni/OSMesa.cpp (2)
76-86: Consider adding comments to explain the purpose of each member variable.While the struct members have descriptive names, it would be beneficial to add brief comments explaining their purpose and usage. This will improve the maintainability and readability of the code.
152-177: Consider adding a comment to explain the pixel format conversion.The comment on line 168 provides valuable information about the implicit byte-order conversion from BGRA to ARGB. However, it would be helpful to add a brief comment explaining why
glReadPixelsis used instead of directly accessingstate->buf, as mentioned in the commented-out line 172-173.jvm/src/main/java/io/openmobilemaps/mapscore/map/util/OffscreenMapRenderer.java (3)
24-33: Consider adding validation for constructor parameters.To improve the robustness of the constructor, consider adding validation checks for the input parameters to ensure they are within acceptable ranges. For example:
- Check that
widthandheightare positive and within reasonable limits.- Validate that
numSamplesis a positive integer.- Ensure that
dpiis a positive value.Throwing an
IllegalArgumentExceptionwith a descriptive message when the parameters are invalid can help catch potential issues early and provide better error messages to the users of this class.
57-60: Ensure proper cleanup of resources.The
destroymethod is responsible for cleaning up the map and OSMesa context. Consider the following:
- Verify that all resources associated with the map and context are properly released to avoid memory leaks.
- Handle any potential exceptions that may occur during the cleanup process gracefully.
- Document any preconditions or postconditions for calling the
destroymethod, such as the expected state of the renderer before and after destruction.
172-195: Consider providing more information in theMapLayerException.The custom
MapLayerExceptionclass is used to capture the state of layers that are not ready to render. To enhance the usefulness of this exception, consider the following:
- Include additional information in the exception, such as the specific reason why the layer is not ready (e.g., missing data, initialization failure, etc.).
- Provide a method to retrieve the complete state of the layer, including any relevant error messages or diagnostic information.
- Consider adding a method to suggest possible remediation steps or actions that can be taken to resolve the issue.
By enriching the
MapLayerExceptionwith more contextual information, it becomes a more valuable tool for diagnosing and troubleshooting rendering issues related to map layers.jvm/src/main/java/io/openmobilemaps/mapscore/map/util/Tiled2dMapVectorLayerBuilder.java (4)
46-47: Simplify FontLoader InitializationThe font loader is set using a ternary operation:
final FontLoaderInterface fontLoader = this.fontLoader != null ? this.fontLoader : new NoFontLoader();You can simplify this by initializing
fontLoaderwithNoFontLoaderby default when declared, eliminating the need for the ternary check.-@Nullable private FontLoaderInterface fontLoader; +private FontLoaderInterface fontLoader = new NoFontLoader();Then, in the
withFontLoadermethod, you can directly assign the new font loader without worrying about anullvalue.
70-78: Avoid OverloadingwithHttpLoaderwithout Differentiating ParametersThe class provides two overloaded methods
withHttpLoader(): one with no parameters and one that accepts anHttpClient. This can be confusing if not properly differentiated.Consider renaming one of the methods to make their purposes clearer, such as
withDefaultHttpLoader()andwithHttpLoader(HttpClient httpClient).
104-113: Clarify the Use of Local Data ProvidersThe
withLocalDataProvidermethod sets alocalDataProvider, but it's not clear how this interacts with the rest of the loaders, especially when an HTTP loader is also added.Consider adding documentation to explain how the local data provider and other loaders interact. If combining local and remote data sources is supported, ensure that the behavior is well-defined and tested.
115-121: Implement Logging inNoFontLoaderThe
NoFontLoaderclass returns aLoaderStatus.ERROR_404but does not log any information about the missing font. This can make debugging difficult.Consider adding logging to indicate when a font fails to load.
@Override public FontLoaderResult loadFont(@NotNull Font font) { // Log the missing font System.err.println("Font not found: " + font.getFamily()); return new FontLoaderResult(null, null, LoaderStatus.ERROR_404); }jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/HttpDataLoader.java (1)
36-36: Use consistent logging levelsCurrently, both
logger.infoandlogger.warningare used. For error cases, consider usinglogger.severeorlogger.warningto reflect the severity appropriately.Adjust logging levels to ensure consistency and clarity in log outputs.
standalone/main.cpp (1)
70-70: AdjustpixelDensityto a realistic or configurable value.The hardcoded value of
90.0fforpixelDensitymay not be suitable for all environments.Consider calculating
pixelDensitybased on the display's DPI or making it a command-line parameter for flexibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (7)
jvm/src/test/resources/fonts/Roboto-Regular.pngis excluded by!**/*.pngjvm/src/test/resources/golden/OffscreenMapRendererTest_testStyleJson.pngis excluded by!**/*.pngjvm/src/test/resources/golden/OffscreenMapRendererTest_testStyleJsonLabel.pngis excluded by!**/*.pngjvm/src/test/resources/golden/OffscreenMapRendererTest_testTiler_tile_7_66_44.pngis excluded by!**/*.pngjvm/src/test/resources/golden/OffscreenMapRendererTest_testTiler_tile_7_66_45.pngis excluded by!**/*.pngjvm/src/test/resources/golden/OffscreenMapRendererTest_testTiler_tile_7_67_44.pngis excluded by!**/*.pngjvm/src/test/resources/golden/OffscreenMapRendererTest_testTiler_tile_7_67_45.pngis excluded by!**/*.png
📒 Files selected for processing (45)
.github/workflows/jvm_build.yml(1 hunks).github/workflows/jvm_package_deploy.yml(1 hunks).gitignore(2 hunks)CMakeLists.txt(1 hunks)android/src/main/cpp/graphics/OpenGlHelper.h(1 hunks)android/src/main/cpp/graphics/shader/ColorShaderOpenGl.cpp(1 hunks)android/src/main/java/io/openmobilemaps/mapscore/map/util/OffscreenMapRenderer.kt(1 hunks)djinni/map/loader/loader.djinni(1 hunks)jvm/CMakeLists.txt(1 hunks)jvm/pom.xml(1 hunks)jvm/readme.md(1 hunks)jvm/src/main/cpp/jni/GlTextureHelper.cpp(1 hunks)jvm/src/main/cpp/jni/OSMesa.cpp(1 hunks)jvm/src/main/java/android/os/Parcelable.java(1 hunks)jvm/src/main/java/android/util/Log.java(1 hunks)jvm/src/main/java/androidx/annotation/Keep.java(1 hunks)jvm/src/main/java/com(1 hunks)jvm/src/main/java/io/openmobilemaps/mapscore/MapsCore.java(1 hunks)jvm/src/main/java/io/openmobilemaps/mapscore/graphics/BufferedImageTextureHolder.java(1 hunks)jvm/src/main/java/io/openmobilemaps/mapscore/graphics/util/GlTextureHelper.java(1 hunks)jvm/src/main/java/io/openmobilemaps/mapscore/graphics/util/OSMesa.java(1 hunks)jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/FontJsonManifestReader.java(1 hunks)jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/FontLoader.java(1 hunks)jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/HttpDataLoader.java(1 hunks)jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/LocalDataLoader.java(1 hunks)jvm/src/main/java/io/openmobilemaps/mapscore/map/util/MapTileRenderer.java(1 hunks)jvm/src/main/java/io/openmobilemaps/mapscore/map/util/OffscreenMapRenderer.java(1 hunks)jvm/src/main/java/io/openmobilemaps/mapscore/map/util/Tiled2dMapVectorLayerBuilder.java(1 hunks)jvm/src/main/java/io/openmobilemaps/mapscore/shared(1 hunks)jvm/src/main/java/kotlinx/parcelize/Parcelize.java(1 hunks)jvm/src/test/java/io/openmobilemaps/mapscore/OffscreenMapRendererTest.java(1 hunks)shared/public/Logger.h(0 hunks)shared/public/Tiled2dMapSourceImpl.h(0 hunks)shared/public/Vec2DHelper.h(1 hunks)shared/src/logger/Logger.cpp(2 hunks)shared/src/map/coordinates/CoordinateSystemIdentifiers.cpp(1 hunks)shared/src/map/layers/objects/PolygonGroup2dLayerObject.cpp(1 hunks)shared/src/map/layers/objects/PolygonPatternGroup2dLayerObject.cpp(1 hunks)shared/src/map/layers/objects/QuadMaskObject.cpp(1 hunks)shared/src/map/layers/text/TextLayer.cpp(1 hunks)shared/src/map/layers/tiled/vector/Tiled2dMapVectorLayerParserHelper.cpp(1 hunks)shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSymbolObject.cpp(1 hunks)standalone/CMakeLists.txt(1 hunks)standalone/main.cpp(1 hunks)standalone/readme.md(1 hunks)
💤 Files with no reviewable changes (2)
- shared/public/Logger.h
- shared/public/Tiled2dMapSourceImpl.h
✅ Files skipped from review due to trivial changes (7)
- android/src/main/java/io/openmobilemaps/mapscore/map/util/OffscreenMapRenderer.kt
- jvm/src/main/java/android/os/Parcelable.java
- jvm/src/main/java/android/util/Log.java
- jvm/src/main/java/androidx/annotation/Keep.java
- jvm/src/main/java/com
- jvm/src/main/java/io/openmobilemaps/mapscore/shared
- jvm/src/main/java/kotlinx/parcelize/Parcelize.java
🧰 Additional context used
🪛 actionlint
.github/workflows/jvm_package_deploy.yml
4-4: "push" section is scalar node but mapping node is expected
(syntax-check)
9-9: reusable workflow call "UbiqueInnovation/workflows-backend/.github/workflows/mvn_package_deploy.yml" at "uses" is not following the format "owner/repo/path/to/workflow.yml@ref" nor "./path/to/workflow.yml". see https://docs.github.com/en/actions/learn-github-actions/reusing-workflows for more details
(workflow-call)
🪛 yamllint
.github/workflows/jvm_package_deploy.yml
[error] 2-2: trailing spaces
(trailing-spaces)
🪛 LanguageTool
jvm/readme.md
[uncategorized] ~5-~5: Do not mix variants of the same word (‘offscreen’ and ‘off-screen’) within a single text.
Context: ... For now, this only explicitly supports offscreen rendering via [OSMesa](https://docs.mes...
(EN_WORD_COHERENCY)
standalone/readme.md
[uncategorized] ~7-~7: When ‘platform-specific’ is used as a modifier, it is usually spelled with a hyphen.
Context: ... be used directly from C++, without the platform specific bindings. Currently, this can be built...
(SPECIFIC_HYPHEN)
🪛 Markdownlint
jvm/readme.md
25-25: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
36-36: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
45-45: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
52-52: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
standalone/readme.md
25-25: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
31-31: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (47)
.github/workflows/jvm_build.yml (2)
9-13: LGTM: Input parameters are well configured.
The configuration includes all necessary parameters:
- Correct reference to parent POM
- Recursive submodule checkout enabled
- Maven Central repository enabled
- Required system packages for OpenGL/Mesa development
12-12: Verify platform-specific dependencies.
The APT packages look appropriate for OpenGL/Mesa development on Linux. However, given the PR objective's mention of platform specificity:
- Consider adding a comment explaining these are Linux-specific dependencies
- Consider using platform-specific conditionals when support for other platforms is added
✅ Verification successful
Let me check the CMake files to understand the dependency requirements better.
Based on the analysis of the CMake files and source code, I can now provide a final response:
The APT packages are correctly specified for Linux/JVM builds using OSMesa
The dependencies are appropriate and complete for the Linux JVM build environment:
- The codebase explicitly requires OSMesa through
find_library(OSMESA_LIBRARIES)in the CMake configuration - The JVM implementation uses OSMesa for off-screen rendering as seen in
jvm/src/main/cpp/jni/OSMesa.cpp - The current platform-specific handling is done through CMake's system detection (
${CMAKE_SYSTEM_NAME}and${CMAKE_SYSTEM_PROCESSOR})
No additional platform-specific conditionals are needed at this time since:
- The workflow file is specifically for JVM builds on Linux
- Other platforms (Android, iOS) have their separate build configurations in their respective directories
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify if there are any other OpenGL/Mesa related dependencies in the codebase
# that might need to be included in the build environment
# Look for additional system dependencies in CMake files
rg -l "find_package|pkg_check_modules" | grep -i "CMakeLists.txt"
# Look for OpenGL/Mesa related includes
rg -l "#include.*?(gl|mesa)"
Length of output: 1389
Script:
#!/bin/bash
# Check CMake files for OpenGL/Mesa related dependencies
cat CMakeLists.txt jvm/CMakeLists.txt
# Check OSMesa usage
cat jvm/src/main/cpp/jni/OSMesa.cpp
# Check if there are any platform-specific conditionals already in place
rg -i "if.*?(win|linux|mac|apple|android|ios)" CMakeLists.txt jvm/CMakeLists.txt
Length of output: 11838
jvm/src/main/java/io/openmobilemaps/mapscore/graphics/util/GlTextureHelper.java (1)
1-4: LGTM! Clean package structure and minimal imports.
.github/workflows/jvm_package_deploy.yml (1)
14-14: Verify OpenGL/OSMesa package dependencies.
The APT packages look correct for Linux amd64 builds, but consider adding version constraints for better reproducibility.
Let's verify the package availability and versions:
#!/bin/bash
# Description: Check if the required packages are available in Ubuntu repositories
# and their versions
for pkg in cmake make clang libgl-dev libgles-dev libosmesa6-dev; do
echo "Checking $pkg:"
apt-cache policy $pkg
done.gitignore (3)
63-63: LGTM! Comprehensive build directory pattern.
The build-*/ pattern appropriately covers various build directory configurations, which is essential for cross-platform development.
76-77: LGTM! Standard gcc coverage pattern.
78-80: LGTM! Essential clangd patterns.
The added patterns for clangd are appropriate for C++ development:
.cache/- Excludes clangd's index cachecompile_commands.json- Excludes build system-generated compilation database
android/src/main/cpp/graphics/OpenGlHelper.h (1)
Line range hint 1-57: Consider consolidating error checking mechanisms.
The class currently has two similar but different error checking mechanisms (checkGlError and glCheckError_). This could lead to confusion about which one to use.
Additionally, since this code is part of the OSMesa offscreen rendering support:
- We should verify that all these GL error codes are supported in OSMesa context
- Consider adding OSMesa-specific error codes if any exist
Let's verify OSMesa compatibility:
✅ Verification successful
Let me gather more information about the error handling in OSMesa context:
Consolidation of error checking is not necessary, current implementation is appropriate
The review comment's concerns can be addressed based on the evidence:
-
The two error checking mechanisms serve different purposes:
checkGlErroris a simple error logger for operationsglCheckError_is a more detailed diagnostic tool with specific error messages
-
Regarding OSMesa compatibility:
- The codebase shows that OSMesa uses standard OpenGL error codes (GL_INVALID_ENUM, GL_INVALID_OPERATION, etc.)
- OSMesa-specific error handling is properly implemented at a higher level through
OSMesaErrorclass for context creation/management - The current GL error checking implementation covers all relevant error cases for both standard OpenGL and OSMesa contexts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for OSMesa error handling patterns in the codebase
rg -l "OSMesa.*Error"
# Search for any OSMesa-specific error codes we might need to handle
rg "OSMESA_[A-Z_]+"
Length of output: 2145
Script:
#!/bin/bash
# Check how OSMesa errors are handled in the codebase
rg -A 5 "OSMesa.*Error|OSMesaCreate"
# Look for any GL error handling in OSMesa related files
rg -A 5 "glGetError|GL_ERROR" standalone/ jvm/src/main/cpp/jni/
Length of output: 5624
jvm/src/main/java/io/openmobilemaps/mapscore/graphics/BufferedImageTextureHolder.java (1)
19-37: Consider texture dimension requirements.
The current implementation assumes texture dimensions match image dimensions exactly. However, OpenGL often requires textures to be power-of-2 sized. Consider adding support for this case.
djinni/map/loader/loader.djinni (1)
35-39: Verify the impact of adding async methods
The new async methods look good and follow the same parameter pattern as their synchronous counterparts. However, we should verify:
- That all language bindings (C++, Java, Objective-C) properly support the
futuretype - That existing implementations are prepared to handle these new async methods
Let's check for any existing implementations that need to be updated:
jvm/CMakeLists.txt (2)
53-60: Improve platform detection and naming scheme.
The current platform detection might not handle all cases correctly, particularly for ARM architectures which might be needed in the future.
Let's verify the current platform detection:
#!/bin/bash
# Check current platform detection
cmake -P - <<'EOF'
message("System name: ${CMAKE_SYSTEM_NAME}")
message("Processor: ${CMAKE_SYSTEM_PROCESSOR}")
message("Architecture: ${CMAKE_ARCHITECTURE_ID}")
EOFConsider this improved approach:
-string(TOLOWER ${CMAKE_SYSTEM_NAME} TARGET_OS)
-string(REPLACE "x86_64" "amd64" TARGET_ARCHITECTURE ${CMAKE_SYSTEM_PROCESSOR})
+# Determine OS
+if(CMAKE_SYSTEM_NAME STREQUAL "Linux")
+ set(TARGET_OS "linux")
+else()
+ message(FATAL_ERROR "Unsupported operating system: ${CMAKE_SYSTEM_NAME}")
+endif()
+
+# Determine architecture
+if(CMAKE_SYSTEM_PROCESSOR MATCHES "x86_64|amd64")
+ set(TARGET_ARCHITECTURE "amd64")
+elseif(CMAKE_SYSTEM_PROCESSOR MATCHES "aarch64|arm64")
+ set(TARGET_ARCHITECTURE "arm64")
+else()
+ message(FATAL_ERROR "Unsupported architecture: ${CMAKE_SYSTEM_PROCESSOR}")
+endif()13-19: Consider replacing GLOB_RECURSE with explicit source lists.
Using GLOB_RECURSE can lead to unexpected behavior if unintended files are added to the source directories. CMake's official documentation recommends against using GLOB_RECURSE for better build predictability.
Let's verify what files are being included:
shared/src/map/coordinates/CoordinateSystemIdentifiers.cpp (1)
12-12: LGTM! The stdexcept include enables proper error handling.
The addition of <stdexcept> properly supports the throwing of std::invalid_argument exceptions, improving error handling robustness.
CMakeLists.txt (1)
80-81: Add platform-specific conditions for subdirectories.
Given that the PR is currently limited to Linux amd64, the build system should explicitly handle platform restrictions.
+if(UNIX AND CMAKE_SYSTEM_PROCESSOR MATCHES "amd64")
add_subdirectory(standalone)
add_subdirectory(jvm)
+else()
+ message(STATUS "Skipping JVM bindings: currently supported only on Linux amd64")
+endif()jvm/src/main/cpp/jni/GlTextureHelper.cpp (1)
58-60: Address the image orientation question.
The comment "XXX: why isnt this upside down?" suggests uncertainty about the image orientation handling. This needs investigation as OpenGL typically expects textures to be bottom-up while most image formats are top-down.
Let's check if this is documented or handled elsewhere:
✅ Verification successful
The image orientation is correct by design
The image orientation is handled correctly through the following flow:
- Java side gets the image data using
BufferedImage.getRGB()which provides data in top-down order - The native code uses
GL_BGRAformat which preserves the orientation when the bytes are reinterpreted from ARGB integers - No explicit flipping is needed as both the source data and OpenGL texture coordinates align naturally
The comment can be safely removed as it's no longer relevant.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related image orientation handling
rg -l "GL_UNPACK_ALIGNMENT|GL_UNPACK_ROW_LENGTH|glPixelStorei"
Length of output: 64
Script:
#!/bin/bash
# Let's search more broadly for texture and image handling code
rg -i "texture|image" --type cpp -A 5 jvm/src/main/cpp/
# Also search for any GL state setup before texture operations
rg "glTex|glPixel" --type cpp -A 5 jvm/src/main/cpp/
Length of output: 4129
Script:
#!/bin/bash
# Let's look for the Java side of the texture handling to understand the data format
rg -l "GlTextureHelper" --type java
# And then examine the content of any found files
fd -e java . | xargs rg "createTextureARGB|GlTextureHelper" -A 5
Length of output: 3853
Script:
#!/bin/bash
# Let's examine how the BufferedImage is processed before being passed to createTextureARGB
rg -A 10 "createTexture.*BufferedImage" jvm/src/main/java/io/openmobilemaps/mapscore/graphics/util/GlTextureHelper.java
# Also look for any image processing or conversion code
rg "getRGB|getDataBuffer|BufferedImage" --type java -A 5 jvm/src/main/java/io/openmobilemaps/mapscore/graphics/
Length of output: 6071
jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/FontJsonManifestReader.java (3)
1-16: LGTM! Class structure and imports are well-organized.
The package-private visibility is appropriate for this utility class that's likely only used internally by FontLoader.
79-83: LGTM! Exception class is well-defined.
The exception class follows Java conventions and provides appropriate error handling for manifest parsing issues.
34-34: Verify the assumption about ignoring scaleH.
The comment "XXX: really ignore scaleH?" suggests uncertainty about whether the height scale should be considered.
Let's check if other implementations consider scaleH:
✅ Verification successful
Let me gather more context about the font loading implementation.
Let me search for more context about the font loading implementation, particularly looking for any documentation or code that explains the usage of scaleH.
Let me search for the actual font loading implementation and any related code that might explain how the texture atlas dimensions are used.
Let me check the actual usage of the font texture atlas dimensions in the code to understand if scaleH is needed.
Based on the code analysis, I can now provide a definitive response about the scaleH usage:
The scaleH parameter can be safely ignored in this implementation.
The code consistently uses a square texture atlas where both width and height are set to scaleW. This is evident from:
- The FontJsonManifestReader creates a Vec2D with identical values:
new Vec2D(imageSize, imageSize) - The test font JSON shows both values are typically equal:
"scaleW": 512, "scaleH": 512 - The FontWrapper class stores the bitmap size as a Vec2D and all implementations use the same value for both dimensions
This approach simplifies texture atlas handling while being compatible with the common case where font atlases are square textures. The XXX comment can be removed as this is an intentional design choice.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of scaleH in font loading across the codebase
rg -i "scaleh" --type cpp --type java
Length of output: 213
Script:
#!/bin/bash
# Look for font loading related files and their content
fd -e java -e cpp -e h font -i --exec rg -l "class|struct" {} \; | xargs rg -i "font|glyph|scale" -C 2
# Also check for any documentation about font loading
fd README -i --exec cat {} \;
Length of output: 99524
Script:
#!/bin/bash
# Look for font loading related documentation and implementation details
rg -i "scaleh|font.*atlas|msdf" -C 3 --type md --type txt
# Also check for any font loading test files that might show expected behavior
fd -e test -e spec -i font --exec rg -i "scaleh|atlas" {} \;
Length of output: 125
Script:
#!/bin/bash
# Look for font loading implementation details
ast-grep --pattern $'class $_ {
$$$
extractFontData($_, $_) {
$$$
}
$$$
}'
# Also search for any texture atlas related code
rg -i "texture.*atlas|atlas.*texture|atlas.*size" -C 3
Length of output: 4807
Script:
#!/bin/bash
# Look for where the font texture dimensions are used
rg -i "fontWrapper|bitmapSize|scaleW|scaleH" -B 2 -A 2
# Also check for any font rendering code that might use these dimensions
ast-grep --pattern 'new FontWrapper($$$)'
Length of output: 32491
shared/src/logger/Logger.cpp (1)
106-106: Verify logging behavior across platforms
The new Linux-specific logging condition might affect the logging behavior on different platforms. Please ensure that:
- The logging still works correctly on Android when built with Linux toolchain
- The log levels are consistent across all platforms
✅ Verification successful
Platform-specific logging implementation is correctly handled
The code review concern can be resolved as the implementation shows:
- Android logging is properly isolated with
__ANDROID__macro and uses the correct__android_log_printfunction with appropriate priority mapping - Linux logging (non-Android) is correctly handled in the
defined(__linux__) && !defined(__ANDROID__)condition - Log levels are consistently mapped across all platforms:
- Android: Uses native priority levels (2-6)
- Apple/Windows: Uses os_log with appropriate severity levels
- Linux/Testing: Uses stdout with consistent ERROR/WARNING/DEBUG/INFO/TRACE levels
The condition defined(BANDIT_TESTING) || (defined(__linux__) && !defined(__ANDROID__)) ensures proper separation between Android and non-Android Linux builds.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential platform-specific logging implementations
echo "Searching for platform-specific logging code..."
rg -l "(android_log|os_log|OutputDebugString)" --type cpp
# Check for other log level definitions that might conflict
echo "Checking for log level definitions..."
rg "LOG_LEVEL|LogLevel" --type hpp --type cpp
Length of output: 354
Script:
#!/bin/bash
# Let's check the actual Logger.cpp implementation for platform-specific handling
echo "Checking Logger.cpp implementation:"
cat shared/src/logger/Logger.cpp
# Look for header files that might contain log level definitions
echo -e "\nSearching for logger header files:"
fd -e h -e hpp Logger
# Check for Android-specific logging code
echo -e "\nChecking for Android logging references:"
rg "android" -B2 -A2 shared/src/logger/Logger.cpp
# Look for platform-specific macros and conditions
echo -e "\nChecking for platform-specific conditions:"
rg "#if.*defined" shared/src/logger/Logger.cpp
Length of output: 5808
shared/src/map/layers/objects/QuadMaskObject.cpp (2)
13-13: LGTM: Necessary header inclusion for mathematical functions.
The addition of header is appropriate as it provides the trigonometric functions used in coordinate transformations.
Line range hint 65-86: Review needed: Coordinate transformation logic may be incorrect.
The refactored transformation logic has potential issues:
- The transform function no longer applies the origin offset:
auto transform = [&origin](const Coord coordinate) -> Vec3D {
double x = coordinate.x; // Should be coordinate.x - origin.x
double y = coordinate.y; // Should be coordinate.y - origin.y
double z = coordinate.z; // Should be coordinate.z - origin.z
return Vec3D(x, y, z);
};- The 3D transformation appears incomplete:
- Origin is calculated in spherical coordinates
- But transform function treats coordinates as Cartesian
- This mismatch could lead to incorrect 3D positioning
Consider applying this fix:
auto transform = [&origin](const Coord coordinate) -> Vec3D {
- double x = coordinate.x;
- double y = coordinate.y;
- double z = coordinate.z;
+ if (is3D) {
+ double x = 1.0 * sin(coordinate.y) * cos(coordinate.x) - origin.x;
+ double y = 1.0 * cos(coordinate.y) - origin.y;
+ double z = -1.0 * sin(coordinate.y) * sin(coordinate.x) - origin.z;
+ return Vec3D(x, y, z);
+ } else {
+ double x = coordinate.x - origin.x;
+ double y = coordinate.y - origin.y;
+ double z = coordinate.z - origin.z;
+ return Vec3D(x, y, z);
+ }
- return Vec3D(x, y, z);
};Let's verify if this transformation is used elsewhere in the codebase:
shared/src/map/layers/objects/PolygonGroup2dLayerObject.cpp (2)
14-14: LGTM: Necessary header inclusion for trigonometric functions.
The addition of <cmath> header is appropriate for the trigonometric functions used in the 3D coordinate transformations.
Line range hint 85-96: Verify numerical precision for large coordinate values.
The current implementation might face precision issues when dealing with large coordinate values during averaging and adjustment operations.
#!/bin/bash
# Search for test cases that might verify coordinate precision
rg -l "PolygonGroup2dLayerObject.*test"
# Look for existing coordinate range definitions
rg "coordinate.*range|bounds" Consider using double-precision operations consistently and adding test cases for large coordinate values.
shared/src/map/layers/objects/PolygonPatternGroup2dLayerObject.cpp (2)
16-16: LGTM: Appropriate header inclusion for mathematical operations.
The addition of <cmath> header is necessary for the trigonometric functions used in coordinate transformations.
Line range hint 108-110: Verify buffer sizes in SharedBytes construction.
The SharedBytes construction with raw pointers should include size validation to prevent buffer overflows.
shared/public/Vec2DHelper.h (1)
Line range hint 1-17: LGTM! Includes are appropriate and well-organized.
The necessary headers are included for vector operations, geometric computations, and standard algorithms.
jvm/src/test/java/io/openmobilemaps/mapscore/OffscreenMapRendererTest.java (1)
130-149:
Fix text rendering issue instead of using workaround.
The test contains a workaround for a text rendering issue (XXX comment). This should be properly fixed in the renderer.
Let's verify if this is a known issue:
shared/src/map/layers/text/TextLayer.cpp (1)
223-225: Verify error handling across Java-C++ boundary.
Since this is part of Java bindings for offscreen rendering, ensure that font loading failures are properly handled and reported across the Java-C++ boundary.
Let's check for error handling in the Java bindings:
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Font loading errors are properly handled across Java-C++ boundary
The error handling for font loading failures is well-implemented across the language boundary:
- Java side (
FontLoader.java) properly handles and propagates errors throughFontLoaderResultwith appropriate status codes:- File not found:
LoaderStatus.ERROR_404 - Invalid manifest:
LoaderStatus.ERROR_OTHER - General IO errors:
LoaderStatus.ERROR_OTHER
- File not found:
- C++ side (
TextLayer.cpp) correctly handles null font data by skipping the iteration - A fallback mechanism exists in
FontLoader.loadFont()that attempts to load a fallback font when the primary font fails - Error states are logged for debugging via the logger
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for font loading error handling in Java code
echo "Checking Java code for font loading error handling..."
rg -t java "FontLoader|loadFont" -A 5 -B 5
# Search for error propagation mechanisms
echo "Checking for error propagation mechanisms..."
rg -t java "throws|Exception|Error.*font" -A 2
Length of output: 39421
shared/src/map/layers/tiled/vector/Tiled2dMapVectorLayerParserHelper.cpp (3)
Line range hint 1-500: Enhance error handling consistency throughout the parser
The error handling across different JSON parsing operations is inconsistent. Some errors return immediately while others continue processing, which could lead to undefined behavior.
Consider implementing a consistent error handling strategy:
- Create an error collection mechanism:
struct ParsingError {
std::string context;
std::string message;
LoaderStatus status;
std::optional<std::string> errorCode;
};
class ErrorCollector {
std::vector<ParsingError> errors;
public:
void addError(const ParsingError& error);
bool hasErrors() const;
ParsingError getMostSevere() const;
};- Update the return type to include all errors:
struct Tiled2dMapVectorLayerParserResult {
std::shared_ptr<VectorMapDescription> description;
LoaderStatus status;
std::optional<std::string> errorCode;
std::optional<std::string> metadata;
std::vector<ParsingError> errors; // Add this field
};This would help in:
- Collecting all validation errors before failing
- Providing better error context to Java clients
- Maintaining consistency in error handling
Let's check for existing error handling patterns:
Line range hint 1-500: Consider platform-specific requirements for Java bindings
Given that this code will be called from Java in non-Android environments, there are several platform-specific considerations to address.
Recommendations:
-
Memory Management:
- Ensure all C++ exceptions are caught before crossing JNI boundaries
- Consider using RAII patterns for resource cleanup
- Document memory ownership rules for Java developers
-
Error Propagation:
- Map C++ errors to meaningful Java exceptions
- Ensure error messages are properly encoded for Java String conversion
- Consider adding debug logging for troubleshooting
-
Platform Compatibility:
- Document platform-specific requirements (e.g., OSMesa dependencies)
- Consider adding platform checks in critical sections
- Ensure file paths work correctly across platforms
Let's check for platform-specific considerations:
206-208:
Critical: Add proper validation to prevent segfaults in GeoJSON parsing
The XXX comments indicate several scenarios where the parser can crash. This is particularly concerning for Java bindings where native crashes can bring down the JVM. We should implement proper validation and error handling.
Consider implementing these safety checks:
- // XXX: segfault on invalid data
- // XXX: fails on "type": "Feature" instead of "FeatureCollection"
- // XXX: fails if there is no "properties"
- geojsonSources[key] = GeoJsonVTFactory::getGeoJsonVt(GeoJsonParser::getGeoJson(val["data"]), options);
+ try {
+ auto& data = val["data"];
+ // Validate GeoJSON structure
+ if (!data.is_object()) {
+ throw std::runtime_error("Invalid GeoJSON: root must be an object");
+ }
+
+ // Validate type
+ if (!data["type"].is_string()) {
+ throw std::runtime_error("Invalid GeoJSON: missing or invalid 'type' field");
+ }
+
+ // Handle both FeatureCollection and Feature types
+ const auto& type = data["type"].get<std::string>();
+ if (type != "FeatureCollection" && type != "Feature") {
+ throw std::runtime_error("Invalid GeoJSON: 'type' must be 'FeatureCollection' or 'Feature'");
+ }
+
+ // Ensure properties exist if required
+ if (type == "Feature" && (!data.contains("properties") || data["properties"].is_null())) {
+ throw std::runtime_error("Invalid GeoJSON: Feature missing 'properties'");
+ }
+
+ geojsonSources[key] = GeoJsonVTFactory::getGeoJsonVt(GeoJsonParser::getGeoJson(data), options);
+ } catch (const std::exception& e) {
+ LogError <<= "Failed to parse GeoJSON: " + std::string(e.what());
+ continue;
+ }Let's verify if there are any existing GeoJSON validation utilities in the codebase:
jvm/src/main/java/io/openmobilemaps/mapscore/graphics/util/OSMesa.java (1)
41-42: Verify Java version compatibility for var usage
The use of var for local variable type inference requires Java 10 or newer. Please ensure that the project targets a Java version that supports this feature. Using var may cause compatibility issues if the project needs to support earlier Java versions.
If the project must support Java versions prior to 10, replace var with explicit type declarations:
-var colorModel = image.getColorModel();
+ColorModel colorModel = image.getColorModel();
-var raster = image.getRaster();
+WritableRaster raster = image.getRaster();
-var out = new BufferedImage(width, height, BufferedImage.TYPE_INT_ARGB);
+BufferedImage out = new BufferedImage(width, height, BufferedImage.TYPE_INT_ARGB);
-var tmpARGB = new int[width * height];
+int[] tmpARGB = new int[width * height];Also applies to: 79-82
jvm/src/main/java/io/openmobilemaps/mapscore/map/util/MapTileRenderer.java (1)
97-98: Investigate potential issues with tile border rendering
The TODO comment raises concerns about the safety of calling update() on the camera and layer for each tile separately, which could cause issues at tile borders. It's important to verify whether this approach is safe or if modifications are needed to prevent rendering artifacts along tile edges.
Would you like assistance in investigating this potential issue or proposing a solution to ensure seamless tile border rendering?
jvm/src/main/cpp/jni/OSMesa.cpp (7)
1-8: LGTM!
The header includes are appropriate and necessary for the functionality provided in this file.
9-40: Excellent error handling!
The glCheckError_ function provides detailed error messages for various OpenGL error codes, which will be helpful for debugging. The use of a macro to automatically capture the file and line number is a nice touch.
42-71: Great framebuffer status checking!
The checkFramebufferStatus function thoroughly checks the status of the framebuffer and provides informative error messages for different incomplete states. This will aid in identifying issues related to framebuffer setup.
117-150: Excellent handling of multisampling and framebuffer setup!
The Java_io_openmobilemaps_mapscore_graphics_util_OSMesa_makeCurrent function properly manages the framebuffer and renderbuffer objects based on the numSamples argument. The code correctly sets up the multisampled framebuffer and falls back to the default framebuffer when multisampling is not requested. The error checking using checkFramebufferStatus ensures the framebuffer is complete before proceeding.
179-189: Properly cleans up resources.
The Java_io_openmobilemaps_mapscore_graphics_util_OSMesa_destroy function correctly deletes the framebuffers and renderbuffers, destroys the OSMesa context, and frees the allocated buffer. This ensures proper cleanup of resources when the context is no longer needed.
88-115: Verify OSMesa context creation across different versions.
The code handles different OSMesa versions by using appropriate functions and attributes for context creation. However, it's important to thoroughly test this functionality across the supported versions to ensure consistent behavior.
To verify the OSMesa context creation:
#!/bin/bash
# Description: Verify OSMesa context creation across different versions.
# Test: Search for the version checks and context creation calls.
# Expect: Code handles at least 3 different version ranges.
rg -A 5 $'OSMesaCreateContext(Attribs|Ext)?\(' 1-190: Verify the usage of the JNI functions in the Java code.
The C++ code defines the JNI functions, but it's crucial to ensure that these functions are correctly called from the Java side. Review the Java code (e.g., OSMesa.java) to confirm that the JNI functions are invoked with the appropriate arguments and that the returned values are handled correctly.
To verify the usage of the JNI functions in the Java code:
✅ Verification successful
JNI functions are correctly implemented and used in the Java code
The Java code in OSMesa.java properly declares and uses all the native methods that correspond to the C++ JNI implementations:
createContext()is called in the constructormakeCurrent(long state, int width, int height, int numSamples)is called with correct argument typesreadARGB(long state, int[] out)is used ingetImage()with proper array handlingdestroy(long state)is called in the cleanup method
The class also properly manages the native state pointer and includes appropriate error handling. The OffscreenMapRenderer demonstrates correct usage of the OSMesa class by properly initializing it with width, height, and sample parameters.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of the JNI functions in the Java code.
# Test: Search for the Java class and its native method declarations.
# Expect: Java class with native methods corresponding to the C++ JNI functions.
rg -A 10 $'io\.openmobilemaps\.mapscore\.graphics\.util\.OSMesa' $(fd -e java$)
# Test: Search for the invocations of the native methods in the Java code.
# Expect: Native methods invoked with the appropriate arguments.
rg -A 5 $'OSMesa\.(createContext|makeCurrent|readARGB|destroy)\(' $(fd -e java$)
Length of output: 1489
Script:
#!/bin/bash
# Let's try a different approach to find the Java class and its usages
# Search for Java files containing 'OSMesa'
fd -e java . | xargs rg -l "OSMesa"
# Search for native method declarations in Java files
fd -e java . | xargs rg "native.*OSMesa"
# Search for any class that might be using OSMesa
fd -e java . | xargs rg "class.*OSMesa"
Length of output: 487
Script:
#!/bin/bash
# Let's examine the content of the Java files we found
# Check the OSMesa.java class implementation
cat jvm/src/main/java/io/openmobilemaps/mapscore/graphics/util/OSMesa.java
# Check how OSMesa is used in OffscreenMapRenderer
rg -A 5 "OSMesa" jvm/src/main/java/io/openmobilemaps/mapscore/map/util/OffscreenMapRenderer.java
Length of output: 4244
jvm/src/main/java/io/openmobilemaps/mapscore/map/util/OffscreenMapRenderer.java (3)
1-196: Comprehensive and well-structured implementation!
The OffscreenMapRenderer class provides a robust solution for offscreen map rendering using OpenGL and OSMesa. The code is well-organized, with clear separation of concerns and appropriate error handling. The class encapsulates the rendering logic effectively, managing the lifecycle of the map interface and graphics context.
Key highlights:
- Constructors initialize the renderer with specified dimensions and configurations.
- Methods for setting the framebuffer size, destroying the rendering context, and drawing frames are provided.
- The
drawFramemethod handles layer updates and readiness checks with a timeout mechanism. - Error handling is implemented through a custom
MapLayerException. - Control flow is managed through various checks and loops to ensure rendering tasks are executed on the correct thread.
- The class is thoroughly tested in
OffscreenMapRendererTest.java.
Overall, the implementation is comprehensive, maintainable, and follows best practices.
72-98: Verify the necessity of disabling animations and multiple layer updates.
The drawFrame method includes logic to disable animations for each layer and perform multiple updates before the actual drawing. While the comments mention a workaround for text icon fade-in, it's important to consider the following:
- Investigate the root cause of the text icon fade-in issue and determine if there's a more appropriate fix that doesn't require disabling animations and multiple updates.
- Assess the impact of disabling animations on the overall rendering performance and user experience.
- Consider if the multiple layer updates are necessary for all use cases or if they can be optimized based on specific requirements.
If the current approach is indeed the best solution, ensure that it is well-documented and consider adding comments explaining the rationale behind the workaround.
100-108: Use the forceDrawFrame method with caution.
The forceDrawFrame method is intended to handle scenarios where map layers may get stuck but are actually ready to render. However, as mentioned in the comment, using this method comes with risks. Consider the following:
- Clearly document the potential consequences of using
forceDrawFrame, such as inconsistent rendering or unexpected behavior. - Provide guidance on when it is appropriate to use this method and when to rely on the regular
drawFramemethod. - Consider implementing additional safeguards or checks within
forceDrawFrameto minimize the chances of rendering issues.
It's important to ensure that users of this class are aware of the implications of using forceDrawFrame and use it judiciously.
jvm/src/main/java/io/openmobilemaps/mapscore/map/util/Tiled2dMapVectorLayerBuilder.java (3)
80-94: Prevent Setting Both styleJsonData and styleJsonURL
The methods withStyleJsonData and withStyleJsonURL correctly throw an IllegalArgumentException if both styleJsonData and styleJsonURL are set. However, this check is only performed when setting the second value.
To ensure robustness, add checks in both methods to prevent this issue regardless of the order in which the methods are called.
public Tiled2dMapVectorLayerBuilder withStyleJsonData(String styleJson) {
if (styleJson != null && this.styleJsonURL != null) {
throw new IllegalArgumentException("Cannot set both style data and style URL");
}
this.styleJsonData = styleJson;
return this;
}
public Tiled2dMapVectorLayerBuilder withStyleJsonURL(String styleJsonURL) {
if (styleJsonURL != null && this.styleJsonData != null) {
throw new IllegalArgumentException("Cannot set both style URL and style data");
}
this.styleJsonURL = styleJsonURL;
return this;
}96-102: Handle Possible Null fontFallbackName
In the withFontLoader method, fontFallbackName is annotated with @Nullable, but it's not clear how the FontLoader handles a null value for this parameter.
Ensure that the FontLoader class can handle a null fontFallbackName without throwing exceptions.
163-172: Handle Null BufferedImage in wrapTextureLoaderResult
In the wrapTextureLoaderResult method, if image is null, a TextureLoaderResult is created with null image holder and a status of LoaderStatus.ERROR_404. Ensure that this behavior is appropriate for the calling code.
Confirm that the rest of the system can handle a TextureLoaderResult with a null image and an error status without issues.
standalone/main.cpp (1)
310-329: Prevent potential infinite loop in awaitReadyToRenderOffscreen.
The loop may run indefinitely if the layers never reach a ready state.
Implement a timeout mechanism:
static void awaitReadyToRenderOffscreen(std::shared_ptr<MapInterface> map) {
bool unready = true;
+ int maxRetries = 1000;
+ int retryCount = 0;
while (unready) {
while (map->getScheduler()->runGraphicsTasks()) {
};
unready = std::ranges::any_of(map->getLayers(), [](std::shared_ptr<LayerInterface> &layer) -> bool {
return layer->isReadyToRenderOffscreen() == LayerReadyState::NOT_READY;
});
+
+ if (++retryCount >= maxRetries) {
+ std::cerr << "Timeout waiting for layers to be ready." << std::endl;
+ break;
+ }
}
jvm/src/main/java/io/openmobilemaps/mapscore/graphics/util/GlTextureHelper.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
shared/src/graphics/SceneInterface.cpp (1)
Line range hint
11-17: Good architectural improvement moving from platform to feature-based conditional compilation!The change from
__linux__toOPENMOBILEMAPS_GLis a significant improvement as it:
- Decouples the OpenGL implementation from specific platforms
- Makes the code more maintainable and portable
- Aligns with the PR's goal of supporting non-Android Java environments
Consider documenting the
OPENMOBILEMAPS_GLmacro in the project's build documentation to help future contributors understand when and how to use it.shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonTile.h (1)
Line range hint
58-62: Good architectural improvement moving from platform to feature-specific conditioningThe change from
__ANDROID__toOPENMOBILEMAPS_GLbetter reflects the actual dependency on OpenGL capabilities rather than the platform. This aligns well with the PR's objective of supporting non-Android Java environments and using explicit defines for OpenGL entrypoints.shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonPatternTile.h (1)
Line range hint
64-68: Document the rationale for different style limitsThe change from
__ANDROID__toOPENMOBILEMAPS_GLaligns with the PR's objective of supporting OpenGL-based rendering. However, please add a comment explaining why OpenGL implementation requires a lower limit (16 vs 256 styles per group) to help future maintainers understand the technical constraints.#ifdef OPENMOBILEMAPS_GL + // OpenGL implementation has a lower limit due to shader uniform constraints static const int maxStylesPerGroup = 16; #else + // Non-OpenGL implementations can handle more styles per group static const int maxStylesPerGroup = 256; #endifshared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSourceSymbolDataManager.h (1)
Line range hint
153-158: Enhance documentation for the feature group limit.The code change looks good, replacing Android-specific directive with OpenGL-specific one. However, the comment could be more descriptive.
Consider expanding the comment to better explain the technical constraints:
#ifdef OPENMOBILEMAPS_GL - // Higher counts may cause issues for instanced text rendering + // Limit set to 3500 features per group to prevent performance degradation + // and potential instability in OpenGL instanced text rendering. This value + // was determined through testing with OSMesa offscreen rendering. int32_t maxNumFeaturesPerGroup = 3500;android/src/main/cpp/graphics/shader/ColorLineGroup2dShaderOpenGl.cpp (2)
14-14: Good addition of safety checks!The assertions effectively prevent buffer overflows by validating both the number of styles and the total data size. Consider enhancing this further with runtime checks for production builds where assertions might be disabled.
Consider adding runtime checks that throw exceptions for production builds:
+ if (styles.elementCount > maxNumStyles) { + throw std::invalid_argument("styles.elementCount exceeds maxNumStyles"); + } + if (styles.elementCount * styles.bytesPerElement > lineValues.size() * sizeof(float)) { + throw std::invalid_argument("styles data size exceeds buffer capacity"); + } assert(styles.elementCount <= maxNumStyles); assert(styles.elementCount * styles.bytesPerElement <= lineValues.size() * sizeof(float));Also applies to: 64-65
Line range hint
148-151: Consider adding shader-side bounds checkingThe vertex shader accesses lineValues array using computed indices. While the C++ assertions protect against buffer overflows during data loading, adding runtime bounds checking in the shader would provide an additional safety layer.
Consider modifying the shader code to add explicit bounds checking:
int lineIndex = int(floor(fStyleIndex + 0.5)); if (lineIndex < 0) { lineIndex = 0; - } else if (lineIndex > numStyles) { + } else if (lineIndex >= numStyles) { lineIndex = numStyles - 1; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
CMakeLists.txt(1 hunks)android/src/main/cpp/graphics/shader/ColorLineGroup2dShaderOpenGl.cpp(2 hunks)shared/src/graphics/SceneInterface.cpp(2 hunks)shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSourceSymbolDataManager.h(1 hunks)shared/src/map/layers/tiled/vector/tiles/line/Tiled2dMapVectorLineTile.h(1 hunks)shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonPatternTile.h(1 hunks)shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonTile.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CMakeLists.txt
🔇 Additional comments (7)
shared/src/graphics/SceneInterface.cpp (2)
Line range hint 28-34: Verify Scene inheritance from SceneInterface
The implementation looks correct, but let's verify the inheritance relationship to ensure the safety of the static_pointer_cast.
Also, consider adding a runtime assertion or using dynamic_pointer_cast in debug builds to catch potential inheritance issues early.
✅ Verification successful
static_pointer_cast usage is safe - Scene properly inherits from SceneInterface
The Scene class is explicitly declared with public inheritance from SceneInterface (class Scene : public SceneInterface), and all virtual methods are properly overridden. The static_pointer_cast usage in SceneInterface.cpp is therefore type-safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Scene inherits from SceneInterface to ensure safe static_pointer_cast
# Search for Scene class declaration
ast-grep --pattern 'class Scene : $$$SceneInterface$$$'
# Alternative search using ripgrep for header file
rg -l "class\s+Scene\s*:\s*(?:public\s+)?SceneInterface" --type cpp
Length of output: 155
Script:
#!/bin/bash
# Let's verify the actual inheritance declaration in Scene.h
cat shared/src/graphics/Scene.h
Length of output: 1953
Line range hint 1-34: Verify OSMesa integration with OpenGL context
Since this PR aims to support offscreen rendering through OSMesa, let's verify the OpenGL context initialization.
✅ Verification successful
Based on the analysis of the codebase, I can now provide a final response regarding the OSMesa integration verification:
OSMesa integration is properly handled through separate implementations
The codebase shows that OSMesa integration is handled through dedicated implementations in the standalone and JNI paths, rather than being integrated into the OpenGlContext class. This is the correct approach as it:
- Maintains clean separation between platform-specific (Android OpenGL) and offscreen (OSMesa) rendering contexts
- Allows for proper context initialization based on the runtime environment
- Preserves the existing OpenGlContext implementation for Android platform use
The OSMesa context initialization and management is properly implemented in:
standalone/main.cppfor command-line usagejvm/src/main/cpp/jni/OSMesa.cppfor Java/JNI integration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify OSMesa integration in OpenGlContext
# Search for OSMesa initialization in OpenGlContext
rg -A 10 "OSMesa" --type cpp
# Check if OSMesa headers are properly included
ast-grep --pattern '#include $$$OSMesa$$$'
Length of output: 9737
Script:
#!/bin/bash
# Let's verify if OpenGlContext properly handles OSMesa integration
rg -A 10 "OpenGlContext" --type cpp
# Check if there are any OpenGL context initialization files
fd -e h -e hpp -e cpp "Context" --exec rg -l "OSMesa|OpenGL"
Length of output: 83626
Script:
#!/bin/bash
# Let's check the OpenGlContext header and implementation for OSMesa integration
rg -A 10 "class OpenGlContext" --type cpp
# Also check for any OSMesa-specific context implementations
fd -e h -e cpp "Context" --exec rg -l "OSMesa.*Context"
Length of output: 915
shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonTile.h (1)
Line range hint 58-62: Verify the performance impact of reduced styles per group in OpenGL mode
The significant reduction in maxStylesPerGroup from 256 to 16 for OpenGL builds could impact rendering performance or memory usage. Please verify:
- The rationale behind the 16-style limit for OpenGL
- Any potential performance implications
- Whether this limit has been tested with complex polygon styles
✅ Verification successful
Let me gather more information about OpenGL-specific implementation and any performance considerations.
Based on the gathered information, I can now provide a final response:
OpenGL style group limits are intentionally restricted for performance optimization
The reduced style group limits in OpenGL mode are part of a broader pattern of OpenGL-specific optimizations:
- Polygon tiles: 16 styles per group (vs 256)
- Line tiles: 32 styles per group (vs 256)
- Symbol rendering: 3500 features per group (vs unlimited)
These limits appear to be carefully chosen constraints for OpenGL implementations to optimize batch rendering and reduce draw calls. The comment "Higher counts may cause issues for instanced text rendering" in the symbol manager suggests these limits are based on practical performance considerations rather than arbitrary restrictions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any performance-related comments or tests regarding style group limits
rg -A 5 "maxStylesPerGroup|style.*group.*limit"
# Look for related test cases
ast-grep --pattern 'class $_(Test|Tests) {
$$$
test$_($$$) {
$$$
maxStylesPerGroup
$$$
}
$$$
}'
Length of output: 6779
Script:
#!/bin/bash
# Search for OpenGL-specific shader implementations and batch rendering logic
rg -A 10 "class.*PolygonGroupShaderInterface|class.*LineGroupShaderInterface"
# Look for any performance-related comments or documentation
rg -i "performance|batch.*render|draw.*call" shared/src/map/layers/tiled/vector/
# Search for any GL-specific implementation details
rg "OPENMOBILEMAPS_GL" -A 5
Length of output: 18769
shared/src/map/layers/tiled/vector/tiles/line/Tiled2dMapVectorLineTile.h (1)
Line range hint 54-58: Platform-specific constant change looks good, but needs verification.
The switch from __ANDROID__ to OPENMOBILEMAPS_GL aligns well with the PR's objective of supporting offscreen rendering through OSMesa in non-Android environments. However, a few points to consider:
- Please verify that 32 styles per group are sufficient for OpenGL implementations, particularly for complex map scenarios.
- Consider adding a comment explaining why OpenGL requires a more constrained limit compared to other platforms.
Let's verify the style usage across the codebase:
✅ Verification successful
Based on the search results, I can now generate the final response:
The 32 styles per group limit for OpenGL is appropriate and well-supported by the codebase.
The codebase shows consistent style group size management across different vector tile types:
- Lines: 32 styles for OpenGL vs 256 for others
- Polygons: 16 styles for OpenGL vs 256 for others
- Pattern Polygons: 16 styles for OpenGL vs 256 for others
The implementation in ColorLineGroup2dShaderOpenGl and LineGroup2dOpenGl shows that styles are managed efficiently with proper buffer management and OpenGL state handling. The line styles are processed in groups to optimize rendering performance while staying within hardware capabilities.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential style group size issues
# Look for places where multiple styles are created/used to ensure 32 is sufficient
# Search for style group creation patterns
rg -A 5 "ShaderLineStyle|LineGroup2d|styleHashToGroupMap"
# Look for any existing comments or constants related to style limits
rg "maxStylesPerGroup|MAX_STYLES"
Length of output: 98697
shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonPatternTile.h (1)
Line range hint 64-68: Verify style limits across platforms
Since this PR introduces OpenGL support via OSMesa for Linux/amd64, we should verify that these style limits are appropriate across different platforms and rendering contexts.
✅ Verification successful
Based on the search results, I can now generate a final response as we have found clear evidence of the style limits across different components:
Style limits are consistently defined but vary by component type in OpenGL mode
The codebase shows a clear pattern of different style group limits between OpenGL and non-OpenGL modes:
- Line tiles: 32 styles (GL) vs 256 styles (non-GL)
- Polygon tiles: 16 styles (GL) vs 256 styles (non-GL)
- Polygon pattern tiles: 16 styles (GL) vs 256 styles (non-GL)
These variations appear intentional, with polygon-based tiles having more restrictive limits in OpenGL mode compared to line tiles, likely due to shader uniform limitations and rendering complexity differences between these geometric types.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for other style limit definitions and related shader code
# to ensure consistency across the codebase
echo "Searching for other maxStylesPerGroup definitions..."
rg "maxStylesPerGroup" -A 2 -B 2
echo "Searching for shader uniform declarations that might be affected..."
rg "uniform\s+\w+\s+\w+\[\d+\]" --type glsl
echo "Checking for related OpenGL shader code..."
ast-grep --pattern 'uniform $type $name[$size];'
Length of output: 5740
shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSourceSymbolDataManager.h (1)
Line range hint 153-158: Verify the feature group limit impact.
Let's verify how this constant is used across the codebase to ensure the limit is appropriate for all use cases.
✅ Verification successful
Let me generate additional scripts to better understand the context and usage of this feature grouping mechanism.
Based on the gathered information, I can now provide a final response:
The feature group limit of 3500 for GL-based text rendering is appropriate.
The code analysis reveals that:
- The limit is only applied when OpenGL rendering is enabled (
OPENMOBILEMAPS_GL) - The feature grouping is used in
Tiled2dMapVectorSourceSymbolDataManagerto batch symbol processing - The implementation shows careful handling of group sizes with proper chunking logic
- There's a vertex index check against
uint16_t::max()in the symbol group implementation, suggesting the limit also aligns with GPU buffer constraints
The 3500 limit appears to be a reasonable balance for instanced text rendering in OpenGL, while no limit is applied for other rendering backends.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usage of maxNumFeaturesPerGroup to understand its impact
# Find direct references to maxNumFeaturesPerGroup
echo "=== Direct references to maxNumFeaturesPerGroup ==="
rg -A 3 "maxNumFeaturesPerGroup"
# Find related feature group size checks
echo "=== Related feature group size checks ==="
ast-grep --pattern 'if ($expr.size() > $_) { $$$ }'
# Look for performance-related comments
echo "=== Performance-related comments about feature groups ==="
rg -i "feature.*group.*performance|performance.*feature.*group"
Length of output: 2556
Script:
#!/bin/bash
# Search for the Tiled2dMapVectorSymbolGroup class implementation and usage
echo "=== Tiled2dMapVectorSymbolGroup implementation ==="
rg -A 5 "class Tiled2dMapVectorSymbolGroup"
# Look for any comments or documentation about the feature grouping
echo "=== Documentation about feature grouping ==="
rg -i "feature.*group" "shared/src/map/layers/tiled/vector/symbol/"
# Check if there are any GL-specific implementations or comments
echo "=== GL-specific implementations ==="
rg -i "gl.*text.*render|text.*render.*gl" "shared/src/map/layers/tiled/vector/"
# Look for any performance or memory related comments in the symbol rendering code
echo "=== Performance related comments in symbol rendering ==="
rg -i "performance|memory|limit" "shared/src/map/layers/tiled/vector/symbol/"
Length of output: 5364
android/src/main/cpp/graphics/shader/ColorLineGroup2dShaderOpenGl.cpp (1)
Line range hint 83-84: Verify OSMesa compatibility with GLES 3.2 features
Given that this PR aims to support offscreen rendering through OSMesa, we should verify that all GLES 3.2 features used in the shader are supported by the target OSMesa version.
matzf
force-pushed
the
feature/jvm-lib
branch
from
de23a81 to
bd99dfe
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
android/CMakeLists.txt (1)
Line range hint
141-150: Consider platform-specific linking strategyThe current linking setup uses GLESv3 for Android. Given that the PR introduces OSMesa support:
- Consider implementing a more flexible linking strategy that can handle both GLESv3 and OSMesa based on the target platform
- Document the platform-specific requirements and limitations
Consider restructuring the linking logic to support multiple OpenGL implementations:
if(ANDROID) find_library(opengl-lib GLESv3) target_link_libraries(mapscore ${opengl-lib}) elseif(UNIX AND NOT APPLE) find_library(osmesa-lib OSMesa) target_link_libraries(mapscore ${osmesa-lib}) endif()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
CMakeLists.txt(1 hunks)android/CMakeLists.txt(1 hunks)android/src/main/cpp/graphics/shader/ColorLineGroup2dShaderOpenGl.cpp(2 hunks)shared/src/graphics/SceneInterface.cpp(2 hunks)shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSourceSymbolDataManager.h(1 hunks)shared/src/map/layers/tiled/vector/tiles/line/Tiled2dMapVectorLineTile.h(1 hunks)shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonPatternTile.h(1 hunks)shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonTile.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonTile.h
- android/src/main/cpp/graphics/shader/ColorLineGroup2dShaderOpenGl.cpp
- shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonPatternTile.h
- shared/src/map/layers/tiled/vector/tiles/line/Tiled2dMapVectorLineTile.h
- shared/src/graphics/SceneInterface.cpp
- shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSourceSymbolDataManager.h
- CMakeLists.txt
🔇 Additional comments (1)
android/CMakeLists.txt (1)
117-117: Verify OpenGL implementation selection
The addition of OPENMOBILEMAPS_GL=1 needs careful consideration:
- This Android build links against GLESv3, while the PR introduces OSMesa support for Linux. Ensure this define doesn't cause conflicts between different OpenGL implementations.
- The PR mentions needing explicit defines for OpenGL entrypoints instead of platform-specific preprocessor directives.
Let's verify the OpenGL implementation selection:
✅ Verification successful
Based on the results, I can now generate the final response:
OPENMOBILEMAPS_GL define is used correctly for platform-specific optimizations
The OPENMOBILEMAPS_GL define is used consistently across the codebase to:
- Select OpenGL-specific graphics implementation (GraphicsObjectFactoryOpenGl, ShaderFactoryOpenGl)
- Apply platform-specific optimizations for rendering:
- Adjusted limits for instanced text rendering
- Reduced maximum styles per group for vector lines (32 vs 256)
- Reduced maximum styles per group for vector polygons (16 vs 256)
These adjustments appear to be intentional optimizations for OpenGL-based platforms, and the define doesn't interfere with the OSMesa implementation which would be controlled by different compilation flags.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how OPENMOBILEMAPS_GL is used across the codebase
rg -A 5 "OPENMOBILEMAPS_GL"
# Check for any platform-specific OpenGL initialization
ast-grep --pattern 'if ($PLATFORM_CHECK) {
$$$
gl$$$
$$$
}'
Length of output: 4586
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (12)
.github/workflows/jvm_build.yml (1)
7-15: Consider adding caching strategy for dependencies.The build job could benefit from caching Maven dependencies and APT packages to speed up subsequent builds.
Add caching configuration to the external workflow parameters:
with: ref_name: ${{ github.ref_name }} checkout_submodules: recursive parent_pom: jvm/pom.xml install_apt_packages: "cmake make clang libgl-dev libgles-dev libosmesa6-dev" use_mvn_central: true + enable_caching: truejvm/readme.md (3)
26-27: Enhance APT installation command security.Consider pinning package versions and using
-yflag for non-interactive installation.-apt install --no-install-recommends maven cmake make clang libgl-dev libgles-dev libosmesa6-dev openjdk-21-jdk-headless +apt install -y --no-install-recommends \ + maven=3.9.* \ + cmake=3.25.* \ + make=4.3.* \ + clang=1:14.* \ + libgl-dev=1.6.* \ + libgles-dev=1.6.* \ + libosmesa6-dev=22.3.* \ + openjdk-21-jdk-headless=21.*
25-27: Add language specifiers to code blocks.Add language specifiers to fenced code blocks for better syntax highlighting.
-``` +```bash apt install --no-install-recommends maven cmake make clang libgl-dev libgles-dev libosmesa6-dev openjdk-21-jdk-headlessAlso applies to: 35-38, 45-48, 52-54 <details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint (0.35.0)</summary> 25-25: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> --- `3-5`: **Maintain consistent terminology.** Use consistent terminology throughout the documentation: either "offscreen" or "off-screen", but not both. ```diff -This directory contains a java library to use the OpenGL-variant of the -openmobilemaps in a non-android Java environment. -For now, this only explicitly supports offscreen rendering via +This directory contains a Java library to use the OpenGL-variant of +OpenMobileMaps in a non-Android Java environment. +For now, this only explicitly supports off-screen rendering via🧰 Tools
🪛 LanguageTool
[uncategorized] ~5-~5: Do not mix variants of the same word (‘offscreen’ and ‘off-screen’) within a single text.
Context: ... For now, this only explicitly supports offscreen rendering via [OSMesa](https://docs.mes...(EN_WORD_COHERENCY)
jvm/pom.xml (2)
111-124: Consider platform-specific native library handling.The current configuration assumes
.sofiles, but future platform support might require handling.dllor.dylibfiles.Consider using a more flexible configuration:
<resource> <directory>${project.build.directory}/build-native-release/jvm</directory> <includes> - <include>libmapscore_jni_*.so</include> + <include>libmapscore_jni_*${native.library.suffix}</include> </includes> <targetPath>native/</targetPath> </resource>Add a property to define the platform-specific suffix:
<properties> <native.library.suffix>${os.detected.classifier}.so</native.library.suffix> </properties>
53-56: Document the Ninja generator issue.The comment about the Ninja generator issue could be more informative.
- <!-- - Disabled, bug in cmake-maven-plugin; creates bad commandline -G"Ninja" (bad quoting). Set generator to ninja (usually faster than make). - <generator>Ninja</generator> - --> + <!-- + TODO: Enable Ninja generator once https://github.com/cmake-maven-project/cmake-maven-project/issues/XXX is fixed + Ninja is preferred as it's usually faster than Make, but currently disabled due to command-line quoting issues. + <generator>Ninja</generator> + -->jvm/src/main/cpp/jni/GlTextureHelper.cpp (3)
1-7: Add header guards and error handling utilities.Consider the following improvements:
- Add header guards to prevent multiple inclusion
- Add a header for error handling utilities
+#ifndef GLHELPER_H +#define GLHELPER_H #include <GL/gl.h> #include <cstdlib> #include <cstring> #include <iostream> #include <jni.h> +#include "GlError.h" // Add your error handling utilities extern "C" {
24-24: Address or remove the XXX comment.The comment questions the texture orientation but doesn't provide context. Either document the expected behavior or remove the comment if it's no longer relevant.
23-25: Enhance documentation of byte-order handling.The current comment about byte-order conversion could be more detailed. Consider documenting:
- The exact byte layout in memory
- Why BGRA format is chosen
- Platform-specific considerations
- // NOTE: little-endian! Implicit byte-order conversion from ARGB ints (highest-order byte is A) into BGRA byte buffer - // XXX: why isnt this upside down? + // Byte order handling: + // - Input: Java ARGB integers (0xAARRGGBB format) + // - OpenGL expects BGRA for optimal performance on little-endian platforms + // - The implicit conversion works because the byte order in memory matches OpenGL's expectationsjvm/src/main/cpp/jni/OSMesa.cpp (3)
9-38: Consider improving error logging mechanismThe
checkFramebufferStatusfunction usesstd::coutfor error logging, which isn't ideal for production code. Consider:
- Using a proper logging framework that integrates with Java's logging system
- Adding error context (e.g., function name, additional state information)
- Adding GL error checking around the
glCheckFramebufferStatuscallstatic bool checkFramebufferStatus() { std::string status; + GLenum error = glGetError(); + if (error != GL_NO_ERROR) { + // Log GL error + return false; + } switch (glCheckFramebufferStatus(GL_FRAMEBUFFER)) { // ... existing cases ... } - std::cout << status << std::endl; + // Use proper logging mechanism + // LogToJava("Framebuffer incomplete: " + status); return false; }
43-53: Enhance structure documentationWhile the structure is well-organized, consider adding detailed documentation for each field, especially:
- The expected lifetime of
buf- The valid ranges for
widthandheight- The relationship between
fboandrboarrays
1-155: Consider architectural improvements for resource managementThe current implementation could benefit from:
- RAII wrapper classes for GL/OSMesa resources to prevent leaks
- A unified error reporting mechanism to Java
- A more robust state validation system
Would you like me to provide an example implementation of these improvements?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
.github/workflows/jvm_build.yml(1 hunks)jvm/pom.xml(1 hunks)jvm/readme.md(1 hunks)jvm/src/main/cpp/jni/GlTextureHelper.cpp(1 hunks)jvm/src/main/cpp/jni/OSMesa.cpp(1 hunks)jvm/src/main/java/io/openmobilemaps/mapscore/graphics/BufferedImageTextureHolder.java(1 hunks)jvm/src/main/java/io/openmobilemaps/mapscore/graphics/util/GlTextureHelper.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- jvm/src/main/java/io/openmobilemaps/mapscore/graphics/BufferedImageTextureHolder.java
- jvm/src/main/java/io/openmobilemaps/mapscore/graphics/util/GlTextureHelper.java
🧰 Additional context used
📓 Learnings (2)
.github/workflows/jvm_build.yml (1)
Learnt from: matzf
PR: openmobilemaps/maps-core#652
File: .github/workflows/jvm_build.yml:7-7
Timestamp: 2024-12-02T19:52:56.213Z
Learning: In this repository, it's acceptable to use `@main` for reusable workflow references in GitHub Actions workflows, and pinning to a specific version is not required.
jvm/src/main/cpp/jni/GlTextureHelper.cpp (2)
Learnt from: matzf
PR: openmobilemaps/maps-core#652
File: jvm/src/main/java/io/openmobilemaps/mapscore/graphics/util/GlTextureHelper.java:16-18
Timestamp: 2024-12-02T19:53:53.134Z
Learning: The private native method `createTextureARGB` in `GlTextureHelper.java` is only used internally via the public `createTexture` method, which ensures that valid data is passed. Therefore, additional input validation in `createTextureARGB` is not necessary.
Learnt from: matzf
PR: openmobilemaps/maps-core#652
File: jvm/src/main/java/io/openmobilemaps/mapscore/graphics/util/GlTextureHelper.java:16-18
Timestamp: 2024-12-02T19:53:53.134Z
Learning: In the `GlTextureHelper` class, the `textureId` parameter in `deleteTexture` is an opaque ID that cannot be validated and does not require additional input validation.
🪛 LanguageTool
jvm/readme.md
[uncategorized] ~5-~5: Do not mix variants of the same word (‘offscreen’ and ‘off-screen’) within a single text.
Context: ... For now, this only explicitly supports offscreen rendering via [OSMesa](https://docs.mes...
(EN_WORD_COHERENCY)
🪛 Markdownlint (0.35.0)
jvm/readme.md
25-25: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
36-36: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
45-45: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
52-52: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (3)
jvm/pom.xml (1)
23-27:
Fix invalid Jackson version.
The specified Jackson version 2.17.2 doesn't exist yet.
<dependency>
<groupId>com.fasterxml.jackson.core</groupId>
<artifactId>jackson-databind</artifactId>
- <version>2.17.2</version>
+ <version>2.16.1</version>
</dependency>jvm/src/main/cpp/jni/GlTextureHelper.cpp (2)
32-37: Improve error handling in texture deletion.
While textureId validation isn't necessary (as per learnings), the error handling should be improved.
JNIEXPORT void JNICALL Java_io_openmobilemaps_mapscore_graphics_util_GlTextureHelper_deleteTexture(JNIEnv *env, jclass,
jint textureId) {
GLuint textures[] = {(GLuint)textureId};
glDeleteTextures(1, textures);
- glGetError();
+ GLenum error = glGetError();
+ if (error != GL_NO_ERROR) {
+ jclass exClass = env->FindClass("java/lang/RuntimeException");
+ std::string errorMsg = "Failed to delete texture: " + std::to_string(error);
+ env->ThrowNew(exClass, errorMsg.c_str());
+ }
}12-29:
Add error handling and resource cleanup.
The texture creation process lacks proper error handling and cleanup. While input validation is handled by the Java side (as per learnings), OpenGL errors should still be checked.
GLuint textureId;
+ jint* dataBuf = nullptr;
+ try {
glGenTextures(1, &textureId);
+ if (textureId == 0) {
+ throw std::runtime_error("Failed to generate texture");
+ }
glBindTexture(GL_TEXTURE_2D, textureId);
glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR);
glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR);
glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE);
glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);
const jsize outLen = env->GetArrayLength(data);
dataBuf = env->GetIntArrayElements(data, NULL);
+ if (!dataBuf) {
+ throw std::runtime_error("Failed to get array elements");
+ }
glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA8, (GLsizei)width, (GLsizei)height, 0, GL_BGRA, GL_UNSIGNED_BYTE, dataBuf);
+ GLenum error = glGetError();
+ if (error != GL_NO_ERROR) {
+ throw std::runtime_error("Failed to create texture: " + std::to_string(error));
+ }
env->ReleaseIntArrayElements(data, dataBuf, JNI_ABORT);
return (jint)textureId;
+ } catch (const std::exception& e) {
+ if (dataBuf) {
+ env->ReleaseIntArrayElements(data, dataBuf, JNI_ABORT);
+ }
+ if (textureId != 0) {
+ glDeleteTextures(1, &textureId);
+ }
+ jclass exClass = env->FindClass("java/lang/RuntimeException");
+ env->ThrowNew(exClass, e.what());
+ return 0;
+ }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/LocalDataLoader.java (1)
130-132: Consider Implementing thecancelMethodThe
cancelmethod is currently empty. If cancellation of ongoing operations is required, consider implementing the logic to support it.jvm/src/main/java/io/openmobilemaps/mapscore/map/util/MapTileRenderer.java (3)
119-120: Investigate potential edge issues at tile bordersThe TODO comment mentions uncertainty about the safety of calling
update()on the camera or layer for each tile separately, possibly causing issues at tile borders. It's important to verify whether this could lead to rendering artifacts or inconsistencies between adjacent tiles.Would you like assistance in analyzing this potential issue and proposing a solution? I can help assess the impact and suggest remedial actions. If needed, we can open a new GitHub issue to track this task.
128-138: Enhance exception handling inforceRenderTilemethodIn
forceRenderTile, the method only checks forLayerReadyState.NOT_READY. There might be other states where forcing a draw frame is appropriate, such asPARTIALLY_READY. Consider updating the exception handling to account for other relevant states.Apply this diff to improve state handling:
} catch (OffscreenMapRenderer.MapLayerException e) { - if (e.getState() == LayerReadyState.NOT_READY) { + if (e.getState() == LayerReadyState.NOT_READY || e.getState() == LayerReadyState.PARTIALLY_READY) { return renderer.forceDrawFrame(); } else { throw e; } }This change ensures that the method reacts appropriately to additional layer ready states.
14-16: Provide detailed class-level documentationThe current class-level JavaDoc provides a brief description. Expanding this documentation to include details about the class's purpose, usage examples, and any assumptions will improve maintainability and help other developers understand how to use
MapTileRenderereffectively.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/LocalDataLoader.java(1 hunks)jvm/src/main/java/io/openmobilemaps/mapscore/map/util/MapTileRenderer.java(1 hunks)
🔇 Additional comments (3)
jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/LocalDataLoader.java (3)
122-122: Typo in Method Name loadTextureAsnyc
There's a typo in the method name loadTextureAsnyc. It should be loadTextureAsync.
Apply this diff to correct the method name:
@NotNull
@Override
-public Future<TextureLoaderResult> loadTextureAsnyc(
+public Future<TextureLoaderResult> loadTextureAsync(
@NotNull String url, @Nullable String etag) {89-96: Make loadDataAsync Truly Asynchronous
The method loadDataAsync currently executes synchronously, which may block the calling thread and defeats the purpose of asynchronous execution.
Consider using an ExecutorService to execute this method asynchronously. Apply this diff to update the method:
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+private final ExecutorService executorService = Executors.newCachedThreadPool();
@NotNull
@Override
public Future<DataLoaderResult> loadDataAsync(@NotNull String url, @Nullable String etag) {
- // No async here.
- var result = new Promise<DataLoaderResult>();
- result.setValue(loadData(url, etag));
- return result.getFuture();
+ var promise = new Promise<DataLoaderResult>();
+ executorService.submit(() -> {
+ promise.setValue(loadData(url, etag));
+ });
+ return promise.getFuture();
}
+// Ensure to shut down the executor service appropriately, perhaps in a close method or when the application exits.122-128: Make loadTextureAsync Truly Asynchronous
The method loadTextureAsync currently executes synchronously, which may block the calling thread and defeats the purpose of asynchronous execution.
Consider using an ExecutorService to execute this method asynchronously. Apply this diff to update the method:
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+private final ExecutorService executorService = Executors.newCachedThreadPool();
@NotNull
@Override
-public Future<TextureLoaderResult> loadTextureAsnyc(
+public Future<TextureLoaderResult> loadTextureAsync(
@NotNull String url, @Nullable String etag) {
- // No async here.
- var result = new Promise<TextureLoaderResult>();
- result.setValue(loadTexture(url, etag));
- return result.getFuture();
+ var promise = new Promise<TextureLoaderResult>();
+ executorService.submit(() -> {
+ promise.setValue(loadTexture(url, etag));
+ });
+ return promise.getFuture();
}
+// Ensure to shut down the executor service appropriately, perhaps in a close method or when the application exits.
jvm/src/main/java/io/openmobilemaps/mapscore/map/util/MapTileRenderer.java
Outdated
Show resolved
Hide resolved
jvm/src/main/java/io/openmobilemaps/mapscore/map/util/MapTileRenderer.java
Outdated
Show resolved
Hide resolved
jvm/src/main/java/io/openmobilemaps/mapscore/map/util/MapTileRenderer.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
jvm/src/main/java/io/openmobilemaps/mapscore/map/util/MapTileRenderer.java (1)
129-139:⚠️ Potential issueCamera modifications in MapTileRenderer are not thread-safe
The
renderTilemethod modifies shared camera state without synchronization, which could lead to race conditions in concurrent usage.Consider adding synchronization:
public BufferedImage renderTile(int zoomLevel, int xcol, int yrow, Duration timeout) throws OffscreenMapRenderer.MapLayerException { + synchronized(renderer) { var cam = renderer.getMap().getCamera(); cam.freeze(false); var tile = getTileBBox(zoomLevel, xcol, yrow); cam.moveToBoundingBox(tile, 0.0f, false, null, null); return renderer.drawFrame(timeout); + } }jvm/src/main/cpp/jni/OSMesa.cpp (2)
55-82:⚠️ Potential issueAddress memory safety and error handling concerns
This code segment has several safety issues that need to be addressed:
- Missing null check after allocation
- No error handling for context creation failure
- Unsafe pointer-to-long conversion
Building upon the previous review, apply these improvements:
JNIEXPORT jlong JNICALL Java_io_openmobilemaps_mapscore_graphics_util_OSMesa_createContext(JNIEnv *env, jclass) { - JNIOSMesaState *state = new JNIOSMesaState{}; + JNIOSMesaState *state = new (std::nothrow) JNIOSMesaState{}; + if (!state) { + env->ThrowNew(env->FindClass("java/lang/OutOfMemoryError"), "Failed to allocate OSMesa state"); + return 0; + } // ... context creation ... + if (!state->ctx) { + delete state; + env->ThrowNew(env->FindClass("java/lang/RuntimeException"), "Failed to create OSMesa context"); + return 0; + } - return (jlong)state; + return static_cast<jlong>(reinterpret_cast<uintptr_t>(state)); }
144-154:⚠️ Potential issueImprove cleanup safety and resource handling
The cleanup function needs improvements in:
- Null checking
- GL error handling
- Resource cleanup order
Apply these safety improvements:
JNIEXPORT void JNICALL Java_io_openmobilemaps_mapscore_graphics_util_OSMesa_destroy( JNIEnv *env, jclass, jlong stateArg) { JNIOSMesaState *state = (JNIOSMesaState *)stateArg; + if (!state) return; + // Delete GL resources first glDeleteRenderbuffers(2, state->rbo); glDeleteFramebuffers(1, &state->fbo); + + // Then destroy context and free buffer OSMesaDestroyContext(state->ctx); - free(state->buf); + if (state->buf) { + free(state->buf); + } - *state = JNIOSMesaState{}; // Don't zero state before deletion delete state; }
🧹 Nitpick comments (9)
jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/FontLoader.java (3)
24-25: Enhance documentation for DPI factor calculationThe comment
render-DPI / 160.0could be more descriptive. Consider documenting why 160.0 is used as the base DPI value and how it relates to Android's density-independent pixel calculations.- private final double - dpFactor; // !< render-DPI / 160.0, factor for "Density Independent Pixel" size. + private final double dpFactor; // Base DPI of 160 matches Android's reference density. + // This factor converts font sizes to density-independent pixels + // to maintain consistent rendering across different DPI displays.
52-53: Use structured logging for better observabilityThe current logging uses string concatenation and basic formatting. Consider using structured logging with log levels for better filtering and monitoring.
- logger.info("loadFont " + fontName); + logger.fine(() -> String.format("Starting font load request for: %s", fontName)); var result = loadFont(fontName); - logger.info(String.format("loadFont %s -> %s", fontName, result.getStatus())); + logger.info(() -> String.format("Font load completed - name: %s, status: %s", + fontName, result.getStatus()));Also applies to: 65-66
129-131: Add constructor validation for FontDataStreamsConsider adding validation in the record constructor to ensure at least one of the streams is non-null.
protected record FontDataStreams(InputStream image, InputStream manifest) implements AutoCloseable { + public FontDataStreams { + if (image == null && manifest == null) { + throw new IllegalArgumentException("At least one stream must be non-null"); + } + }jvm/src/main/java/io/openmobilemaps/mapscore/map/util/MapTileRenderer.java (3)
14-17: Enhance class documentation with thread safety detailsThe class documentation should specify thread safety guarantees or limitations, especially since it manages shared resources like camera state and renderer.
Add thread safety documentation:
/** * Off-screen tile renderer for an openmobilemaps Map. + * + * Note: This class is not thread-safe. All methods should be called from the same thread + * that owns the OffscreenMapRenderer instance. */
145-155: Make timeout configurable in forceRenderTileThe method uses a fixed 50ms timeout which might be too short for slower systems or complex scenes.
- public BufferedImage forceRenderTile(int zoomLevel, int xcol, int yrow) throws OffscreenMapRenderer.MapLayerException { + public BufferedImage forceRenderTile(int zoomLevel, int xcol, int yrow, Duration timeout) throws OffscreenMapRenderer.MapLayerException { + Duration effectiveTimeout = timeout != null ? timeout : Duration.ofMillis(50); try { - return renderTile(zoomLevel, xcol, yrow, Duration.ofMillis(50)); + return renderTile(zoomLevel, xcol, yrow, effectiveTimeout);
157-158: Add validation in TileRange recordThe record should validate that the ranges are consistent (minColumn ≤ maxColumn, minRow ≤ maxRow).
- public record TileRange(int zoomLevel, int minColumn, int maxColumn, int minRow, int maxRow) { + public record TileRange(int zoomLevel, int minColumn, int maxColumn, int minRow, int maxRow) { + public TileRange { + if (minColumn > maxColumn) { + throw new IllegalArgumentException("minColumn > maxColumn"); + } + if (minRow > maxRow) { + throw new IllegalArgumentException("minRow > maxRow"); + } + } }jvm/src/main/cpp/jni/OSMesa.cpp (2)
9-38: Consider improving error logging mechanismThe
checkFramebufferStatusfunction usesstd::coutdirectly for error logging, which may not be appropriate for a library. Consider:
- Using a proper logging mechanism that can be configured by the application
- Including error codes in the output for better debugging
- std::cout << status << std::endl; + // TODO: Replace with proper logging mechanism + std::cerr << "Framebuffer incomplete: " << status << " (0x" + << std::hex << glCheckFramebufferStatus(GL_FRAMEBUFFER) + << ")" << std::endl;
43-53: Consider enhancing struct safety and const correctnessThe
JNIOSMesaStatestruct could benefit from:
- Default member initialization
- Making width/height const after initialization
- Adding validity checks
struct JNIOSMesaState { - OSMesaContext ctx; - void *buf; + OSMesaContext ctx = nullptr; + void *buf = nullptr; - GLuint width; - GLuint height; + GLuint width = 0; + GLuint height = 0; // Multisampled framebuffer / renderbuffer objects; if using Multisample Anti-Aliasing. - GLuint fbo; - GLuint rbo[2]; + GLuint fbo = 0; + GLuint rbo[2] = {0, 0}; + + bool isValid() const { return ctx != nullptr && buf != nullptr; } };jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/HttpDataLoader.java (1)
78-78: Log exceptions with appropriate level and stack traceExceptions are currently being logged at the
infolevel without the stack trace, which might make debugging harder. Consider logging exceptions at theWARNINGorSEVERElevel and include the stack trace for better diagnostics.Apply this diff to improve exception logging:
For
loadDatamethod (line 78):-logger.info(String.format("loadData %s -> %s", url, e)); +logger.log(Level.SEVERE, String.format("loadData %s -> exception", url), e);For
loadTexturemethod (line 120):-logger.info(String.format("loadTexture %s -> %s", url, e)); +logger.log(Level.SEVERE, String.format("loadTexture %s -> exception", url), e);For
completeLoadDatamethod (line 169):-// Exception handling without logging +logger.log(Level.SEVERE, String.format("completeLoadData %s -> exception", response.uri()), e);For
completeLoadTexturemethod (line 202):-// Exception handling without logging +logger.log(Level.SEVERE, String.format("completeLoadTexture %s -> exception", response.uri()), e);Also applies to: 120-120, 169-169, 202-202
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
jvm/src/test/resources/golden/OffscreenMapRendererTest_testStyleJsonPortrait.pngis excluded by!**/*.png
📒 Files selected for processing (5)
jvm/src/main/cpp/jni/OSMesa.cpp(1 hunks)jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/FontLoader.java(1 hunks)jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/HttpDataLoader.java(1 hunks)jvm/src/main/java/io/openmobilemaps/mapscore/map/util/MapTileRenderer.java(1 hunks)jvm/src/test/java/io/openmobilemaps/mapscore/OffscreenMapRendererTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- jvm/src/test/java/io/openmobilemaps/mapscore/OffscreenMapRendererTest.java
🧰 Additional context used
📓 Learnings (2)
jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/FontLoader.java (1)
Learnt from: matzf
PR: openmobilemaps/maps-core#652
File: jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/FontJsonManifestReader.java:26-77
Timestamp: 2024-12-02T20:36:38.651Z
Learning: In the Java file `jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/FontJsonManifestReader.java`, the user prefers to keep the `extractFontData` method as is, without refactoring it into smaller methods.
jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/HttpDataLoader.java (1)
Learnt from: matzf
PR: openmobilemaps/maps-core#652
File: jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/HttpDataLoader.java:150-152
Timestamp: 2024-12-16T08:12:19.941Z
Learning: In `HttpDataLoader.java`, within the `completeLoadData` method, we need to use `ByteBuffer.allocateDirect` to create a direct allocated buffer, as it is required for our use case.
🔇 Additional comments (10)
jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/FontLoader.java (2)
107-108: DPI scaling implementation note
The comment about moving DPI scaling to the shader has been previously discussed. This is a of a prior review suggestion.
78-80:
Enhance error handling with detailed logging
The catch block swallows the exception details, making it difficult to diagnose issues.
} catch (Exception e) {
+ logger.log(Level.WARNING, String.format("Failed to load font: %s", fontName), e);
return new FontLoaderResult(null, null, LoaderStatus.ERROR_OTHER);
}Likely invalid or redundant comment.
jvm/src/main/java/io/openmobilemaps/mapscore/map/util/MapTileRenderer.java (3)
62-89: LGTM! Well-implemented tile range calculation
The implementation correctly follows the OGC standard with proper parameter validation and floating-point handling.
91-122: LGTM! Comprehensive implementation with proper validation
The method correctly implements the OGC standard for calculating tile bounding boxes with thorough parameter validation.
42-42: Review the usage of placeholder strings
The strings "unusedLayerName" and "unusedTileUrlPattern" suggest these parameters are not used. Consider if there's a better way to obtain zoom level information without requiring these unused parameters.
jvm/src/main/cpp/jni/OSMesa.cpp (2)
84-117:
Improve input validation and GL error handling
The context setup needs additional safety measures:
- Input validation for dimensions
- Integer overflow protection
- GL error checking after operations
Apply these safety improvements:
JNIEXPORT jlong JNICALL Java_io_openmobilemaps_mapscore_graphics_util_OSMesa_makeCurrent(
JNIEnv *env, jclass, jlong stateArg, jint width, jint height, jint numSamples) {
+ if (width <= 0 || height <= 0 || numSamples < 0) {
+ env->ThrowNew(env->FindClass("java/lang/IllegalArgumentException"),
+ "Invalid dimensions or sample count");
+ return false;
+ }
+
+ // Check for integer overflow
+ if (width > INT_MAX / height || width * height > INT_MAX / 4) {
+ env->ThrowNew(env->FindClass("java/lang/IllegalArgumentException"),
+ "Dimensions too large");
+ return false;
+ }
JNIOSMesaState *state = (JNIOSMesaState *)stateArg;
+ if (!state) {
+ env->ThrowNew(env->FindClass("java/lang/IllegalArgumentException"),
+ "Invalid state pointer");
+ return false;
+ }
// ... existing code ...
+ auto checkGLError = [env](const char* op) {
+ GLenum err = glGetError();
+ if (err != GL_NO_ERROR) {
+ std::string msg = std::string(op) + " failed: 0x" +
+ std::to_string(err);
+ env->ThrowNew(env->FindClass("java/lang/RuntimeException"),
+ msg.c_str());
+ return false;
+ }
+ return true;
+ };
glGenRenderbuffers(2, state->rbo);
+ if (!checkGLError("glGenRenderbuffers")) return false;
// Add similar error checking for other GL callsLikely invalid or redundant comment.
119-142:
Enhance pixel reading safety and error handling
The pixel reading implementation needs improvements in:
- Array size validation
- GL error checking
- Exception safety for JNI array operations
Apply these safety improvements:
JNIEXPORT void JNICALL Java_io_openmobilemaps_mapscore_graphics_util_OSMesa_readARGB(
JNIEnv *env, jclass, jlong stateArg, jintArray out) {
JNIOSMesaState *state = (JNIOSMesaState *)stateArg;
+ if (!state) {
+ env->ThrowNew(env->FindClass("java/lang/IllegalArgumentException"),
+ "Invalid state pointer");
+ return;
+ }
const jsize outLen = env->GetArrayLength(out);
+ if (outLen < state->width * state->height) {
+ env->ThrowNew(env->FindClass("java/lang/IllegalArgumentException"),
+ "Output array too small");
+ return;
+ }
- jint *outBuf = env->GetIntArrayElements(out, NULL);
+ jint *outBuf = env->GetIntArrayElements(out, nullptr);
+ if (!outBuf) {
+ env->ThrowNew(env->FindClass("java/lang/OutOfMemoryError"),
+ "Failed to get array elements");
+ return;
+ }
// ... blitting code ...
glReadPixels(0, 0, state->width, state->height, GL_BGRA, GL_UNSIGNED_BYTE, outBuf);
+ GLenum error = glGetError();
env->ReleaseIntArrayElements(out, outBuf, 0);
+ if (error != GL_NO_ERROR) {
+ env->ThrowNew(env->FindClass("java/lang/RuntimeException"),
+ "glReadPixels failed");
+ }
}Likely invalid or redundant comment.
jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/HttpDataLoader.java (3)
127-127: Typo in method name loadTextureAsnyc
There's a typo in the method name loadTextureAsnyc. It should be loadTextureAsync to maintain consistency and avoid confusion.
Apply this diff to correct the typo:
- public Future<TextureLoaderResult> loadTextureAsnyc(@NotNull String url, String etag) {
+ public Future<TextureLoaderResult> loadTextureAsync(@NotNull String url, String etag) {150-152: Implement cancellation logic in cancel method
The cancel method is currently empty. If cancellation functionality is required, consider implementing logic to cancel ongoing asynchronous requests.
You could keep track of ongoing requests using a ConcurrentHashMap and call cancel on the corresponding Future when needed.
183-183: Specify expected MIME types
The // TODO: MIME type? comment suggests checking the MIME type of the response to ensure it's an image format that ImageIO can handle.
Consider validating the Content-Type header before processing:
+String contentType = response.headers().firstValue("Content-Type").orElse("");
+if (!contentType.startsWith("image/")) {
+ logger.warning("Invalid content type for image: " + contentType);
+ return new TextureLoaderResult(null, null, LoaderStatus.ERROR_OTHER, "Invalid content type");
+}
jvm/src/main/java/io/openmobilemaps/mapscore/map/util/MapTileRenderer.java
Show resolved
Hide resolved
jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/HttpDataLoader.java
Show resolved
Hide resolved
Small fixes, e.g. avoid crashing on missing font results. Use small JNI function to load textures, instead of depending on a full-blown GL framework for Java. Somewhat simplistic and not optimized, but probably good enough. DataLoader uses java http client which is a bit bare-bones. Might have to be made more tweakable.
Name suffix is linux_amd64 (plus _debug for debug build).
Some of these are used in the Djinni Java support libraries (@keep, Log), some are used in the generated code (@Parcelize/Parcelable). Previously, I've had to modify both the support lib and the generated sources to building on non-android Java.
Also set workflow branch to main in build only workflow, as this has been merged
Accidentally used width as height. For portrait images, it did not read the entire image. There appears to be internal clamping to the actual framebuffer size in glReadPixels, as it did not seem to write past the buffer.
matzf
force-pushed
the
feature/jvm-lib
branch
from
4e6cb17 to
ebd3ae8
Compare
There was a problem hiding this comment.
Actionable comments posted: 11
🔭 Outside diff range comments (1)
android/src/main/cpp/graphics/shader/SphereEffectShaderOpenGl.cpp (1)
Line range hint
44-49: Add bounds checking for safer memory operationsWhile the implementation is thread-safe with mutex protection, there are potential memory safety concerns:
- Missing size validation for the destination buffer
- No null pointer check for the source address
- Potential integer overflow in size calculation
Consider adding these safety checks:
void SphereEffectShaderOpenGl::setEllipse(const SharedBytes &coefficients) { std::lock_guard<std::mutex> lock(dataMutex); - if(coefficients.elementCount > 0) { + if (coefficients.elementCount <= 0 || !coefficients.address) { + return; + } + + // Prevent integer overflow in size calculation + if (coefficients.elementCount > SIZE_MAX / coefficients.bytesPerElement) { + return; + } + + // Ensure destination buffer is large enough + if (coefficients.elementCount > this->coefficients.size()) { + return; + } + std::memcpy(this->coefficients.data(), (void *)coefficients.address, coefficients.elementCount * coefficients.bytesPerElement); - } }
♻️ Duplicate comments (5)
jvm/pom.xml (1)
23-27:⚠️ Potential issueFix invalid Jackson version.
The specified Jackson version 2.17.2 doesn't exist yet. The latest stable version is 2.16.1.
standalone/main.cpp (3)
183-186:⚠️ Potential issueInitialize OpenGL object identifiers to zero.
Using negative values for
GLuinttypes can lead to undefined behavior.- GLuint fbo = -1; - GLuint rbo[2] = {(GLuint)-1, (GLuint)-1}; + GLuint fbo = 0; + GLuint rbo[2] = {0, 0};
57-58: 🛠️ Refactor suggestionUse RAII for safer memory management.
Replace manual memory management with RAII patterns to prevent memory leaks.
- void *buf = malloc(width * height * 4); + std::vector<unsigned char> buf(width * height * 4); - auto ok = OSMesaMakeCurrent(ctx, buf, GL_UNSIGNED_BYTE, width, height); + auto ok = OSMesaMakeCurrent(ctx, buf.data(), GL_UNSIGNED_BYTE, width, height); // ... existing code ... - free(buf); // buf is automatically freedAlso applies to: 113-114
74-77:⚠️ Potential issueFix camera setup order.
The camera must be set before viewport size as indicated by the BUG comment.
map->getRenderingContext()->onSurfaceCreated(); - map->setViewportSize(Vec2I(width, height)); - // map->setCamera(MapCamera2dInterface::create(map, 1.0f)); // BUG! must be _before_ setViewportSize + map->setCamera(MapCamera2dInterface::create(map, 1.0f)); + map->setViewportSize(Vec2I(width, height));jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/LocalDataLoader.java (1)
92-96: 🛠️ Refactor suggestionImprove Asynchronous Handling in
loadDataAsyncandloadTextureAsyncCurrently, the methods
loadDataAsyncandloadTextureAsyncexecute synchronously, which may block the calling thread and defeat the purpose of asynchronous execution.Consider using an
ExecutorServiceto execute these methods asynchronously:Add necessary imports:
+import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors;Initialize the
ExecutorService:+private final ExecutorService executorService = Executors.newCachedThreadPool();Update
loadDataAsyncmethod:@NotNull @Override public Future<DataLoaderResult> loadDataAsync(@NotNull String url, @Nullable String etag) { - // No async here. - var result = new Promise<DataLoaderResult>(); - result.setValue(loadData(url, etag)); - return result.getFuture(); + var promise = new Promise<DataLoaderResult>(); + executorService.submit(() -> { + promise.setValue(loadData(url, etag)); + }); + return promise.getFuture(); }Update
loadTextureAsyncmethod:@NotNull @Override public Future<TextureLoaderResult> loadTextureAsync(@NotNull String url, @Nullable String etag) { - // No async here. - var result = new Promise<TextureLoaderResult>(); - result.setValue(loadTexture(url, etag)); - return result.getFuture(); + var promise = new Promise<TextureLoaderResult>(); + executorService.submit(() -> { + promise.setValue(loadTexture(url, etag)); + }); + return promise.getFuture(); }Remember to shut down the executor service appropriately, perhaps in a
closemethod or when the application exits.Also applies to: 123-128
🧹 Nitpick comments (21)
jvm/readme.md (6)
1-9: Maintain consistent spelling throughout the documentThe term "off-screen" is used in the title while "offscreen" is used in the content. Choose one spelling and use it consistently.
-# Java bindings for off-screen use of openmobilemaps +# Java bindings for offscreen use of openmobilemaps🧰 Tools
🪛 LanguageTool
[uncategorized] ~5-~5: Do not mix variants of the same word (‘offscreen’ and ‘off-screen’) within a single text.
Context: ... For now, this only explicitly supports offscreen rendering via [OSMesa](https://docs.mes...(EN_WORD_COHERENCY)
26-27: Add language specification to the code blockSpecify the shell language for proper syntax highlighting.
-``` +```shell apt install --no-install-recommends maven cmake make clang libgl-dev libgles-dev libosmesa6-dev openjdk-21-jdk-headless--- `36-38`: **Add language specification to the code block** Specify the shell language for proper syntax highlighting. ```diff -``` +```shell apt install --no-install-recommends libosmesa6 libgl1<details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint (0.37.0)</summary> 36-36: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> --- `45-48`: **Add language specification to the code block** Specify the shell language for proper syntax highlighting. ```diff -``` +```shell git submodule init git submodule update<details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint (0.37.0)</summary> 45-45: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> --- `51-54`: **Enhance build instructions documentation** 1. Add language specification to the code block 2. Consider adding more details about the tests that will be run ```diff -``` +```shell mvn clean installConsider adding a brief explanation of what tests are included and how to verify successful execution. <details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint (0.37.0)</summary> 52-52: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> --- `14-23`: **Consider enhancing dependency documentation** The build dependencies section could benefit from: 1. Specific version requirements or minimum versions for each dependency 2. A troubleshooting section addressing common build issues </blockquote></details> <details> <summary>jvm/pom.xml (2)</summary><blockquote> `15-15`: **Consider making the C++ compiler configurable** Hard-coding `clang++` as the C++ compiler might limit portability across different build environments. Consider making this configurable through a profile or system property. Example approach: ```xml <properties> <CMAKE_CXX_COMPILER>${env.CXX_COMPILER:-clang++}</CMAKE_CXX_COMPILER> </properties>
111-124: Add support for platform-specific native librariesThe current configuration only handles Linux
.sofiles. Given the PR objectives mention potential multi-platform support:
- The resource copying should handle platform-specific library extensions (
.dll,.dylib, etc.)- Consider using platform-specific resource directories to avoid loading unnecessary libraries
Example approach:
<resources> <resource> <directory>${project.build.directory}/build-native-release/jvm</directory> <includes> <include>libmapscore_jni_*.so</include> <include>libmapscore_jni_*.dll</include> <include>libmapscore_jni_*.dylib</include> </includes> <targetPath>native/${os.detected.classifier}/</targetPath> </resource> </resources>Consider adding the os-maven-plugin for proper OS detection:
<build> <extensions> <extension> <groupId>kr.motd.maven</groupId> <artifactId>os-maven-plugin</artifactId> <version>1.7.1</version> </extension> </extensions> </build>jvm/src/main/java/io/openmobilemaps/mapscore/map/util/MapTileRenderer.java (4)
14-17: Enhance class documentationThe current documentation is minimal. Consider adding:
- Thread safety guarantees
- Coordinate system details
- Usage examples
- Relationship with
OffscreenMapRenderer
26-32: Extract default values into named constantsConsider extracting the magic numbers into named constants for better maintainability and documentation:
+ private static final int DEFAULT_TILE_SIZE = 512; + private static final int DEFAULT_NUM_SAMPLES = 4; + public MapTileRenderer(OffscreenMapRenderer renderer) { this( renderer, - 512, - 4 + DEFAULT_TILE_SIZE, + DEFAULT_NUM_SAMPLES ); }
81-87: Extract epsilon into a named constantConsider extracting the epsilon value into a named constant with documentation explaining its purpose:
+ /** + * Small value used to handle floating-point arithmetic precision in tile calculations. + */ + private static final double TILE_CALCULATION_EPSILON = 1e-6; + // ... - final double epsilon = 1e-6; final int minCol = (int) Math.floor((bboxMinX - tileMatrixMinX) / tileSpan + epsilon); final int maxCol = (int) Math.floor((bboxMaxX - tileMatrixMinX) / tileSpan - epsilon); // Note: tiles indexed from top row (tileMatrixMaxY corresponds to row 0). final int minRow = (int) Math.floor((tileMatrixMaxY - bboxMaxY) / tileSpan + epsilon); final int maxRow = (int) Math.floor((tileMatrixMaxY - bboxMinY) / tileSpan - epsilon);
157-159: Consider adding validation to TileRange recordWhile records are typically simple data holders, consider adding validation to ensure invariants:
- public record TileRange(int zoomLevel, int minColumn, int maxColumn, int minRow, int maxRow) { + public record TileRange(int zoomLevel, int minColumn, int maxColumn, int minRow, int maxRow) { + public TileRange { + if (minColumn > maxColumn) { + throw new IllegalArgumentException("minColumn must be <= maxColumn"); + } + if (minRow > maxRow) { + throw new IllegalArgumentException("minRow must be <= maxRow"); + } + if (zoomLevel < 0) { + throw new IllegalArgumentException("zoomLevel must be non-negative"); + } + } }shared/src/map/layers/objects/Circle2dLayerObject.cpp (2)
Line range hint
31-45: Document spherical coordinate transformation assumptionsThe 3D transformation assumes a unit sphere (radius 1.0) but this isn't documented. Consider adding comments explaining the coordinate system and assumptions.
void Circle2dLayerObject::setPosition(Coord position, double radius) { + // Convert to render system coordinates Coord renderPos = conversionHelper->convertToRenderSystem(position); auto origin = Vec3D(renderPos.x, renderPos.y, renderPos.z); if (is3d) { + // Transform to unit sphere coordinates (radius = 1.0) + // x = sin(lat) * cos(lon) + // y = cos(lat) + // z = -sin(lat) * sin(lon) origin.x = 1.0 * sin(renderPos.y) * cos(renderPos.x); origin.y = 1.0 * cos(renderPos.y); origin.z = -1.0 * sin(renderPos.y) * sin(renderPos.x); }
Line range hint
31-45: Consider adding parameter validation and using named constantsThe method could benefit from input validation and better readability through named constants.
void Circle2dLayerObject::setPosition(Coord position, double radius) { + if (radius <= 0.0) { + throw std::invalid_argument("Circle radius must be positive"); + } + + const double UNIT_SPHERE_RADIUS = 1.0; Coord renderPos = conversionHelper->convertToRenderSystem(position); auto origin = Vec3D(renderPos.x, renderPos.y, renderPos.z); if (is3d) { - origin.x = 1.0 * sin(renderPos.y) * cos(renderPos.x); - origin.y = 1.0 * cos(renderPos.y); - origin.z = -1.0 * sin(renderPos.y) * sin(renderPos.x); + origin.x = UNIT_SPHERE_RADIUS * sin(renderPos.y) * cos(renderPos.x); + origin.y = UNIT_SPHERE_RADIUS * cos(renderPos.y); + origin.z = -UNIT_SPHERE_RADIUS * sin(renderPos.y) * sin(renderPos.x); }standalone/readme.md (2)
3-10: Improve documentation readability and clarity.
- Add hyphenation to "platform-specific" for better readability.
- Consider adding a note about potential future platform support plans.
-be used directly from C++, without the platform specific bindings. +be used directly from C++, without the platform-specific bindings.🧰 Tools
🪛 LanguageTool
[uncategorized] ~7-~7: When ‘platform-specific’ is used as a modifier, it is usually spelled with a hyphen.
Context: ... be used directly from C++, without the platform specific bindings. Currently, this can be built...(SPECIFIC_HYPHEN)
24-27: Add language specifiers to code blocks.Add language specifiers to the code blocks for better syntax highlighting:
-``` +```bash apt install --no-install-recommends cmake make clang libgl-dev libgles-dev libosmesa6-dev-
+bash
cmake -G build-directory
cmake --build build-directory -- testmainAlso applies to: 31-37 <details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint (0.37.0)</summary> 25-25: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </blockquote></details> <details> <summary>standalone/main.cpp (2)</summary><blockquote> `117-147`: **Standardize error logging mechanism.** The function uses `std::cout` while other parts of the code use `printf`. Consider standardizing to `std::cerr` for error logging. ```diff - std::cout << error << " | " << file << ":" << line << std::endl; + std::cerr << error << " | " << file << ":" << line << std::endl;
275-282: Consider extracting polygon coordinates to configuration.Hardcoded coordinates make the code less maintainable. Consider moving these to a configuration file or constants.
+namespace { + const std::vector<std::pair<float, float>> RAUTE_COORDS = { + {0.0332951, 0.569277}, + {0.445824, 0.874362}, + // ... remaining coordinates + }; + + const std::vector<std::pair<float, float>> HEXAGON_COORDS = { + {0.507147, 0.258516}, + {0.719898, 0.415755}, + // ... remaining coordinates + }; +}Also applies to: 292-299
jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/LocalDataLoader.java (1)
131-133: Implement Cancellation Logic incancelMethodThe
cancelmethod is currently empty. If cancellation functionality is required, consider implementing logic to cancel ongoing asynchronous operations.jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/HttpDataLoader.java (1)
77-80: Use Appropriate Logging Levels for ExceptionsCurrently, exceptions are being logged at the
infolevel inloadData,loadDataAsync,loadTexture, andloadTextureAsyncmethods. To improve error diagnosis, consider usingwarningorseverelog levels when exceptions occur.Apply this diff to adjust the logging levels:
In
loadData:} catch (IOException | InterruptedException e) { - logger.info(String.format("loadData %s -> %s", url, e)); + logger.log(Level.WARNING, String.format("loadData %s -> %s", url, e), e); return new DataLoaderResult(null, null, LoaderStatus.ERROR_OTHER, e.toString()); }In
loadDataAsync:.exceptionally(e -> { - logger.info(String.format("loadDataAsync %s -> %s", url, e)); + logger.log(Level.WARNING, String.format("loadDataAsync %s -> %s", url, e), e); return new DataLoaderResult(null, null, LoaderStatus.ERROR_OTHER, e.toString()); })In
loadTexture:} catch (InterruptedException | IOException e) { - logger.info(String.format("loadTexture %s -> %s", url, e)); + logger.log(Level.WARNING, String.format("loadTexture %s -> %s", url, e), e); return new TextureLoaderResult(null, null, LoaderStatus.ERROR_OTHER, e.toString()); }In
loadTextureAsync:.exceptionally(e -> { - logger.info(String.format("loadTextureAsync %s -> %s", url, e)); + logger.log(Level.WARNING, String.format("loadTextureAsync %s -> %s", url, e), e); return new TextureLoaderResult(null, null, LoaderStatus.ERROR_OTHER, e.toString()); })Also applies to: 98-100, 119-122, 140-142
jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/FontLoader.java (1)
56-59: ReusefallbackNameVariable to Avoid Redundant CallsIn the
loadFont(Font font)method, you can reuse thefallbackNamevariable instead of callinggetFallbackFont(fontName)again. This prevents redundant method calls and enhances code readability.Apply this diff:
if (fallbackName != null) { - result = loadFont(getFallbackFont(fontName)); + result = loadFont(fallbackName); logger.info( String.format( "loadFont(%s) -> Fallback %s -> %s", fontName, fallbackName, result.getStatus())); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
jvm/src/test/resources/fonts/Roboto-Regular.pngis excluded by!**/*.pngjvm/src/test/resources/golden/OffscreenMapRendererTest_testStyleJson.pngis excluded by!**/*.pngjvm/src/test/resources/golden/OffscreenMapRendererTest_testStyleJsonLabel.pngis excluded by!**/*.pngjvm/src/test/resources/golden/OffscreenMapRendererTest_testStyleJsonPortrait.pngis excluded by!**/*.pngjvm/src/test/resources/golden/OffscreenMapRendererTest_testTiler_tile_7_66_44.pngis excluded by!**/*.pngjvm/src/test/resources/golden/OffscreenMapRendererTest_testTiler_tile_7_66_45.pngis excluded by!**/*.pngjvm/src/test/resources/golden/OffscreenMapRendererTest_testTiler_tile_7_67_44.pngis excluded by!**/*.pngjvm/src/test/resources/golden/OffscreenMapRendererTest_testTiler_tile_7_67_45.pngis excluded by!**/*.png
📒 Files selected for processing (54)
.github/workflows/jvm_build.yml(1 hunks).github/workflows/jvm_package_deploy.yml(1 hunks).gitignore(2 hunks)CMakeLists.txt(1 hunks)android/CMakeLists.txt(1 hunks)android/src/main/cpp/graphics/OpenGlHelper.h(1 hunks)android/src/main/cpp/graphics/shader/ColorLineGroup2dShaderOpenGl.cpp(2 hunks)android/src/main/cpp/graphics/shader/ColorShaderOpenGl.cpp(1 hunks)android/src/main/cpp/graphics/shader/SphereEffectShaderOpenGl.cpp(2 hunks)android/src/main/java/io/openmobilemaps/mapscore/map/util/OffscreenMapRenderer.kt(1 hunks)jvm/CMakeLists.txt(1 hunks)jvm/pom.xml(1 hunks)jvm/readme.md(1 hunks)jvm/src/main/cpp/jni/GlTextureHelper.cpp(1 hunks)jvm/src/main/cpp/jni/OSMesa.cpp(1 hunks)jvm/src/main/java/android/os/Parcelable.java(1 hunks)jvm/src/main/java/android/util/Log.java(1 hunks)jvm/src/main/java/androidx/annotation/Keep.java(1 hunks)jvm/src/main/java/com(1 hunks)jvm/src/main/java/io/openmobilemaps/mapscore/MapsCore.java(1 hunks)jvm/src/main/java/io/openmobilemaps/mapscore/graphics/BufferedImageTextureHolder.java(1 hunks)jvm/src/main/java/io/openmobilemaps/mapscore/graphics/util/GlTextureHelper.java(1 hunks)jvm/src/main/java/io/openmobilemaps/mapscore/graphics/util/OSMesa.java(1 hunks)jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/FontJsonManifestReader.java(1 hunks)jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/FontLoader.java(1 hunks)jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/HttpDataLoader.java(1 hunks)jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/LocalDataLoader.java(1 hunks)jvm/src/main/java/io/openmobilemaps/mapscore/map/util/MapTileRenderer.java(1 hunks)jvm/src/main/java/io/openmobilemaps/mapscore/map/util/OffscreenMapRenderer.java(1 hunks)jvm/src/main/java/io/openmobilemaps/mapscore/map/util/Tiled2dMapVectorLayerBuilder.java(1 hunks)jvm/src/main/java/io/openmobilemaps/mapscore/shared(1 hunks)jvm/src/main/java/kotlinx/parcelize/Parcelize.java(1 hunks)jvm/src/test/java/io/openmobilemaps/mapscore/OffscreenMapRendererTest.java(1 hunks)shared/public/Logger.h(0 hunks)shared/public/Tiled2dMapSourceImpl.h(0 hunks)shared/public/Vec2DHelper.h(1 hunks)shared/src/graphics/SceneInterface.cpp(2 hunks)shared/src/logger/Logger.cpp(2 hunks)shared/src/map/camera/MapCamera3d.cpp(1 hunks)shared/src/map/coordinates/CoordinateSystemIdentifiers.cpp(1 hunks)shared/src/map/layers/objects/Circle2dLayerObject.cpp(1 hunks)shared/src/map/layers/objects/PolygonGroup2dLayerObject.cpp(1 hunks)shared/src/map/layers/objects/PolygonPatternGroup2dLayerObject.cpp(1 hunks)shared/src/map/layers/objects/QuadMaskObject.cpp(1 hunks)shared/src/map/layers/text/TextLayer.cpp(1 hunks)shared/src/map/layers/tiled/vector/Tiled2dMapVectorLayerParserHelper.cpp(1 hunks)shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSourceSymbolDataManager.h(1 hunks)shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSymbolObject.cpp(1 hunks)shared/src/map/layers/tiled/vector/tiles/line/Tiled2dMapVectorLineTile.h(1 hunks)shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonPatternTile.h(1 hunks)shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonTile.h(1 hunks)standalone/CMakeLists.txt(1 hunks)standalone/main.cpp(1 hunks)standalone/readme.md(1 hunks)
💤 Files with no reviewable changes (2)
- shared/public/Logger.h
- shared/public/Tiled2dMapSourceImpl.h
✅ Files skipped from review due to trivial changes (1)
- shared/src/map/camera/MapCamera3d.cpp
🚧 Files skipped from review as they are similar to previous changes (39)
- android/src/main/cpp/graphics/shader/ColorShaderOpenGl.cpp
- shared/src/map/coordinates/CoordinateSystemIdentifiers.cpp
- android/CMakeLists.txt
- android/src/main/cpp/graphics/shader/ColorLineGroup2dShaderOpenGl.cpp
- shared/src/map/layers/tiled/vector/Tiled2dMapVectorLayerParserHelper.cpp
- shared/src/map/layers/text/TextLayer.cpp
- android/src/main/cpp/graphics/OpenGlHelper.h
- shared/src/logger/Logger.cpp
- shared/src/map/layers/objects/QuadMaskObject.cpp
- jvm/src/main/java/com
- jvm/src/main/java/androidx/annotation/Keep.java
- jvm/src/main/java/android/os/Parcelable.java
- .gitignore
- shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonTile.h
- shared/src/graphics/SceneInterface.cpp
- shared/src/map/layers/objects/PolygonGroup2dLayerObject.cpp
- jvm/src/main/java/kotlinx/parcelize/Parcelize.java
- shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonPatternTile.h
- jvm/src/main/java/io/openmobilemaps/mapscore/shared
- shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSymbolObject.cpp
- jvm/src/main/java/android/util/Log.java
- android/src/main/java/io/openmobilemaps/mapscore/map/util/OffscreenMapRenderer.kt
- shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSourceSymbolDataManager.h
- shared/src/map/layers/objects/PolygonPatternGroup2dLayerObject.cpp
- .github/workflows/jvm_build.yml
- jvm/src/main/java/io/openmobilemaps/mapscore/graphics/util/GlTextureHelper.java
- jvm/CMakeLists.txt
- .github/workflows/jvm_package_deploy.yml
- jvm/src/main/java/io/openmobilemaps/mapscore/MapsCore.java
- shared/src/map/layers/tiled/vector/tiles/line/Tiled2dMapVectorLineTile.h
- jvm/src/main/java/io/openmobilemaps/mapscore/graphics/BufferedImageTextureHolder.java
- jvm/src/main/cpp/jni/GlTextureHelper.cpp
- shared/public/Vec2DHelper.h
- standalone/CMakeLists.txt
- jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/FontJsonManifestReader.java
- CMakeLists.txt
- jvm/src/main/java/io/openmobilemaps/mapscore/map/util/OffscreenMapRenderer.java
- jvm/src/main/java/io/openmobilemaps/mapscore/graphics/util/OSMesa.java
- jvm/src/test/java/io/openmobilemaps/mapscore/OffscreenMapRendererTest.java
🧰 Additional context used
📓 Learnings (2)
jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/FontLoader.java (1)
Learnt from: matzf
PR: openmobilemaps/maps-core#652
File: jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/FontJsonManifestReader.java:26-77
Timestamp: 2024-12-02T20:36:38.651Z
Learning: In the Java file `jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/FontJsonManifestReader.java`, the user prefers to keep the `extractFontData` method as is, without refactoring it into smaller methods.
jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/HttpDataLoader.java (1)
Learnt from: matzf
PR: openmobilemaps/maps-core#652
File: jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/HttpDataLoader.java:150-152
Timestamp: 2024-12-16T08:12:19.941Z
Learning: In `HttpDataLoader.java`, within the `completeLoadData` method, we need to use `ByteBuffer.allocateDirect` to create a direct allocated buffer, as it is required for our use case.
🪛 LanguageTool
jvm/readme.md
[uncategorized] ~5-~5: Do not mix variants of the same word (‘offscreen’ and ‘off-screen’) within a single text.
Context: ... For now, this only explicitly supports offscreen rendering via [OSMesa](https://docs.mes...
(EN_WORD_COHERENCY)
standalone/readme.md
[uncategorized] ~7-~7: When ‘platform-specific’ is used as a modifier, it is usually spelled with a hyphen.
Context: ... be used directly from C++, without the platform specific bindings. Currently, this can be built...
(SPECIFIC_HYPHEN)
🪛 Markdownlint (0.37.0)
jvm/readme.md
25-25: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
36-36: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
45-45: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
52-52: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
standalone/readme.md
25-25: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
31-31: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (21)
android/src/main/cpp/graphics/shader/SphereEffectShaderOpenGl.cpp (2)
14-14: LGTM: Include directive properly added
The addition of <cstring> header is appropriate for the use of std::memcpy in the code.
Line range hint 52-150: Verify shader compatibility with OSMesa
Given that this PR introduces offscreen rendering support through OSMesa, please verify that:
- The OpenGL ES 3.2 shader version (320 es) is supported by OSMesa
- All shader operations are compatible with offscreen rendering context
✅ Verification successful
OpenGL ES 3.2 shaders are compatible with OSMesa
The verification shows that:
- OSMesa version checks in the codebase (
standalone/main.cppandjvm/src/main/cpp/jni/OSMesa.cpp) indicate proper version handling for OSMesa 11.2+ - All shaders consistently use OpenGL ES 3.2 (320 es), which is supported by OSMesa 11.2 and later versions
- The shader operations used (matrix transformations, texture sampling, etc.) are standard operations supported in offscreen rendering
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for OpenGL ES version compatibility in shader code
# Search for OpenGL ES version declarations in shaders
echo "Checking shader versions in the codebase:"
rg "OMMVersionedGlesShaderCode\(\s*320\s+es" -A 2
# Look for OSMesa version checks or configurations
echo -e "\nChecking OSMesa version configurations:"
rg "OSMESA_.*_VERSION" --type cpp
Length of output: 13053
jvm/pom.xml (1)
53-56: Verify if the Ninja generator bug is still present
The comment mentions a bug in cmake-maven-plugin regarding Ninja generator quoting. This should be verified against the latest plugin version.
✅ Verification successful
Let me gather more information about the CMake plugin configuration in the project.
The Ninja generator issue is still present in the latest version
The project is using the latest version (19.7.1) of cmake-maven-plugin, and the issue with Ninja generator quoting hasn't been fixed. The commented configuration correctly warns about the bug that creates bad command line with -G"Ninja" quoting. Since this is a confirmed issue in the latest version and there's no evidence of a fix, it's best to keep using the default generator as currently configured.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the Ninja generator quoting issue is fixed in latest versions
# Get latest version of cmake-maven-plugin
curl -s "https://search.maven.org/solrsearch/select?q=g:com.borasoftware.balau+AND+a:cmake-maven-plugin&rows=1&wt=json" | jq -r '.response.docs[0].latestVersion'
# Check for any related issues or PRs
gh search issues "cmake-maven-plugin Ninja generator quoting" --repo borasoftware/cmake-maven-plugin
Length of output: 273
Script:
#!/bin/bash
# Check the current cmake-maven-plugin version and configuration
grep -A 10 "cmake-maven-plugin" jvm/pom.xml
# Check if Ninja is available in the system
which ninja
# Look for any CMake related configuration files
fd -e cmake
Length of output: 978
jvm/src/main/java/io/openmobilemaps/mapscore/map/util/MapTileRenderer.java (3)
91-122: LGTM! Well-implemented tile bounding box calculation
The method is well-documented, has comprehensive parameter validation, and correctly implements the OGC standard.
141-155: LGTM! Well-implemented force rendering with proper error handling
The method effectively handles the NOT_READY state and provides a good fallback mechanism.
136-138: Address TODO comment about tile border issues
The concern about tile border artifacts due to per-tile camera/layer updates is valid. Consider:
- Adding a test case specifically for tile border consistency
- Implementing a mechanism to update camera/layer state only when necessary
- Documenting any known limitations or artifacts at tile borders
Let's check if there are any existing tests for tile border consistency:
✅ Verification successful
Based on the gathered evidence, I can now provide a final response:
Tile border artifacts are unlikely due to existing safeguards
The concern about tile border artifacts from per-tile camera/layer updates appears to be mitigated by the current implementation:
- The codebase has visual regression tests (
assertImageMatchesGolden) that would catch tile border inconsistencies - The
OffscreenMapRendererTestspecifically tests tile rendering across multiple adjacent tiles - Layer and camera updates are managed at a higher level in
MapScene.cppwhich ensures consistent state across the entire view
The TODO comment can be safely removed as the implementation has proven to be reliable through existing test coverage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tile border-related tests
rg -B 2 -A 5 "border|edge|artifact" "jvm/src/test"
Length of output: 50
Script:
#!/bin/bash
# Let's search more broadly for tile-related tests and rendering code
rg -B 2 -A 5 "renderTile|drawFrame" "jvm/src/test"
# Also search for any existing tile-related tests
rg -B 2 -A 5 "tile" "jvm/src/test"
# Let's also check the implementation to understand the rendering logic better
ast-grep --pattern 'drawFrame($$$) { $$$ }'
Length of output: 8230
Script:
#!/bin/bash
# Let's check the implementation of MapTileRenderer to understand the tile rendering logic
rg -B 5 -A 10 "class MapTileRenderer"
# Also check for any existing visual tests that compare rendered images
rg -B 2 -A 5 "assertImageMatches|compareImages"
# Look for any existing documentation about rendering behavior
rg -B 2 -A 5 "camera.*update|layer.*update"
Length of output: 14806
shared/src/map/layers/objects/Circle2dLayerObject.cpp (2)
Line range hint 15-22: LGTM: Clean initialization with proper 2D/3D handling
The addition of <cmath> is well-justified for the trigonometric functions needed in 3D transformations. The constructor properly initializes all components with clear conditional logic for 2D/3D modes.
Line range hint 39-45: Verify 3D quad positioning
The quad frame is set with z=0 even in 3D mode. This might be intentional, but it's worth verifying if the z-coordinate should be adjusted for proper 3D rendering.
✅ Verification successful
Based on the examination of the Circle2dLayerObject implementation and the codebase context, I can now provide a final response:
The z=0 quad frame is correct and intentional in both 2D and 3D modes
The Circle2dLayerObject implementation shows that the z-coordinate handling is working as intended:
- In 2D mode, both the quad frame and origin use z=0 as expected
- In 3D mode, while the quad frame vertices use z=0, the object's origin is properly transformed for spherical coordinates (origin.x/y/z are set with trigonometric calculations)
- This pattern allows the quad to be properly positioned on the sphere's surface while maintaining its shape integrity
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar patterns in other 3D object implementations
ast-grep --pattern 'setFrame($quad, $_, Vec3D($_, $_, 0.0), $_)'
Length of output: 66
Script:
#!/bin/bash
# Let's search for the Circle2dLayerObject class implementation to understand the context better
rg -l "class Circle2dLayerObject"
# Search for any usage of Vec3D with z-coordinate in the codebase
rg "Vec3D.*,\s*[^,]+\)" -A 2
# Search for other layer objects to see their 3D handling
fd "LayerObject" --type f
Length of output: 65678
Script:
#!/bin/bash
# Let's search for Circle2dLayerObject implementation and its usage of z-coordinate
rg -A 5 "Circle2dLayerObject.*setFrame"
# Search for other layer objects' handling of z-coordinate in 3D mode
rg -A 5 "is3d.*\{.*z\s*=\s*"
# Look for any documentation about 3D rendering in the codebase
rg -l "3[dD].*render|render.*3[dD]"
Length of output: 1817
Script:
#!/bin/bash
# Let's examine the Circle2dLayerObject implementation specifically
cat shared/src/map/layers/objects/Circle2dLayerObject.cpp
# Look for any 3D rendering documentation in the changelog
cat CHANGELOG.md
# Check the implementation of other 2D layer objects for comparison
rg -A 10 "setFrame.*Vec3D.*is3d\s*\{" --type cpp
Length of output: 9636
jvm/src/main/cpp/jni/OSMesa.cpp (4)
1-8: LGTM: Headers and includes are appropriate
The necessary headers are included, and GL_GLEXT_PROTOTYPES is correctly defined before including osmesa.h.
144-154: Previous review comments still apply
The cleanup safety concerns identified in the previous review remain valid.
119-142:
Investigate OSMesa buffer behavior and add error checking
The comment about unexpected behavior with OSMesa's buffer needs investigation, and GL operations lack error checking.
Let's verify the OSMesa buffer behavior:
Add proper error checking:
glFinish();
+ GLenum error = glGetError();
+ if (error != GL_NO_ERROR) {
+ // Throw GL error as JNI exception
+ return;
+ }
if (state->fbo != 0) {
glBindFramebuffer(GL_READ_FRAMEBUFFER, state->fbo);
glBindFramebuffer(GL_DRAW_FRAMEBUFFER, 0);
glBlitFramebuffer(0, 0, state->width, state->height, 0, 0, state->width, state->height, GL_COLOR_BUFFER_BIT, GL_NEAREST);
+ error = glGetError();
+ if (error != GL_NO_ERROR) {
+ // Throw GL error as JNI exception
+ return;
+ }55-82:
Improve version handling and type safety
The current implementation has several concerns:
- The fallback case with "Just hope" comment needs proper error handling
- Type punning with
(jlong)stateis unsafe on some architectures - Version-specific code paths lack validation
Consider these improvements:
JNIEXPORT jlong JNICALL Java_io_openmobilemaps_mapscore_graphics_util_OSMesa_createContext(JNIEnv *env, jclass) {
JNIOSMesaState *state = new (std::nothrow) JNIOSMesaState{};
if (!state) {
// Throw Java OutOfMemoryError
return 0;
}
+ // Get OSMesa version
+ GLint version[2] = {0, 0};
+ OSMesaGetIntegerv(OSMESA_MAJOR_VERSION, &version[0]);
+ OSMesaGetIntegerv(OSMESA_MINOR_VERSION, &version[1]);
+
+ if (version[0] < 3) {
+ delete state;
+ // Throw exception: "Unsupported OSMesa version"
+ return 0;
+ }
// ... context creation ...
- return (jlong)state; // "type-punning" aka. a lazy hack
+ return static_cast<jlong>(reinterpret_cast<uintptr_t>(state));
}Likely invalid or redundant comment.
standalone/main.cpp (1)
201-238:
Improve framebuffer error handling.
The function should clean up resources and return an error state when framebuffer initialization fails.
- auto fbStatus = glCheckFramebufferStatus(GL_DRAW_FRAMEBUFFER);
- switch (fbStatus) {
+ const auto fbStatus = glCheckFramebufferStatus(GL_DRAW_FRAMEBUFFER);
+ if (fbStatus != GL_FRAMEBUFFER_COMPLETE) {
+ std::cerr << "Framebuffer initialization failed: ";
+ switch (fbStatus) {
// ... existing cases ...
+ }
+ glDeleteFramebuffers(1, &fbo);
+ glDeleteRenderbuffers(2, rbo);
+ return MSAAState{0, {0, 0}}; // Return invalid state
}Likely invalid or redundant comment.
jvm/src/main/java/io/openmobilemaps/mapscore/map/util/Tiled2dMapVectorLayerBuilder.java (3)
65-68: Validate layerName is not null or empty in withLayerName method.
The withLayerName method sets the layerName without checking if it's null or empty. An invalid layerName could cause issues downstream.
141-149:
Reset ByteBuffer position after writing data in allocateDirect method.
After writing data into the ByteBuffer using buf.put(encoded);, the buffer's position is at the end. Before it can be read from the beginning, you should reset the position to zero by calling buf.flip();. Not doing so may result in incorrect data being read elsewhere.
Apply this diff to reset the buffer position:
private static ByteBuffer allocateDirect(String data) {
if (data == null) {
return null;
}
byte[] encoded = data.getBytes();
var buf = ByteBuffer.allocateDirect(encoded.length);
buf.put(encoded);
+ buf.flip();
return buf;
}Likely invalid or redundant comment.
197-200:
Handle potential null values in loadGeojson method.
In the loadGeojson method, if loadGeojson.apply(sourceName, url) returns null, ensure that the LoaderStatus is set appropriately to indicate an error.
Apply this diff to adjust the status based on the geoJSON data's existence:
public Future<DataLoaderResult> loadGeojson(
@NotNull String sourceName, @NotNull String url) {
String geoJson = loadGeojson.apply(sourceName, url);
return wrapDataLoaderResult(geoJson);
}Ensure the wrapDataLoaderResult method handles null data as shown in the previous comment.
Likely invalid or redundant comment.
jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/HttpDataLoader.java (2)
72-72: Include etag in HTTP Request Headers
The methods accept an etag parameter but do not use it in the HTTP requests. To leverage caching and conditional requests, consider adding an If-None-Match header to the request if the etag is provided.
Apply this diff to include the etag in the request headers:
In loadData:
var requestBuilder = HttpRequest.newBuilder(uri);
+if (etag != null) {
+ requestBuilder.header("If-None-Match", etag);
+}
var request = requestBuilder.build();Similarly, update loadDataAsync, loadTexture, and loadTextureAsync methods.
Also applies to: 93-93, 114-114, 135-135
67-67: Consider Implementing Caching Mechanism
The // TODO: cache? comment suggests adding caching to improve performance by avoiding redundant network requests for the same resources.
Would you like assistance in implementing a basic caching strategy, perhaps leveraging HttpClient's caching features or introducing an in-memory cache?
jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/FontLoader.java (3)
78-80: Log Exceptions for Better Debugging
In the loadFont(String fontName) method, exceptions are caught but not logged. This can make debugging more difficult when an error occurs. Consider logging the exception details.
Apply this diff to add logging:
} catch (Exception e) {
+ logger.log(Level.WARNING, "Failed to load font: " + fontName, e);
return new FontLoaderResult(null, null, LoaderStatus.ERROR_OTHER);
}123-125: Log Full Exception Stack Trace for Better Diagnostics
In the readFont method, only the exception's string representation is logged. Logging the full stack trace provides more detailed information, which is helpful for troubleshooting.
Apply this diff to improve logging:
} catch (IOException | FontJsonManifestReader.InvalidManifestException e) {
- logger.warning(e.toString());
+ logger.log(Level.WARNING, "Error reading font data", e);
return new FontLoaderResult(null, null, LoaderStatus.ERROR_OTHER);
}107-108: Consider Moving DPI Scaling to Shader Code
Adjusting the font size for render resolution in Java code may feel clunky. Consider handling DPI scaling in the shader to simplify the Java code and potentially improve rendering efficiency.
jvm/src/main/java/io/openmobilemaps/mapscore/map/util/Tiled2dMapVectorLayerBuilder.java
Show resolved
Hide resolved
jvm/src/main/java/io/openmobilemaps/mapscore/map/util/Tiled2dMapVectorLayerBuilder.java
Show resolved
Hide resolved
jvm/src/main/java/io/openmobilemaps/mapscore/map/util/Tiled2dMapVectorLayerBuilder.java
Show resolved
Hide resolved
jvm/src/main/java/io/openmobilemaps/mapscore/map/util/Tiled2dMapVectorLayerBuilder.java
Show resolved
Hide resolved
jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/LocalDataLoader.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
standalone/readme.md (4)
7-7: Fix hyphenation in "platform specific".Add a hyphen between "platform" and "specific" as it's being used as a compound modifier.
-without the platform specific bindings. +without the platform-specific bindings.🧰 Tools
🪛 LanguageTool
[uncategorized] ~7-~7: When ‘platform-specific’ is used as a modifier, it is usually spelled with a hyphen.
Context: ... be used directly from C++, without the platform specific bindings. Currently, this can be built...(SPECIFIC_HYPHEN)
24-27: Add language specification to code block and consider version requirements.
- Add shell language specification to the code block.
- Consider specifying minimum version requirements for critical dependencies.
-``` +```shell apt install --no-install-recommends cmake make clang libgl-dev libgles-dev libosmesa6-dev<details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint (0.37.0)</summary> 25-25: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> --- `31-37`: **Enhance build instructions with language specification and success criteria.** 1. Add shell language specification to the code block. 2. Consider adding information about expected output or how to verify successful execution. ```diff -``` +```shell cmake -B build-directory cmake --build build-directory -- testmain # run build-directory/standalone/testmain -``` +``` + +### Expected Output + +When successfully built and run, the application should...🧰 Tools
🪛 Markdownlint (0.37.0)
31-31: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
37-37: Consider adding a troubleshooting section.It would be helpful to add a troubleshooting section that covers common issues users might encounter during build or execution, particularly since this is a platform-specific implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
standalone/readme.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
standalone/readme.md
[uncategorized] ~7-~7: When ‘platform-specific’ is used as a modifier, it is usually spelled with a hyphen.
Context: ... be used directly from C++, without the platform specific bindings. Currently, this can be built...
(SPECIFIC_HYPHEN)
🪛 Markdownlint (0.37.0)
standalone/readme.md
25-25: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
31-31: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
Update version to match android build. Adapt build plugins to follow documentated procedure and avoid needlessly running the slow "kapt".
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/LocalDataLoader.java (2)
94-97: 🛠️ Refactor suggestionImprove Asynchronous Handling in
loadDataAsyncandloadTextureAsyncThe methods
loadDataAsyncandloadTextureAsynccurrently execute synchronously, which may block the calling thread and defeats the purpose of asynchronous execution. Consider using anExecutorServiceto execute these methods asynchronously.Also applies to: 124-127
109-111:⚠️ Potential issueHandle Potential Null Image in
loadTextureIf
ImageIO.read(uri.toURL())returnsnull, it could lead to aNullPointerExceptionwhen creating aBufferedImageTextureHolder.Add a null check to handle this scenario.
Apply this diff to add the null check:
try { BufferedImage image = ImageIO.read(uri.toURL()); + if (image == null) { + logger.warn("Failed to decode image from " + uri); + return new TextureLoaderResult(null, null, LoaderStatus.ERROR_OTHER, "Failed to decode image"); + } return new TextureLoaderResult( new BufferedImageTextureHolder(image), null, LoaderStatus.OK, null); } catch (FileNotFoundException e) { // ...
🧹 Nitpick comments (3)
jvm/pom.xml (2)
17-45: Consider using properties for dependency versions.Move all dependency versions to properties section for better maintainability and consistency.
<properties> <!-- existing properties --> + <jackson.version>2.16.1</jackson.version> + <junit.version>5.11.1</junit.version> + <slf4j.version>2.0.16</slf4j.version> </properties> <dependencies> <!-- in each dependency --> <dependency> <groupId>com.fasterxml.jackson.core</groupId> <artifactId>jackson-databind</artifactId> - <version>2.17.2</version> + <version>${jackson.version}</version> </dependency> <!-- similar changes for junit and slf4j dependencies -->
169-185: Consider separating debug and release libraries.Currently, both debug and release libraries are copied to the same directory, which might cause conflicts. Consider:
- Using different target paths for debug and release builds
- Adding a classifier to distinguish between debug and release artifacts
<configuration> <outputDirectory>${project.build.outputDirectory}</outputDirectory> <resources> <resource> <directory>${project.build.directory}/build-native-release/jvm</directory> <includes> <include>libmapscore_jni_*.so</include> </includes> - <targetPath>native/</targetPath> + <targetPath>native/release/</targetPath> </resource> <resource> <directory>${project.build.directory}/build-native-debug/jvm</directory> <includes> <include>libmapscore_jni_*.so</include> </includes> - <targetPath>native/</targetPath> + <targetPath>native/debug/</targetPath> </resource> </resources> </configuration>jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/FontLoader.java (1)
43-43: Consider adding more context about the DPI calculationThe DPI calculation uses Android's DisplayMetrics.density as reference (160.0). Consider adding a more detailed explanation in the documentation about why this specific value is used and its implications for non-Android environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
jvm/pom.xml(1 hunks)jvm/src/main/java/android/util/Log.java(1 hunks)jvm/src/main/java/io/openmobilemaps/mapscore/MapsCore.java(1 hunks)jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/FontLoader.java(1 hunks)jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/HttpDataLoader.java(1 hunks)jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/LocalDataLoader.java(1 hunks)jvm/src/test/java/io/openmobilemaps/mapscore/OffscreenMapRendererTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- jvm/src/main/java/android/util/Log.java
- jvm/src/main/java/io/openmobilemaps/mapscore/MapsCore.java
- jvm/src/test/java/io/openmobilemaps/mapscore/OffscreenMapRendererTest.java
- jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/HttpDataLoader.java
🧰 Additional context used
📓 Learnings (1)
jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/FontLoader.java (1)
Learnt from: matzf
PR: openmobilemaps/maps-core#652
File: jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/FontJsonManifestReader.java:26-77
Timestamp: 2024-12-02T20:36:38.651Z
Learning: In the Java file `jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/FontJsonManifestReader.java`, the user prefers to keep the `extractFontData` method as is, without refactoring it into smaller methods.
🔇 Additional comments (7)
jvm/pom.xml (2)
23-27: Fix invalid Jackson version.
The specified Jackson version 2.17.2 doesn't exist yet. The latest stable version is 2.16.1.
48-52: Verify maven-jar-plugin version.
The specified version 3.4.2 seems incorrect. The latest version is 3.3.0.
jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/FontLoader.java (5)
1-31: LGTM! Well-structured class with proper thread-safety consideration
The class is well-designed with:
- Clear documentation of the font file format requirements
- Thread-safe font caching using ConcurrentHashMap
- Proper encapsulation with final fields
50-68: LGTM! Robust implementation with proper thread-safety and fallback handling
The method effectively combines:
- Thread-safe lazy loading using computeIfAbsent
- Comprehensive logging of font loading outcomes
- Well-structured fallback font mechanism
77-79: Add exception logging for better debugging
The catch block swallows the exception without logging it, which could make debugging difficult.
Previous review already suggested logging improvements for exception handling.
106-107: Consider moving DPI scaling to shader code
Previous review already suggested evaluating whether DPI scaling could be handled in the shader.
128-139: LGTM! Well-implemented resource management
The FontDataStreams record:
- Properly implements AutoCloseable
- Handles null streams safely
- Ensures proper cleanup of resources
jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/LocalDataLoader.java
Show resolved
Hide resolved
jvm/src/main/java/io/openmobilemaps/mapscore/map/loader/LocalDataLoader.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
shared/src/map/errormanager/ErrorManagerImpl.cpp (3)
Line range hint
19-24: Consider using std::remove_if for better iterator safetyWhile the current implementation is correct, using
std::remove_ifwould be more idiomatic and potentially more efficient.Here's a suggested improvement:
void ErrorManagerImpl::removeErrorListener(const std::shared_ptr<ErrorManagerListener> &listener) { std::lock_guard<std::recursive_mutex> lock(mutex); - auto it = std::find(listeners.begin(), listeners.end(), listener); - if (it != listeners.end()) { - listeners.erase(it); - } + listeners.erase( + std::remove_if(listeners.begin(), listeners.end(), + [&](const auto &l) { return l == listener; }), + listeners.end()); }
Line range hint
44-57: Consider using std::erase_if for cleaner error removalThe manual iteration in
removeAllErrorsForLayercould be simplified using C++20'sstd::erase_if.Here's a modern C++ approach:
void ErrorManagerImpl::removeAllErrorsForLayer(const std::string &layerName) { std::lock_guard<std::recursive_mutex> lock_guard(mutex); - bool hasChanges = false; - for (auto it = tiledLayerErrors.cbegin(), next_it = it; it != tiledLayerErrors.cend(); it = next_it) - { - auto const &[url, error] = *it; - ++next_it; - if (error.layerName == layerName) { - tiledLayerErrors.erase(it); - hasChanges = true; - } - } - - if (hasChanges) + const auto erased = std::erase_if(tiledLayerErrors, + [&](const auto &pair) { return pair.second.layerName == layerName; }); + if (erased > 0) notifyListeners(); }Note: If C++20 is not available, the current implementation is a correct way to handle iterator invalidation.
Line range hint
67-75: Consider reserving vector capacity for performanceIn
notifyListeners, we can optimize the vector allocation since we know the final size.void ErrorManagerImpl::notifyListeners() { std::lock_guard<std::recursive_mutex> lock_guard(mutex); std::vector<TiledLayerError> errors; + errors.reserve(tiledLayerErrors.size()); for (auto const &[url, error] : tiledLayerErrors) { errors.push_back(error); } for (const auto &l : listeners) { l->onTiledLayerErrorStateChanged(errors); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
shared/src/map/errormanager/ErrorManagerImpl.cpp(1 hunks)
🔇 Additional comments (2)
shared/src/map/errormanager/ErrorManagerImpl.cpp (2)
12-12: LGTM: Required header for std::find
The addition of the <algorithm> header is necessary as it provides the std::find algorithm used in removeErrorListener.
Line range hint 1-75: Thread safety implementation looks robust
The implementation demonstrates good thread-safety practices:
- Consistent use of recursive_mutex across all public methods
- Proper RAII lock management with std::lock_guard
- Safe notification pattern with data copying before listener callbacks
Let's verify no mutex-related issues exist in the codebase:
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Thread safety implementation is correctly implemented
The implementation shows robust thread safety practices with consistent mutex usage:
- All public methods are protected by
std::lock_guardwith a singlerecursive_mutex - No direct
.lock()or.unlock()calls found inErrorManagerImpl - No other implementations of
ErrorManagerfound that might bypass thread safety - The notification pattern safely copies data before callbacks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential thread safety issues
# Look for mutex usage without lock_guard
rg -n "std::mutex|std::recursive_mutex" --type cpp | rg -v "lock_guard"
# Look for direct mutex operations that might indicate manual locking
rg -n "\.lock\(\)|\.unlock\(\)" --type cpp
# Look for other ErrorManager implementations that might need similar thread safety
ast-grep --pattern 'class $_ : public ErrorManager'
Length of output: 31102
matzf
force-pushed
the
feature/jvm-lib
branch
from
8431fd0 to
c31256f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
shared/src/map/layers/line/LineLayer.cpp (2)
Line range hint
208-237: Add null checks for camera accessThe method should verify that
getCamera()doesn't return nullptr before accessing it. This could lead to a crash in edge cases.for (auto const &lineTuple : lines) { for (auto config : lineTuple.second->getRenderConfig()) { if (!lineTuple.first->getCoordinates().empty()) { + auto camera = mapInterface->getCamera(); + if (!camera) continue; std::vector<float> modelMatrix = - mapInterface->getCamera()->getInvariantModelMatrix(lineTuple.first->getCoordinates()[0], false, false); + camera->getInvariantModelMatrix(lineTuple.first->getCoordinates()[0], false, false); renderPassObjectMap[renderPassIndex].push_back( std::make_shared<RenderObject>(config->getGraphicsObject())); } } }
Line range hint
431-443: Consider adding thread synchronizationThe method could have a race condition if multiple threads modify the touch handler simultaneously. Consider adding a mutex to protect the touch handler registration/removal operations.
+class LineLayer { +private: + std::mutex touchHandlerMutex; + // ... other members ... +}; void LineLayer::setLayerClickable(bool isLayerClickable) { + std::lock_guard<std::mutex> lock(touchHandlerMutex); if (this->isLayerClickable == isLayerClickable) return; this->isLayerClickable = isLayerClickable; if (mapInterface) { if (isLayerClickable) { mapInterface->getTouchHandler()->addListener(shared_from_this()); } else { mapInterface->getTouchHandler()->removeListener(shared_from_this()); } } }
🧹 Nitpick comments (1)
shared/src/map/layers/line/LineLayer.cpp (1)
Line range hint
48-76: Consider using erase-remove idiom in a single lineThe implementation is correct and thread-safe. However, for better readability and to follow modern C++ idioms more closely, consider combining the erase and remove operations.
- addingQueue.erase(std::remove(addingQueue.begin(), addingQueue.end(), line), addingQueue.end()); + addingQueue.erase(std::remove(std::begin(addingQueue), std::end(addingQueue), line), std::end(addingQueue));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
shared/src/map/errormanager/ErrorManagerImpl.cpp(1 hunks)shared/src/map/layers/line/LineLayer.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- shared/src/map/errormanager/ErrorManagerImpl.cpp
🔇 Additional comments (2)
shared/src/map/layers/line/LineLayer.cpp (2)
22-22: LGTM: Appropriate include for STL algorithm usage
The addition of the <algorithm> header is necessary for using std::remove in the line removal operations.
Line range hint 419-429: LGTM: Proper handling of masking object updates
The implementation correctly updates both mask references and ensures the rendering state is properly invalidated.
Include <algorithm> where necessary. Build fails on github runner with libstc++ version 14.
matzf
force-pushed
the
feature/jvm-lib
branch
from
c31256f to
488a609
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
android/src/main/cpp/graphics/OpenGlHelper.h (2)
25-42: Consider optimization and documentation improvementsThe error string conversion is well-implemented. Consider these enhancements:
- Mark the method as
constexprfor potential compile-time evaluation- Add documentation explaining the purpose and return format
Here's the suggested improvement:
+ /** + * Converts an OpenGL error code to a human-readable string. + * @param errorCode The GL error code from glGetError() + * @return A string representation of the error code + */ - static std::string errorString(GLenum errorCode) { + static constexpr std::string errorString(GLenum errorCode) {
Line range hint
1-42: Consider platform-specific error handling needsGiven this PR's focus on offscreen rendering and cross-platform support, consider:
- Adding platform-specific error codes for OSMesa
- Including context information in error messages (e.g., whether running in offscreen mode)
This would help diagnose platform-specific rendering issues, especially important for the planned expansion beyond Linux/amd64.
jvm/src/main/cpp/jni/GlTextureHelper.cpp (1)
32-36: Add error checking to texture deletion.While the function is simple, it should still check for GL errors to catch potential issues.
JNIEXPORT void JNICALL Java_io_openmobilemaps_mapscore_graphics_util_GlTextureHelper_deleteTexture(JNIEnv *, jclass, jint textureId) { GLuint textures[] = {(GLuint)textureId}; glDeleteTextures(1, textures); + checkGLError("glDeleteTextures"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
android/src/main/cpp/graphics/OpenGlHelper.h(1 hunks)android/src/main/cpp/graphics/shader/ColorShaderOpenGl.cpp(1 hunks)jvm/src/main/cpp/jni/GlTextureHelper.cpp(1 hunks)shared/public/Tiled2dMapSourceImpl.h(0 hunks)
💤 Files with no reviewable changes (1)
- shared/public/Tiled2dMapSourceImpl.h
🚧 Files skipped from review as they are similar to previous changes (1)
- android/src/main/cpp/graphics/shader/ColorShaderOpenGl.cpp
🧰 Additional context used
📓 Learnings (1)
jvm/src/main/cpp/jni/GlTextureHelper.cpp (2)
Learnt from: matzf
PR: openmobilemaps/maps-core#652
File: jvm/src/main/java/io/openmobilemaps/mapscore/graphics/util/GlTextureHelper.java:16-18
Timestamp: 2024-12-02T19:53:53.134Z
Learning: The private native method `createTextureARGB` in `GlTextureHelper.java` is only used internally via the public `createTexture` method, which ensures that valid data is passed. Therefore, additional input validation in `createTextureARGB` is not necessary.
Learnt from: matzf
PR: openmobilemaps/maps-core#652
File: jvm/src/main/java/io/openmobilemaps/mapscore/graphics/util/GlTextureHelper.java:16-18
Timestamp: 2024-12-02T19:53:53.134Z
Learning: In the `GlTextureHelper` class, the `textureId` parameter in `deleteTexture` is an opaque ID that cannot be validated and does not require additional input validation.
🔇 Additional comments (2)
jvm/src/main/cpp/jni/GlTextureHelper.cpp (2)
12-29:
Add error checking and ensure proper resource cleanup.
The texture creation process lacks error checking and proper resource cleanup in case of failures.
Apply this diff to add error handling and RAII-style resource management:
+ class TextureRAII {
+ GLuint id = 0;
+ public:
+ TextureRAII() { glGenTextures(1, &id); }
+ ~TextureRAII() { if (id) glDeleteTextures(1, &id); }
+ GLuint release() { GLuint tmp = id; id = 0; return tmp; }
+ operator GLuint() const { return id; }
+ };
- GLuint textureId;
- glGenTextures(1, &textureId);
+ TextureRAII textureId;
+ if (!textureId) {
+ throw std::runtime_error("Failed to generate texture");
+ }
glBindTexture(GL_TEXTURE_2D, textureId);
+ checkGLError("glBindTexture");
glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR);
+ checkGLError("glTexParameteri MIN_FILTER");
glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR);
+ checkGLError("glTexParameteri MAG_FILTER");
glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE);
+ checkGLError("glTexParameteri WRAP_S");
glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);
+ checkGLError("glTexParameteri WRAP_T");
const jsize outLen = env->GetArrayLength(data);
jint *dataBuf = env->GetIntArrayElements(data, NULL);
+ if (!dataBuf) {
+ throw std::runtime_error("Failed to get array elements");
+ }
glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA8, (GLsizei)width, (GLsizei)height, 0, GL_BGRA, GL_UNSIGNED_BYTE, dataBuf);
+ checkGLError("glTexImage2D");
env->ReleaseIntArrayElements(data, dataBuf, JNI_ABORT);
- return (jint)textureId;
+ return (jint)textureId.release();Likely invalid or redundant comment.
24-24: Clarify the image orientation comment.
The XXX comment indicates uncertainty about the image orientation. This should be investigated and documented properly to prevent potential issues.
Let's verify the image orientation handling in the Java side:
✅ Verification successful
Let me analyze the image orientation handling in the codebase based on the search results:
-
The comment is in
GlTextureHelper.cppwhich is called fromBufferedImageTextureHolder.javato create OpenGL textures from Java BufferedImage. -
Looking at the flow:
BufferedImagedata is read usinggetRGB()inGlTextureHelper.java- The raw ARGB data is passed to native
createTextureARGB()method - The native code handles the texture creation
Let me check the native implementation to understand the texture creation:
Let me check how the image data is handled in the Java side, particularly in OSMesa which seems to handle image flipping:
Now I can provide a complete explanation about the image orientation comment:
The XXX comment can be removed as the orientation behavior is well understood
The image is not upside down because:
- In OpenGL, texture coordinates have (0,0) at the bottom-left, while Java's BufferedImage has (0,0) at the top-left
- When reading from BufferedImage using
getRGB(), the data comes in top-to-bottom order - When creating the texture with
glTexImage2D(), OpenGL expects the data in bottom-to-top order - These two "inversions" cancel each other out, resulting in correct orientation without explicit flipping
This is in contrast to the OSMesa rendering path which does require explicit flipping (as seen in setARGBflipV()), because it reads the OpenGL framebuffer which is in bottom-to-top order and needs to be flipped to match BufferedImage's top-to-bottom order.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for image handling code in Java
rg -A 5 "BufferedImage|createTexture" --type java
Length of output: 31168
Script:
#!/bin/bash
# Check the native texture creation implementation
cat jvm/src/main/cpp/jni/GlTextureHelper.cpp
Length of output: 1717
Script:
#!/bin/bash
# Check OSMesa implementation for image orientation handling
cat jvm/src/main/java/io/openmobilemaps/mapscore/graphics/util/OSMesa.java
Length of output: 3421
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 7
🔭 Outside diff range comments (3)
bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/common/Vec2D.kt (1)
Line range hint
1-1:⚠️ Pattern of modifications to autogenerated filesThis is another instance of modifying an autogenerated file. The same pattern of initializing
tempResultappears across multiple files (RectCoord.kt,Vec2F.kt,Vec2D.kt, etc.).Instead of modifying these files individually:
- Identify the common pattern in the Djinni templates
- Make a single fix in the template(s)
- Regenerate all affected files
This ensures consistency and prevents changes from being lost during code generation.
Would you like assistance in:
- Locating all affected autogenerated files?
- Identifying the relevant Djinni templates?
- Proposing a template-level fix?
Also applies to: 12-12
bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/common/Quad2dD.kt (1)
Line range hint
1-1: Modifications to autogenerated files should be made at the sourceThis file is marked as autogenerated with the comment "AUTOGENERATED FILE - DO NOT MODIFY!". While initializing
tempResultis a good practice, the change should be made in the Djinni template/generator rather than directly in the generated file to ensure it persists across regeneration.Consider updating the Djinni template to initialize
tempResultto 0 in all generatedcompareTomethods.Also applies to: 14-14
bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/common/Vec3D.kt (1)
Line range hint
13-38: Consider improving the Djinni template for more idiomatic Kotlin codeThe current comparison implementation is verbose and could be simplified. Since this is an auto-generated file, the improvement should be made in the Djinni template.
Consider updating the Djinni template to generate a more idiomatic Kotlin implementation using the built-in comparison operators:
override fun compareTo(other: Vec3D): Int = compareValuesBy(this, other, { it.x }, { it.y }, { it.z } )This approach would be:
- More concise and readable
- Less error-prone
- More maintainable
- More performant (fewer variable assignments)
Would you like me to help create a PR for updating the Djinni template?
🧹 Nitpick comments (31)
shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonPatternTile.cpp (2)
121-123: Consider clamping alpha multiplication.The statement 'opacities[styleGroupId][index] = alpha * opacity' might yield out-of-bound values if “alpha” or “opacity” is not clamped to [0.0, 1.0]. Despite presumably valid range checks elsewhere, consider verifying or clamping here to avoid unexpected intermediate values.
333-340: Potential reuse of RenderObject instances.Building a new list of RenderObject each time is clear, but consider retaining and updating existing objects to reduce memory churn and CPU overhead in heavy rendering scenarios.
shared/public/VectorTileGeometryHandler.h (2)
144-146: Potential confusion with method naming.The method name and its intent are clear. However, it might be more descriptive to name this method “prepareTriangulatePolygons” or something similar to indicate that it initializes or prepares for the triangulation process, rather than actually beginning — especially since it returns the polygon count.
153-184: Review of triangulatePolygons(size_t i).• The overall design (split methods for beginning, single polygon triangulation, and end) is a good step toward modularity.
• Consider adding boundary checks before using polygonHoles[i] or polygonPoints[i], in case an invalid index passes in.
• The function inverts the winding order by reversing the indices (line 178). Active caution is required for cases where the geometry might already be oriented as intended or if other processes rely on a specific orientation.
• The limitHoles call (line 174) helps guard against extreme data, but you might want to log or handle scenarios in which holes are truncated for better traceability.
• This method currently merges the logic of both geometry conversion (coordinates array) and polygon indexing in a single pass. Splitting it across smaller private helper methods can further improve maintainability.bridging/android/java/io/openmobilemaps/mapscore/shared/map/layers/text/FormattedStringEntry.kt (1)
Line range hint
9-24: Simplify compareTo implementationWhile the current implementation is functionally correct, it can be simplified for better readability and maintainability.
Consider this more concise implementation:
override fun compareTo(other: FormattedStringEntry): Int { - var tempResult = 0 - tempResult = this.text.compareTo(other.text) - if (tempResult != 0) { - return tempResult - } - if (this.scale < other.scale) { - tempResult = -1; - } else if (this.scale > other.scale) { - tempResult = 1; - } else { - tempResult = 0; - } - if (tempResult != 0) { - return tempResult - } - return 0 + return this.text.compareTo(other.text).takeIf { it != 0 } + ?: this.scale.compareTo(other.scale) }However, since this is an auto-generated file, this change should be made in the source template instead.
shared/src/map/controls/DefaultTouchHandler.h (1)
47-48: LGTM! Good state machine enhancement.The addition of
ONE_FINGER_AFTER_TWOstate improves the touch handling by explicitly managing post-two-finger interactions. This prevents accidental triggers of one-finger gestures immediately after two-finger gestures complete.Consider documenting the state machine transitions in a diagram (e.g., using PlantUML or Mermaid) to help future maintainers understand the touch handling flow.
shared/src/map/controls/DefaultTouchHandler.cpp (1)
306-319: LGTM! Well-structured state transition with timeout.The transition to
ONE_FINGER_AFTER_TWOis properly handled with appropriate timeout and state reset scheduling. The use of lock guards ensures thread safety.Consider extracting the timeout scheduling logic into a helper method since it's used in multiple places:
+private void scheduleStateReset(const std::string& taskName, int32_t timeout) { + auto strongScheduler = scheduler.lock(); + if (strongScheduler) { + strongScheduler->addTask(std::make_shared<LambdaTask>( + TaskConfig(taskName, timeout, TaskPriority::NORMAL, ExecutionEnvironment::COMPUTATION), + [=] { checkState(); })); + } +}bridging/android/java/io/openmobilemaps/mapscore/shared/map/coordinates/QuadCoord.kt (1)
Line range hint
1-1: Architectural Recommendation: Update Djinni TemplatesInstead of modifying the autogenerated files directly, I recommend:
- Update the Djinni templates/generator to initialize
tempResultto 0 in all generatedcompareTomethods- Regenerate all the Kotlin files
- Consider adding a CI check to prevent direct modifications to autogenerated files
This will ensure the improvements persist across regeneration and maintain the integrity of the build process.
bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/shader/RasterShaderStyle.kt (2)
Line range hint
28-92: Consider improving the compareTo implementation in the Djinni template.While the current implementation is functionally correct, the comparison logic is quite verbose and repetitive. Consider updating the Djinni template to generate a more concise implementation using Kotlin's built-in comparison functions.
Here's a potential improvement for the Djinni template that would generate more idiomatic Kotlin code:
override fun compareTo(other: RasterShaderStyle): Int { return compareValuesBy(this, other, { it.opacity }, { it.brightnessMin }, { it.brightnessMax }, { it.contrast }, { it.saturation }, { it.gamma }, { it.brightnessShift } ) }Note: Since this is an auto-generated file, this change should be made in the Djinni template rather than directly in this file.
Line range hint
1-2: Consider adding a "do not edit" notice with guidance.The current auto-generated notice could be enhanced to guide developers on how to properly make changes through the Djinni definition file.
Consider updating the template header to something like:
// AUTOGENERATED FILE - DO NOT MODIFY! // This file was generated by Djinni from shader.djinni // To modify this file, update the Djinni definition in shader.djinni and regenerateshared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonPatternTile.h (1)
74-75: Document relationships between render collectionsThe new
renderObjectsmember adds to existing render-related collections (polygonRenderOrder,styleGroupPolygonsMap). Consider adding documentation to clarify:
- The specific purpose of
renderObjects- How it interacts with other render collections
- The lifecycle management of these objects
ios/graphics/Shader/Metal/LineShader.metal (3)
66-70: Ensure naming consistency and clarity.
The newly introduced variables 'dash_fade' and 'dash_animation_speed' are descriptive, but consider retaining a consistent naming convention (e.g., dashFade, dashAnimationSpeed) if that is the established style in the rest of the codebase.
315-315: Wrap the fmod expression in parentheses.
For clarity: fmod((t + startOffsetSegment + timeOffset), dashTotal). This is purely stylistic, but can improve readability.- const float intraDashPos = fmod(t + startOffsetSegment + timeOffset, dashTotal); + const float intraDashPos = fmod((t + startOffsetSegment + timeOffset), dashTotal);
343-354: Ensure dash boundary conditions.
Similar approach for the second dash range. Double-check that you handle the transition if 'style->dash_fade' is close to 1.0 or extremely small.shared/src/map/layers/tiled/vector/tiles/polygon/Tiled2dMapVectorPolygonTile.cpp (1)
358-358: Returning shared render object list.
generateRenderObjects() is now returning the internal vector. This simplifies usage but ensure no external modifications break internal state.shared/src/map/layers/tiled/vector/tiles/line/Tiled2dMapVectorLineTile.cpp (3)
78-78: Minor gap: consider naming consistency.
The local variable name 'zoomIdentifier' is clear, but be consistent with your naming style in other classes.
178-179: Explicit placeholders.
dashAnimationSpeed and dashFade set to zero confirm future enhancements. Possibly add a code comment referencing a planned feature.
447-447: Returning the internal render objects.
Like with polygons, returning the shared renderObjects ensures consistent usage. Mind concurrency if multiple threads read/modify simultaneously.shared/public/Value.h (1)
678-678: Spacing or formatting style.
A blank line or comment might clarify logic flow. Currently, it’s somewhat abrupt.shared/public/RenderObjectInterface.h (1)
21-24: LGTM! Consider documenting visibility behavior.The pure virtual methods for visibility control are correctly defined. Consider documenting the expected behavior of hidden objects, particularly:
- Impact on rendering pipeline
- Interaction with other properties (e.g., opacity)
- Performance implications
Adding comprehensive documentation will help implementers understand the expected behavior across different platforms and rendering contexts (OpenGL, OSMesa, etc.).
bridging/android/java/io/openmobilemaps/mapscore/shared/map/layers/line/LineStyle.kt (1)
14-15: Consider documenting property constraints and units.The new dash animation properties need clear documentation about:
- Valid ranges for
dashFadeanddashAnimationSpeed- Units of measurement (e.g., pixels/second for animation speed)
- Performance implications of animation
Would you like me to help create documentation templates for these properties?
bridging/android/java/io/openmobilemaps/mapscore/shared/map/layers/line/ShaderLineStyle.kt (1)
25-26: LGTM! Consider adding documentation for the new properties.The new properties
dashFadeanddashAnimationSpeedenhance line rendering capabilities. Since this is part of the public API, it would be helpful to add documentation comments explaining:
- The expected range of values
- The visual effect of each property
- Any default values
shared/public/RenderObject.h (1)
32-36: LGTM! Consider thread safety for visibility state.The visibility control implementation looks good. Since this might be accessed from multiple threads in an offscreen rendering context, consider:
- Adding thread safety documentation
- Marking the methods as thread-safe if appropriate
- Using std::atomic for the hidden flag if concurrent access is expected
Also applies to: 42-42
shared/src/graphics/RenderObject.cpp (1)
31-37: LGTM! Consider marking getter as const.The implementation is clean and straightforward. Consider these optimizations:
- Mark
isHidden()asconstsince it doesn't modify object state- If thread safety is needed, use std::atomic as suggested in the header review
-bool RenderObject::isHidden() { +bool RenderObject::isHidden() const { return hidden; }shared/public/LineStyle.h (1)
20-21: LGTM - Auto-generated file with correct implementationThe new fields
dashFadeanddashAnimationSpeedare properly integrated into the struct with appropriate constructor parameter handling and member initialization usingstd::move.Consider adding documentation comments for the new fields to describe their expected ranges and behavior.
Also applies to: 34-35, 47-48
djinni/map/layers/line/line.djinni (1)
37-38: Document shader line style parametersThe new shader parameters
dash_fadeanddash_animation_speedneed documentation to specify:
- Expected value ranges
- Units (e.g., seconds for animation speed)
- Behavior at edge cases
ios/graphics/Shader/LineGroupShader.swift (1)
18-18: Consider buffer lifecycle managementThe
timeMultiBuffer is properly initialized, but consider:
- Documenting the buffer's lifecycle
- Adding cleanup in deinit if needed
Also applies to: 26-26
shared/src/map/layers/tiled/vector/tiles/raster/Tiled2dMapVectorRasterTile.h (1)
55-55: Consider thread synchronization for visibility stateWhile simplifying the visibility tracking from
std::optional<bool>toboolis good, the visibility state might need thread synchronization protection in an offscreen rendering context where multiple threads could access/modify it simultaneously.Consider using
std::atomic<bool>or appropriate mutex protection if the visibility state can be accessed from multiple threads:- bool isVisible = true; + std::atomic<bool> isVisible{true};shared/public/ShaderLineStyle.h (1)
27-28: Document the new dash effect parametersThe new parameters
dashFadeanddashAnimationSpeedlack documentation explaining their purpose and expected value ranges. This is particularly important for cross-platform consistency in offscreen rendering.Consider adding documentation in the source
.djinnifile:/** * Controls the fade effect of dashed lines (range: 0.0 to 1.0) */ float dashFade; /** * Controls the animation speed of dashed lines in pixels per second */ float dashAnimationSpeed;Also applies to: 51-52, 74-75
shared/src/graphics/Renderer.cpp (1)
51-75: Excellent optimization of render state management!The introduction of the
preparedflag effectively consolidates state changes (scissoring, stencil mask) outside the render object loop, reducing redundant operations and improving performance.Consider extracting the preparation logic into a separate method for better readability:
-if (!prepared) { - if (scissoringRect) { - renderingContext->applyScissorRect(scissoringRect); - } - - if (usesStencil) { - renderingContext->preRenderStencilMask(); - } - - if (hasMask) { - maskObject->renderAsMask(renderingContext, pass->getRenderPassConfig(), vpMatrixPointer, identityMatrixPointer, origin, factor); - } - - prepared = true; -} +if (!prepared) { + prepareRenderPass(renderingContext, pass, vpMatrixPointer, identityMatrixPointer, origin, factor); + prepared = true; +}shared/src/map/layers/objects/Line2dLayerObject.cpp (1)
22-22: Consider using named constants for initializationThe style initialization uses magic numbers (0.0f). Consider defining named constants for better maintainability and clarity.
+const float DEFAULT_COLOR_VALUE = 0.0f; +const float DEFAULT_SIZE_VALUE = 0.0; - , style(ColorStateList(Color(0.0f,0.0f,0.0f,0.0f), Color(0.0f,0.0f,0.0f,0.0f)), ColorStateList(Color(0.0f,0.0f,0.0f,0.0f), Color(0.0f,0.0f,0.0f,0.0f)), 0.0, 0.0, SizeType::SCREEN_PIXEL, 0.0, std::vector<float>(), 0, 0, LineCapType::BUTT, 0.0, false, 1.0) + , style(ColorStateList(Color(DEFAULT_COLOR_VALUE,DEFAULT_COLOR_VALUE,DEFAULT_COLOR_VALUE,DEFAULT_COLOR_VALUE), + Color(DEFAULT_COLOR_VALUE,DEFAULT_COLOR_VALUE,DEFAULT_COLOR_VALUE,DEFAULT_COLOR_VALUE)), + ColorStateList(Color(DEFAULT_COLOR_VALUE,DEFAULT_COLOR_VALUE,DEFAULT_COLOR_VALUE,DEFAULT_COLOR_VALUE), + Color(DEFAULT_COLOR_VALUE,DEFAULT_COLOR_VALUE,DEFAULT_COLOR_VALUE,DEFAULT_COLOR_VALUE)), + DEFAULT_SIZE_VALUE, DEFAULT_SIZE_VALUE, SizeType::SCREEN_PIXEL, DEFAULT_SIZE_VALUE, + std::vector<float>(), 0, 0, LineCapType::BUTT, DEFAULT_SIZE_VALUE, false, 1.0)
🛑 Comments failed to post (7)
shared/public/VectorTileGeometryHandler.h (1)
148-151:
⚠️ Potential issueTypographical method name issue.
"endTringulatePolygons" appears to have a typo ("Tringulate" instead of "Triangulate"). Consider correcting this to ensure method names remain consistent and self-explanatory.
-void endTringulatePolygons() { +void endTriangulatePolygons() { polygonHoles.clear(); polygonPoints.clear(); }📝 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.void endTriangulatePolygons() { polygonHoles.clear(); polygonPoints.clear(); }ios/maps/MCFontLoader.swift (1)
30-41: 🛠️ Refactor suggestion
Consider potential thread-safety and performance improvements
The current implementation has several concerns:
- Using
DispatchQueue.main.syncfrom a background thread could lead to deadlocks if the main queue is waiting on the current thread.MainActor.assumeIsolatedassumes thread isolation but doesn't guarantee it.- Blocking initialization could impact app launch time.
- Missing error handling for cases where screen information is unavailable.
Consider this safer alternative:
- let pixelsPerInch = if Thread.isMainThread { - MainActor.assumeIsolated { - UIScreen.pixelsPerInch - } - } else { - DispatchQueue.main.sync { - MainActor.assumeIsolated { - UIScreen.pixelsPerInch - } - } - } - self.pixelsPerInch = pixelsPerInch + // Cache the value at app launch to avoid repeated main thread access + let defaultPPI: Double = 160.0 // fallback value + let ppi: Double + if Thread.isMainThread { + ppi = (try? UIScreen.main.nativeScale * 160.0) ?? defaultPPI + } else { + // Use async value or fall back to default + ppi = DispatchQueue.main.sync { + (try? UIScreen.main.nativeScale * 160.0) ?? defaultPPI + } + } + self.pixelsPerInch = ppiThis approach:
- Provides a fallback value for error cases
- Uses a simpler calculation that's less likely to cause issues
- Still maintains thread-safety but with better error handling
Committable suggestion skipped: line range outside the PR's diff.
shared/src/map/controls/DefaultTouchHandler.cpp (1)
408-415: 🛠️ Refactor suggestion
Code duplication in state reset scheduling.
This code block duplicates the timeout scheduling logic from
handleTouchUp. Consider using the suggested helper method to reduce duplication.Apply the suggested helper method here as well:
-auto strongScheduler = scheduler.lock(); -if (strongScheduler) { - strongScheduler->addTask(std::make_shared<LambdaTask>( - TaskConfig("OneFingerAfterTwoTask", TWO_FINGER_TOUCH_TIMEOUT, TaskPriority::NORMAL, ExecutionEnvironment::COMPUTATION), - [=] { checkState(); })); -} +scheduleStateReset("OneFingerAfterTwoTask", TWO_FINGER_TOUCH_TIMEOUT);Committable suggestion skipped: line range outside the PR's diff.
shared/public/ShaderLineStyle.h (1)
27-28:
⚠️ Potential issue
⚠️ Changes made to autogenerated fileThis file is marked as autogenerated. Changes should be made to the source
line.djinnifile instead of directly modifying this file.Please move these changes to the source
.djinnifile.bridging/android/jni/map/layers/line/NativeLineStyle.h (1)
28-28:
⚠️ Potential issue
⚠️ Changes made to autogenerated fileThis file is marked as autogenerated. Changes should be made to the source
.djinnifile instead of directly modifying this file.Please move these changes to the source
.djinnifile:
- Update the constructor signature
- Add the new fields
dashFadeanddashAnimationSpeedAlso applies to: 36-37
shared/src/map/layers/objects/Line2dLayerObject.cpp (2)
122-122:
⚠️ Potential issuePotential buffer size mismatch in ShaderLineStyle
The buffer size calculation uses
21 * sizeof(float)but the style structure now has 23 parameters. This could lead to buffer overflow.Update the buffer size calculation:
- auto buffer = SharedBytes((int64_t)&s, 1, 21 * sizeof(float)); + auto buffer = SharedBytes((int64_t)&s, 1, 23 * sizeof(float));Also applies to: 152-153
57-60: 💡 Codebase verification
Mixed precision usage detected - consider maintaining consistency
The codebase shows inconsistent precision usage for length calculations:
LineGroup2dLayerObject.cppusesfloatfor both normal components and line lengthLine2dLayerObject.cppmixesdoublefor normal components withfloatfor line length- Shader code (Metal, OpenGL) consistently uses
floatas expected for GPU calculationsSince the length calculation is used for rendering and shader operations which typically use
floatprecision, and given thatLineGroup2dLayerObject.cppalready usesfloatsuccessfully, the increased precision fromdoubleinLine2dLayerObject.cppappears unnecessary and inconsistent.🔗 Analysis chain
Verify precision requirements for length calculations
The change from
floattodoublefor length calculations increases precision but also memory usage. Ensure this precision is necessary for your use case.
Let me gather more context about the usage of these length calculations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other length calculations to verify consistency rg -A 2 "length.*Normal.*[xyz]"Length of output: 3140
Script:
#!/bin/bash # Search for line length calculations and their usage rg -A 3 "lineLength = " # Search for any float/double type declarations in the same files fd "Line.*LayerObject\.cpp$" --exec rg -A 1 "(float|double) \w+"Length of output: 3610
Add a java library to use the OpenGL-variant of the openmobilemaps in a non-android Java environment.
For now, this only explicitly supports offscreen rendering via OSMesa.
Currently, this can be built and run only on Linux on amd64. Supporting other platforms should be doable but will require further tweaks.
Also add a minimal standalone test application that can be run on Linux without any platform specific bindings (can anyone guess what the demo polygon layer in there represents? 😉). This has been valuable to me for diagnostics and debugging purposes. This could also be the starting point to a more generic (offscreen) map rendering regression-test tool that can be run e.g. during PRs.
--
Note: github publish workflow might still require some tweaking due the naming of the organization secrets.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
CMakeLists.txtconfiguration for building a shared library and a standalone project.Tiled2dMapSourceclass for improved tile visibility and loading management.Bug Fixes
Documentation
Tests
OffscreenMapRendererclass.Chores
.gitignoreto enhance ignored file patterns.