-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Release/2.2.0 #624
Release/2.2.0 #624
Conversation
Feature/epsg url template
WalkthroughThe recent updates encompass a range of enhancements and fixes across multiple areas of the project, including workflow adjustments, version updates, bug fixes, performance improvements, and new features. Notable changes include the renaming of environment variables, improved handling of polygon layers, enhanced logging, and the introduction of new edge fill modes for textures. Both Android and iOS platforms see significant updates, ensuring better performance and stability. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Due to this bug, holes were never simplified, and the initial z-value was used to determine whether or not vertices are kept.
# Conflicts: # CHANGELOG.md # android/gradle.properties # ios/readme.md # shared/src/MapsCoreSharedModule.cpp
setZoom will update centerPosition with wrong logic for that usecase
This reverts commit 9141281.
Build on release/-PRs, adjust credential names to match usual pattern
…are-2.0.8-merge # Conflicts: # CHANGELOG.md # android/gradle.properties # android/readme.md # ios/readme.md # shared/src/MapsCoreSharedModule.cpp
Merge 2.0.8 into 2.2.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 8
Outside diff range and nitpick comments (4)
ios/graphics/Model/Quad/Quad2d.swift (1)
Line range hint
153-171
: Refactored buffer handling insetFrame
method to improve memory management.Consider updating the method documentation to reflect the new
copyOrCreate
behavior for better clarity.shared/src/map/layers/tiled/vector/Tiled2dMapVectorSource.cpp (1)
[!TIP]
Codebase VerificationAdd error handling around performance logging calls in the
postLoadingTask
method to prevent potential disruptions in the tile processing workflow.
shared/src/map/layers/tiled/vector/Tiled2dMapVectorSource.cpp
: Add try-catch blocks aroundPERF_LOG_START
andPERF_LOG_END
calls.Analysis chain
Line range hint
50-96
: Added performance logging inpostLoadingTask
to monitor and optimize tile processing times.Ensure that appropriate error handling is in place for the logging functions to prevent any disruptions in the tile processing workflow.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify error handling in performance logging. # Test: Search for error handling related to logging. Expect: Error handling code around logging calls. rg --type cpp $'PERF_LOG_START|PERF_LOG_END'Length of output: 1165
Script:
#!/bin/bash # Description: Check the implementation of PerformanceLogger for error handling mechanisms. # Test: Search for the definition and implementation of the PerformanceLogger class. rg --type cpp 'class PerformanceLogger' -A 50Length of output: 3816
android/src/main/java/io/openmobilemaps/mapscore/map/view/MapView.kt (1)
Line range hint
75-135
: Centralized map view state updates throughupdateMapViewState
.This change enhances maintainability by centralizing state management. Consider adding detailed documentation for the
updateMapViewState
method to explain its role in state management and its impact on the map's lifecycle.Also applies to: 245-251
shared/src/map/layers/tiled/vector/Tiled2dMapVectorLayer.cpp (1)
Line range hint
357-374
: Consider optimizing the GeoJSON source handling logic.The current implementation for handling GeoJSON sources within the
initializeVectorLayer
method involves multiple checks and conditional branches. Consider refactoring this to a separate method to improve readability and maintainability. Here's a proposed refactor:void Tiled2dMapVectorLayer::initializeGeoJsonSource(const std::string& source, const std::shared_ptr<Mailbox>& sourceMailbox) { auto geoJsonSourceIt = mapDescription->geoJsonSources.find(source); if (geoJsonSourceIt != mapDescription->geoJsonSources.end()) { auto geoJsonSource = Actor<Tiled2dVectorGeoJsonSource>(sourceMailbox, mapInterface->getCamera(), mapInterface->getMapConfig(), layerConfigs[source], mapInterface->getCoordinateConverterHelper(), mapInterface->getScheduler(), loaders, selfVectorActor, layersToDecode[source], source, mapInterface->getCamera()->getScreenDensityPpi(), mapDescription->geoJsonSources.at(source), layerName); vectorTileSources[source] = geoJsonSource.strongActor<Tiled2dMapVectorSource>(); geoJsonSourceIt->second->setDelegate(geoJsonSource.weakActor<GeoJSONTileDelegate>()); } else { vectorTileSources[source] = Actor<Tiled2dMapVectorSource>(sourceMailbox, mapInterface->getMapConfig(), layerConfigs[source], mapInterface->getCoordinateConverterHelper(), mapInterface->getScheduler(), loaders, selfVectorActor, layersToDecode[source], source, mapInterface->getCamera()->getScreenDensityPpi(), layerName); } }This method can then be called within
initializeVectorLayer
to handle each GeoJSON source, simplifying the main method's logic.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (38)
- .github/workflows/android.yml (2 hunks)
- .github/workflows/sonatype.yml (2 hunks)
- CHANGELOG.md (3 hunks)
- android/build.gradle (1 hunks)
- android/gradle.properties (1 hunks)
- android/readme.md (1 hunks)
- android/src/main/cpp/graphics/OpenGlContext.cpp (1 hunks)
- android/src/main/cpp/graphics/shader/BaseShaderProgramOpenGl.cpp (1 hunks)
- android/src/main/cpp/graphics/shader/ColorPolygonGroup2dShaderOpenGl.cpp (1 hunks)
- android/src/main/java/io/openmobilemaps/mapscore/graphics/BitmapTextureHolder.kt (2 hunks)
- android/src/main/java/io/openmobilemaps/mapscore/graphics/CanvasEdgeFillMode.kt (1 hunks)
- android/src/main/java/io/openmobilemaps/mapscore/graphics/GLThread.kt (2 hunks)
- android/src/main/java/io/openmobilemaps/mapscore/graphics/GlTextureView.kt (3 hunks)
- android/src/main/java/io/openmobilemaps/mapscore/map/view/MapView.kt (5 hunks)
- ios/graphics/Helpers/MTLDevice+Helpers.swift (2 hunks)
- ios/graphics/Model/Line/LineGroup2d.swift (1 hunks)
- ios/graphics/Model/Polygon/Polygon2d.swift (1 hunks)
- ios/graphics/Model/Polygon/PolygonGroup2d.swift (1 hunks)
- ios/graphics/Model/Polygon/PolygonPatternGroup2d.swift (1 hunks)
- ios/graphics/Model/Quad/Quad2d.swift (2 hunks)
- ios/maps/MCMapView.swift (1 hunks)
- ios/maps/MCTextureLoader.swift (3 hunks)
- ios/readme.md (1 hunks)
- shared/public/PerformanceLogger.cpp (1 hunks)
- shared/public/PerformanceLogger.h (1 hunks)
- shared/public/Tiled2dMapSourceImpl.h (1 hunks)
- shared/public/Tiled2dMapVectorLayerConfig.h (1 hunks)
- shared/public/Tiled2dMapVectorStyleParser.h (1 hunks)
- shared/public/Value.h (2 hunks)
- shared/src/MapsCoreSharedModule.cpp (1 hunks)
- shared/src/map/camera/MapCamera2d.cpp (1 hunks)
- shared/src/map/layers/polygon/PolygonLayer.cpp (2 hunks)
- shared/src/map/layers/tiled/vector/Tiled2dMapVectorLayer.cpp (3 hunks)
- shared/src/map/layers/tiled/vector/Tiled2dMapVectorSource.cpp (5 hunks)
- shared/src/map/layers/tiled/vector/geojson/GeoJsonParser.h (1 hunks)
- shared/src/map/layers/tiled/vector/geojson/geojsonvt/geojsonvt.hpp (3 hunks)
- shared/src/map/layers/tiled/vector/geojson/geojsonvt/tile.hpp (1 hunks)
- shared/src/map/layers/tiled/vector/sourcemanagers/Tiled2dMapVectorSourceTileDataManager.cpp (2 hunks)
Files not reviewed due to errors (2)
- ios/readme.md (no review received)
- android/readme.md (no review received)
Files skipped from review due to trivial changes (4)
- android/gradle.properties
- android/src/main/cpp/graphics/shader/ColorPolygonGroup2dShaderOpenGl.cpp
- android/src/main/java/io/openmobilemaps/mapscore/graphics/CanvasEdgeFillMode.kt
- shared/src/MapsCoreSharedModule.cpp
Additional context used
SwiftLint
ios/graphics/Model/Polygon/PolygonGroup2d.swift
[Warning] 121-121:
where
clauses are preferred over a singleif
inside afor
(for_where)
[Warning] 136-136: Lines should not have trailing semicolons (trailing_semicolon)
[Warning] 137-137: Lines should not have trailing semicolons (trailing_semicolon)
ios/graphics/Model/Polygon/PolygonPatternGroup2d.swift
[Warning] 152-152:
where
clauses are preferred over a singleif
inside afor
(for_where)
[Warning] 164-164: Lines should not have trailing semicolons (trailing_semicolon)
[Warning] 165-165: Lines should not have trailing semicolons (trailing_semicolon)
ios/maps/MCTextureLoader.swift
[Warning] 109-109: Prefer using UTF-8 encoded strings when converting between
String
andData
(non_optional_string_data_conversion)
[Warning] 115-115: Prefer using UTF-8 encoded strings when converting between
String
andData
(non_optional_string_data_conversion)
[Warning] 124-124: Prefer using UTF-8 encoded strings when converting between
String
andData
(non_optional_string_data_conversion)
[Warning] 254-254: Prefer using UTF-8 encoded strings when converting between
String
andData
(non_optional_string_data_conversion)
[Warning] 260-260: Prefer using UTF-8 encoded strings when converting between
String
andData
(non_optional_string_data_conversion)
[Warning] 269-269: Prefer using UTF-8 encoded strings when converting between
String
andData
(non_optional_string_data_conversion)
[Warning] 44-44: Initializing an optional variable with nil is redundant (redundant_optional_initialization)
[Warning] 195-195: Initializing an optional variable with nil is redundant (redundant_optional_initialization)
LanguageTool
CHANGELOG.md
[style] ~32-~32: Unless you want to emphasize “not”, use “cannot” which is more common. (CAN_NOT_PREMIUM)
Context: ...10.04.2024) - Log exception when a font can not be loaded ## Version 2.0.5 (08.04.2024...
[style] ~47-~47: To make your text as clear as possible to all readers, do not use this foreign term. Possible alternatives are “not publicly”, “privately”, “secretly”, “confidentially”, and “behind closed doors”. If you mean the English word ‘camera’, a pronoun or a determiner is probably missing before the word. (IN_CAMERA)
Context: ...OS Polygon Pattern Shader - Fixes a bug in camera restricted bounds calculations - iOS Qu...
[misspelling] ~97-~97: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’. (EN_A_VS_AN)
Context: ...ttempting to load empty texture - Fixes a error in MapCamera2d::getPaddingCorrect...
[uncategorized] ~106-~106: The adjective or adverb “off-screen” is spelled with a hyphen. (ON_OFF_SCREEN_HYPHEN)
Context: ...d raster layer density dependent - Adds off screen rendering helpers - Many improvements a...ios/readme.md
[style] ~27-~27: Consider a more expressive alternative. (DO_ACHIEVE)
Context: ...add this package to your App target. To do this, follow the step by step tutorial ...
[grammar] ~27-~27: Did you mean the adjective or adverb “step-by-step” (spelled with hyphens)? (STEP_BY_STEP_HYPHEN)
Context: ...your App target. To do this, follow the step by step tutorial [Adding Package Dependencies t...
[uncategorized] ~86-~86: A comma might be missing here. (AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
Context: ...ides/vector-tiles-standards/). To add a layer simply reference the style URL. ```swi...
[uncategorized] ~119-~119: You might be missing the article “the” here. (AI_EN_LECTOR_MISSING_DETERMINER_THE)
Context: ...uld be affected by camera movements. In case of user interaction, the given callback...
[grammar] ~136-~136: The singular proper name ‘Line’ must be used with a third-person or a past tense verb. (HE_VERB_AGR)
Context: ...yer?.asLayerInterface()) ``` #### Line layer A line layer can be added to the mapVie...
[uncategorized] ~137-~137: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...added to the mapView as well. Using the MCLineFactory a LineInfo object can be created. The w...
[uncategorized] ~261-~261: This verb does not appear to agree with the subject. Consider using a different form. (AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
Context: ...idging interface between Kotlin and C++ are defined in the djinni files under [djin...
[style] ~265-~265: This phrasing can be wordy. (AS_WELL_AS_AND)
Context: ...e Kotlin bindings, the C++ header files as well as all the Objective C glue code. ### Bui...
[misspelling] ~265-~265: This word is normally spelled with a hyphen. (EN_COMPOUNDS_OBJECTIVE_C)
Context: ...the C++ header files as well as all the Objective C glue code. ### Building the iOS Packag...android/readme.md
[style] ~48-~48: This phrasing can be wordy. (AS_WELL_AS_AND)
Context: ...e Kotlin bindings, the C++ header files as well as all the JNI glue code. ### Building th...
[style] ~112-~112: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing. (REP_NEED_TO_VB)
Context: ...EPSG 3857). Additionally, theMapView
needs to be registered to a lifecycle. ### Disp...
[grammar] ~343-~343: The name of this brand needs to be capitalized. (ON_SKYPE)
Context: ...ing the Camera The camera position and zoom can easily be adjusted by manipulating ...
Markdownlint
CHANGELOG.md
3-3: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
4-4: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
15-15: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
22-22: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
26-26: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
31-31: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
34-34: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
38-38: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
41-41: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
45-45: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
51-51: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
63-63: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
68-68: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
71-71: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
74-74: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
78-78: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
92-92: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
99-99: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
109-109: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
113-113: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
116-116: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
120-120: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
134-134: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
137-137: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
5-5: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
16-16: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
23-23: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
27-27: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
32-32: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
35-35: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
39-39: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
42-42: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
46-46: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
52-52: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
64-64: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
69-69: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
72-72: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
75-75: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
79-79: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
93-93: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
100-100: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
110-110: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
114-114: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
117-117: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
121-121: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
135-135: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
138-138: null (MD032, blanks-around-lists)
Lists should be surrounded by blank linesios/readme.md
46-46: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
66-66: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
269-269: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
38-38: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
41-41: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
42-42: Expected: 1; Actual: 3 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
47-47: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
65-65: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
83-83: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
94-94: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
95-95: Expected: 1; Actual: 3 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
158-158: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
159-159: Expected: 1; Actual: 3 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
23-23: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
26-26: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
136-136: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
271-271: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
267-267: Punctuation: '.' (MD026, no-trailing-punctuation)
Trailing punctuation in heading
32-32: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines
72-72: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines
78-78: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines
254-254: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines
4-4: null (MD045, no-alt-text)
Images should have alternate text (alt text)android/readme.md
72-72: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
134-134: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
182-182: Column: 1 (MD010, no-hard-tabs)
Hard tabs
183-183: Column: 1 (MD010, no-hard-tabs)
Hard tabs
184-184: Column: 1 (MD010, no-hard-tabs)
Hard tabs
185-185: Column: 1 (MD010, no-hard-tabs)
Hard tabs
186-186: Column: 1 (MD010, no-hard-tabs)
Hard tabs
187-187: Column: 1 (MD010, no-hard-tabs)
Hard tabs
188-188: Column: 1 (MD010, no-hard-tabs)
Hard tabs
189-189: Column: 1 (MD010, no-hard-tabs)
Hard tabs
190-190: Column: 1 (MD010, no-hard-tabs)
Hard tabs
191-191: Column: 1 (MD010, no-hard-tabs)
Hard tabs
242-242: Column: 1 (MD010, no-hard-tabs)
Hard tabs
243-243: Column: 1 (MD010, no-hard-tabs)
Hard tabs
244-244: Column: 1 (MD010, no-hard-tabs)
Hard tabs
245-245: Column: 1 (MD010, no-hard-tabs)
Hard tabs
246-246: Column: 1 (MD010, no-hard-tabs)
Hard tabs
247-247: Column: 1 (MD010, no-hard-tabs)
Hard tabs
248-248: Column: 1 (MD010, no-hard-tabs)
Hard tabs
249-249: Column: 1 (MD010, no-hard-tabs)
Hard tabs
255-255: Column: 1 (MD010, no-hard-tabs)
Hard tabs
256-256: Column: 1 (MD010, no-hard-tabs)
Hard tabs
257-257: Column: 1 (MD010, no-hard-tabs)
Hard tabs
271-271: Column: 1 (MD010, no-hard-tabs)
Hard tabs
272-272: Column: 1 (MD010, no-hard-tabs)
Hard tabs
273-273: Column: 1 (MD010, no-hard-tabs)
Hard tabs
274-274: Column: 1 (MD010, no-hard-tabs)
Hard tabs
275-275: Column: 1 (MD010, no-hard-tabs)
Hard tabs
279-279: Column: 1 (MD010, no-hard-tabs)
Hard tabs
280-280: Column: 1 (MD010, no-hard-tabs)
Hard tabs
281-281: Column: 1 (MD010, no-hard-tabs)
Hard tabs
282-282: Column: 1 (MD010, no-hard-tabs)
Hard tabs
283-283: Column: 1 (MD010, no-hard-tabs)
Hard tabs
284-284: Column: 1 (MD010, no-hard-tabs)
Hard tabs
285-285: Column: 1 (MD010, no-hard-tabs)
Hard tabs
293-293: Column: 1 (MD010, no-hard-tabs)
Hard tabs
294-294: Column: 1 (MD010, no-hard-tabs)
Hard tabs
295-295: Column: 1 (MD010, no-hard-tabs)
Hard tabs
296-296: Column: 1 (MD010, no-hard-tabs)
Hard tabs
298-298: Column: 1 (MD010, no-hard-tabs)
Hard tabs
299-299: Column: 1 (MD010, no-hard-tabs)
Hard tabs
300-300: Column: 1 (MD010, no-hard-tabs)
Hard tabs
301-301: Column: 1 (MD010, no-hard-tabs)
Hard tabs
314-314: Column: 1 (MD010, no-hard-tabs)
Hard tabs
315-315: Column: 1 (MD010, no-hard-tabs)
Hard tabs
316-316: Column: 1 (MD010, no-hard-tabs)
Hard tabs
317-317: Column: 1 (MD010, no-hard-tabs)
Hard tabs
318-318: Column: 1 (MD010, no-hard-tabs)
Hard tabs
319-319: Column: 1 (MD010, no-hard-tabs)
Hard tabs
321-321: Column: 1 (MD010, no-hard-tabs)
Hard tabs
322-322: Column: 1 (MD010, no-hard-tabs)
Hard tabs
323-323: Column: 1 (MD010, no-hard-tabs)
Hard tabs
324-324: Column: 1 (MD010, no-hard-tabs)
Hard tabs
326-326: Column: 1 (MD010, no-hard-tabs)
Hard tabs
333-333: Column: 1 (MD010, no-hard-tabs)
Hard tabs
334-334: Column: 1 (MD010, no-hard-tabs)
Hard tabs
335-335: Column: 1 (MD010, no-hard-tabs)
Hard tabs
30-30: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
363-363: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
140-140: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines
352-352: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines
37-37: null (MD040, fenced-code-language)
Fenced code blocks should have a language specified
54-54: null (MD040, fenced-code-language)
Fenced code blocks should have a language specified
66-66: null (MD040, fenced-code-language)
Fenced code blocks should have a language specified
78-78: null (MD040, fenced-code-language)
Fenced code blocks should have a language specified
86-86: null (MD040, fenced-code-language)
Fenced code blocks should have a language specified
6-6: null (MD045, no-alt-text)
Images should have alternate text (alt text)
Additional comments not posted (26)
shared/public/PerformanceLogger.cpp (1)
3-5
: Thread-local storage for performance data is a good practice for thread-safe operations without locking overhead..github/workflows/android.yml (1)
5-5
: Includingrelease/**
in the trigger branches is a good practice to ensure CI checks on release branches.android/src/main/cpp/graphics/OpenGlContext.cpp (1)
56-56
: Ensure that the buffer clearing operations are well-documented to avoid confusion about their intent and usage.Also applies to: 62-62
android/src/main/java/io/openmobilemaps/mapscore/graphics/GlTextureView.kt (3)
39-39
: LGTM! The addition ofpendingEnforcedFinishInterval
property is well-implemented.
51-51
: LGTM! Proper usage ofpendingEnforcedFinishInterval
in setting up the GL thread.
102-105
: LGTM! ThesetEnforcedFinishInterval
method correctly manages the enforced finish interval..github/workflows/sonatype.yml (2)
34-36
: LGTM! Updated environment variable names align with the new naming convention.
54-56
: LGTM! Consistent update of environment variable names across different jobs.android/src/main/cpp/graphics/shader/BaseShaderProgramOpenGl.cpp (1)
35-42
: Enhanced error logging inloadShader
method provides more context, which is beneficial for debugging.shared/src/map/layers/tiled/vector/geojson/geojsonvt/tile.hpp (1)
105-105
: Optimization of geometry processing insimplify
method enhances performance.shared/public/PerformanceLogger.h (1)
21-108
: IntroducedPerformanceLogger
class for performance metrics.This class provides a robust mechanism for performance logging with thread-local storage, ensuring minimal contention. The use of
std::chrono
for high-resolution timing and thread-local data structures for storing start times and counts is well-implemented. The locking mechanism ensures thread safety when accessing the shared global map.android/build.gradle (1)
163-166
: Updated environment variable names in Maven repository configuration.The changes to use new environment variable names (
UB_ARTIFACTORY_URL_ANDROID
,UB_ARTIFACTORY_USERNAME
,UB_ARTIFACTORY_PASSWORD
) are correctly implemented. This should align with the new naming conventions and ensure that the build script uses the correct credentials.ios/graphics/Model/Polygon/PolygonGroup2d.swift (1)
114-137
: IntroducedminX
andminY
to optimize vertex handling inPolygonGroup2d
.
[REFACTOR_SUGGESTion]
The addition ofminX
andminY
to capture the smallest vertex coordinates is a good optimization. It reduces the need to compute these values multiple times, especially in rendering paths where these values are frequently accessed.Consider using
where
clauses in loops for better readability and Swift convention compliance, and remove trailing semicolons to adhere to SwiftLint guidelines.- if let p = UnsafeRawPointer(bitPattern: Int(vertices.address)) { - for i in 0..<vertices.elementCount { - if i % 3 == 0 { + if let p = UnsafeRawPointer(bitPattern: Int(vertices.address)) { + for i in 0..<vertices.elementCount where i % 3 == 0 { let x = (p + 4 * Int(i)).load(as: Float.self) let y = (p + 4 * (Int(i) + 1)).load(as: Float.self) minX = min(x, minX) minY = min(y, minY) - } + } - } + } - self.posOffset.x = minX; - self.posOffset.y = minY; + self.posOffset.x = minX + self.posOffset.y = minYTools
SwiftLint
[Warning] 121-121:
where
clauses are preferred over a singleif
inside afor
(for_where)
[Warning] 136-136: Lines should not have trailing semicolons (trailing_semicolon)
[Warning] 137-137: Lines should not have trailing semicolons (trailing_semicolon)
CHANGELOG.md (1)
Line range hint
3-138
: UpdatedCHANGELOG.md
with entries for multiple versions.The changelog is well-maintained with clear entries for each version, detailing new features, improvements, and fixes. The formatting is consistent, and the entries provide a good overview of the changes in each version.
Consider using "cannot" instead of "can not" for a more formal tone, and ensure proper use of articles like "an" before vowel sounds.
- Log exception when a font can not be loaded + Log exception when a font cannot be loaded - Fixes a error in MapCamera2d::getPaddingCorrectedBounds + Fixes an error in MapCamera2d::getPaddingCorrectedBoundsTools
LanguageTool
[style] ~32-~32: Unless you want to emphasize “not”, use “cannot” which is more common. (CAN_NOT_PREMIUM)
Context: ...10.04.2024) - Log exception when a font can not be loaded ## Version 2.0.5 (08.04.2024...Markdownlint
3-3: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
4-4: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
15-15: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
22-22: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
26-26: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
31-31: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
34-34: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
38-38: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
5-5: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
16-16: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
23-23: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
27-27: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
32-32: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
35-35: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
39-39: null (MD032, blanks-around-lists)
Lists should be surrounded by blank linesios/graphics/Model/Polygon/Polygon2d.swift (1)
146-152
: Refactor enhances buffer management by usingcopyOrCreate
.Consider adding documentation for the
copyOrCreate
method to clarify its behavior and usage.ios/graphics/Model/Line/LineGroup2d.swift (1)
138-146
: Refactor enhances buffer management by usingcopyOrCreate
.Consider adding documentation for the
copyOrCreate
method to clarify its behavior and usage.shared/public/Tiled2dMapVectorLayerConfig.h (1)
35-56
: Enhanced tile URL generation with dynamic bounding box calculations.Consider adding error handling for potential exceptions during string operations to ensure robustness.
android/src/main/java/io/openmobilemaps/mapscore/graphics/BitmapTextureHolder.kt (1)
142-180
: Enhanced texture edge handling with flexibleedgeFillMode
.
- Correct the typo in
CanvasEdgeFillMode.Mirorred
toCanvasEdgeFillMode.Mirrored
.- Consider adding comments to explain the logic behind each
edgeFillMode
case for better maintainability.ios/maps/MCMapView.swift (1)
148-148
: Proper handling of app background state to prevent unnecessary rendering.android/src/main/java/io/openmobilemaps/mapscore/graphics/GLThread.kt (1)
Line range hint
80-183
: Introduced enforced finish interval to control rendering rate. Consider adding documentation to explain its usage and impact.shared/src/map/layers/tiled/vector/geojson/geojsonvt/geojsonvt.hpp (1)
Line range hint
55-140
: Optimized handling of GeoJSON data by adjusting zoom levels based on content complexity.shared/src/map/layers/tiled/vector/geojson/GeoJsonParser.h (1)
49-50
: Enhanced validation checks in GeoJSON parsing to ensure data integrity.shared/src/map/layers/polygon/PolygonLayer.cpp (1)
333-337
: Improved touch interaction handling by checking for a handler before proceeding.Also applies to: 384-391
ios/maps/MCTextureLoader.swift (1)
65-70
: Improved error handling for invalid URLs in asynchronous loading methods.Also applies to: 216-221
shared/src/map/layers/tiled/vector/sourcemanagers/Tiled2dMapVectorSourceTileDataManager.cpp (1)
39-43
: Good use of performance logging.The use of
PerformanceLogger
to measure the update time of tiles is a good practice. It helps in identifying performance bottlenecks and ensuring that the tile updates are efficient.shared/public/Value.h (1)
1127-1131
: Review the simplified interpolation logic inInterpolatedValue
.The new interpolation logic returns
yBase
ifinterpolationFactor
is less than 0.5, otherwiseyTop
. Ensure this change is tested thoroughly to confirm it behaves as expected across all use cases.
auto isStillVisible = weakActor.syncAccess([tile](auto actor){ | ||
auto strongSelf = actor.lock(); | ||
return strongSelf ? strongSelf->isTileVisible(tile) : false; | ||
}); | ||
if (isStillVisible == false) { | ||
weakActor.message(&Tiled2dMapSource::didFailToLoad, tile, loaderIndex, LoaderStatus::ERROR_OTHER, std::nullopt); | ||
} else { | ||
weakActor.message(&Tiled2dMapSource::didLoad, tile, loaderIndex, | ||
strongSelf->postLoadingTask(res, tile)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure proper handling of visibility checks during post-loading tasks.
The visibility check for tiles during post-loading tasks is crucial for performance and correctness. However, the current implementation might lead to race conditions or incorrect behavior if the tile visibility state changes between the check and the action. Consider locking or other synchronization mechanisms to ensure consistency.
double targetZoom = std::clamp(zoom, zoomMax, zoomMin); | ||
zoomAnimation = std::make_shared<DoubleAnimation>( | ||
DEFAULT_ANIM_LENGTH, this->zoom, targetZoom, InterpolatorFunction::EaseIn, | ||
[=](double zoom) { | ||
this->zoom = zoom; | ||
}, | ||
[=] { | ||
this->zoom = targetZoom; | ||
this->zoomAnimation = nullptr; | ||
}); | ||
zoomAnimation->start(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactor the zoom animation logic for clarity and efficiency.
The zoom animation logic has been updated to use a clamped value for the target zoom. This ensures that the zoom level stays within the allowed range. However, the logic can be further optimized by consolidating the repeated clamping operations and ensuring that the animation callbacks are set up correctly to update the internal state only once at the end of the animation.
double targetZoom = std::clamp(zoom, zoomMax, zoomMin);
zoomAnimation = std::make_shared<DoubleAnimation>(
DEFAULT_ANIM_LENGTH, this->zoom, targetZoom, InterpolatorFunction::EaseIn,
[=](double zoom) {
this->zoom = zoom;
},
[=] {
this->zoom = targetZoom;
this->zoomAnimation = nullptr;
});
zoomAnimation->start();
This refactoring removes redundant code and ensures that the zoom level is updated efficiently.
auto mapInterface = this->mapInterface; | ||
auto mapDescription = this->mapDescription; | ||
|
||
if (!mapInterface || !mapDescription) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure robust error handling for map interface and description checks.
The method reloadLocalDataSource
checks for the validity of mapInterface
and mapDescription
but does not handle the case where either is null. This could lead to further execution of the method with invalid state. Consider adding error handling or logging to manage these scenarios effectively:
if (!mapInterface || !mapDescription) {
LogError << "Map interface or description is not available.";
return;
}
This ensures that the function exits early and safely when required components are not available, preventing potential null dereferences later in the method.
UB_ARTIFACTORY_URL_ANDROID: ${{secrets.UBIQUE_ARTIFACTORY_URL}} | ||
UB_ARTIFACTORY_USERNAME: ${{secrets.UBIQUE_ARTIFACTORY_USER}} | ||
UB_ARTIFACTORY_PASSWORD: ${{secrets.UBIQUE_ARTIFACTORY_PASS}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tip
Codebase Verification
The old environment variables (UB_ARTIFACTORY_URL
, UB_ARTIFACTORY_USERNAME
, UB_ARTIFACTORY_PASSWORD
) are still present in the android/build.gradle
file. These should be updated to the new environment variables to ensure consistency across the codebase.
android/build.gradle
:UB_ARTIFACTORY_URL
should be updated toUBIQUE_ARTIFACTORY_URL
UB_ARTIFACTORY_USERNAME
should be updated toUBIQUE_ARTIFACTORY_USER
UB_ARTIFACTORY_PASSWORD
should be updated toUBIQUE_ARTIFACTORY_PASS
Analysis chain
Ensure that the renamed environment variables are consistently used across all configuration files.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the consistent use of renamed environment variables across all configuration files.
# Test: Search for the old and new environment variable names. Expect: Only occurrences of the new names.
rg --type all $'UBIQUE_ARTIFACTORY_URL|UBIQUE_ARTIFACTORY_USER|UBIQUE_ARTIFACTORY_PASS'
Length of output: 88
Script:
#!/bin/bash
# Description: Verify the consistent use of renamed environment variables across all configuration files.
# Test: Search for the old and new environment variable names. Expect: Only occurrences of the new names.
rg --type all $'UBIQUE_ARTIFACTORY_URL|UBIQUE_ARTIFACTORY_USER|UBIQUE_ARTIFACTORY_PASS|UB_ARTIFACTORY_URL|UB_ARTIFACTORY_USERNAME|UB_ARTIFACTORY_PASSWORD'
Length of output: 532
func copyMemory(bytes pointer: UnsafeRawPointer, length: Int) { | ||
self.contents().copyMemory(from: pointer, byteCount: length) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The addition of memory management methods is a good practice. Consider adding unit tests to ensure these methods work as expected.
Would you like me to help with writing these unit tests?
Also applies to: 50-61
if (!stop[0].is_number()) { | ||
LogError <<= "Tiled2dMapVectorStyleParser not handled: " + json.dump(); | ||
return nullptr; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improve error handling by adding specific error messages.
The error logging in the parseValue
method could be more descriptive. Currently, it logs a generic message which might not be helpful for debugging specific issues. Consider including more details about the expected format or the nature of the error.
- LogError <<= "Tiled2dMapVectorStyleParser not handled: " + json.dump();
+ LogError <<= "Expected a number but got: " + json.dump();
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.
if (!stop[0].is_number()) { | |
LogError <<= "Tiled2dMapVectorStyleParser not handled: " + json.dump(); | |
return nullptr; | |
} | |
if (!stop[0].is_number()) { | |
LogError <<= "Expected a number but got: " + json.dump(); | |
return nullptr; | |
} |
@@ -15,6 +15,7 @@ | |||
#include "Tiled2dMapVectorLineTile.h" | |||
#include "Tiled2dMapVectorLayer.h" | |||
#include "RenderPass.h" | |||
#include "PerformanceLogger.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tip
Codebase Verification
The search results confirm that PerformanceLogger
is included in the file but not used anywhere else. This indicates that the inclusion of PerformanceLogger.h
is unnecessary and should be removed.
- shared/src/map/layers/tiled/vector/sourcemanagers/Tiled2dMapVectorSourceTileDataManager.cpp: Remove the unused
#include "PerformanceLogger.h"
directive.
Analysis chain
Ensure PerformanceLogger.h
is necessary here.
The inclusion of PerformanceLogger.h
suggests that performance logging features are being used in this file. Verify that this header is required and that the performance logging is implemented correctly throughout the file.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for usage of PerformanceLogger in Tiled2dMapVectorSourceTileDataManager.cpp
rg --type cpp "PerformanceLogger" shared/src/map/layers/tiled/vector/sourcemanagers/Tiled2dMapVectorSourceTileDataManager.cpp
Length of output: 156
Script:
#!/bin/bash
# Description: Check for any usage of PerformanceLogger methods or instances in Tiled2dMapVectorSourceTileDataManager.cpp
rg --type cpp "PerformanceLogger" shared/src/map/layers/tiled/vector/sourcemanagers/Tiled2dMapVectorSourceTileDataManager.cpp
Length of output: 156
|
||
var minX = Float.greatestFiniteMagnitude | ||
var minY = Float.greatestFiniteMagnitude | ||
|
||
if let p = UnsafeRawPointer(bitPattern: Int(vertices.address)) { | ||
for i in 0..<vertices.elementCount { | ||
if i % 3 == 0 { | ||
let x = (p + 4 * Int(i)).load(as: Float.self) | ||
let y = (p + 4 * (Int(i) + 1)).load(as: Float.self) | ||
minX = min(x, minX) | ||
minY = min(y, minY) | ||
} | ||
} | ||
} | ||
lock.withCritical { | ||
self.indicesCount = Int(indices.elementCount) | ||
self.verticesBuffer = verticesBuffer | ||
self.indicesBuffer = indicesBuffer | ||
|
||
if let p = UnsafeRawPointer(bitPattern: Int(vertices.address)) { | ||
var minX = Float.greatestFiniteMagnitude | ||
var minY = Float.greatestFiniteMagnitude | ||
|
||
for i in 0..<vertices.elementCount { | ||
if i % 3 == 0 { | ||
let x = (p + 4 * Int(i)).load(as: Float.self) | ||
let y = (p + 4 * (Int(i) + 1)).load(as: Float.self) | ||
minX = min(x, minX) | ||
minY = min(y, minY) | ||
} | ||
} | ||
|
||
self.posOffset.x = minX; | ||
self.posOffset.y = minY; | ||
} | ||
self.posOffset.x = minX; | ||
self.posOffset.y = minY; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored setVertices
to calculate minimum x and y values for vertices.
Consider adding a comment explaining the purpose of calculating these minimum values, as it could help other developers understand their usage in the rendering process.
Address SwiftLint warnings:
- Replace the
if
inside thefor
loop with awhere
clause for clarity. - Remove trailing semicolons for Swift style compliance.
- if i % 3 == 0 {
+ for i in stride(from: 0, to: vertices.elementCount, by: 3) {
- self.posOffset.x = minX;
- self.posOffset.y = minY;
+ self.posOffset.x = minX
+ self.posOffset.y = minY
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.
var minX = Float.greatestFiniteMagnitude | |
var minY = Float.greatestFiniteMagnitude | |
if let p = UnsafeRawPointer(bitPattern: Int(vertices.address)) { | |
for i in 0..<vertices.elementCount { | |
if i % 3 == 0 { | |
let x = (p + 4 * Int(i)).load(as: Float.self) | |
let y = (p + 4 * (Int(i) + 1)).load(as: Float.self) | |
minX = min(x, minX) | |
minY = min(y, minY) | |
} | |
} | |
} | |
lock.withCritical { | |
self.indicesCount = Int(indices.elementCount) | |
self.verticesBuffer = verticesBuffer | |
self.indicesBuffer = indicesBuffer | |
if let p = UnsafeRawPointer(bitPattern: Int(vertices.address)) { | |
var minX = Float.greatestFiniteMagnitude | |
var minY = Float.greatestFiniteMagnitude | |
for i in 0..<vertices.elementCount { | |
if i % 3 == 0 { | |
let x = (p + 4 * Int(i)).load(as: Float.self) | |
let y = (p + 4 * (Int(i) + 1)).load(as: Float.self) | |
minX = min(x, minX) | |
minY = min(y, minY) | |
} | |
} | |
self.posOffset.x = minX; | |
self.posOffset.y = minY; | |
} | |
self.posOffset.x = minX; | |
self.posOffset.y = minY; | |
var minX = Float.greatestFiniteMagnitude | |
var minY = Float.greatestFiniteMagnitude | |
if let p = UnsafeRawPointer(bitPattern: Int(vertices.address)) { | |
for i in stride(from: 0, to: vertices.elementCount, by: 3) { | |
let x = (p + 4 * Int(i)).load(as: Float.self) | |
let y = (p + 4 * (Int(i) + 1)).load(as: Float.self) | |
minX = min(x, minX) | |
minY = min(y, minY) | |
} | |
} | |
lock.withCritical { | |
self.indicesCount = Int(indices.elementCount) | |
self.verticesBuffer = verticesBuffer | |
self.indicesBuffer = indicesBuffer | |
self.posOffset.x = minX | |
self.posOffset.y = minY |
Tools
SwiftLint
[Warning] 152-152:
where
clauses are preferred over a singleif
inside afor
(for_where)
[Warning] 164-164: Lines should not have trailing semicolons (trailing_semicolon)
[Warning] 165-165: Lines should not have trailing semicolons (trailing_semicolon)
Summary by CodeRabbit
New Features
{bbox-epsg-3857}
placeholder in URL templates.CanvasEdgeFillMode
with options: Mirrored, Clamped, and None.Improvements
Bug Fixes
Documentation