Misc. OpenGL improvements and fixes #573
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Warning Rate Limit Exceeded@maurhofer-ubique has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 43 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe updates across various files in the Android graphics package reflect an optimization effort, primarily focusing on improving the management of OpenGL data buffers. A new boolean flag Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (29)
- android/src/main/cpp/graphics/objects/LineGroup2dOpenGl.cpp (3 hunks)
- android/src/main/cpp/graphics/objects/LineGroup2dOpenGl.h (1 hunks)
- android/src/main/cpp/graphics/objects/Polygon2dOpenGl.cpp (2 hunks)
- android/src/main/cpp/graphics/objects/Polygon2dOpenGl.h (1 hunks)
- android/src/main/cpp/graphics/objects/PolygonGroup2dOpenGl.cpp (2 hunks)
- android/src/main/cpp/graphics/objects/PolygonGroup2dOpenGl.h (1 hunks)
- android/src/main/cpp/graphics/objects/PolygonPatternGroup2dOpenGl.cpp (2 hunks)
- android/src/main/cpp/graphics/objects/PolygonPatternGroup2dOpenGl.h (1 hunks)
- android/src/main/cpp/graphics/objects/Quad2dInstancedOpenGl.cpp (4 hunks)
- android/src/main/cpp/graphics/objects/Quad2dInstancedOpenGl.h (2 hunks)
- android/src/main/cpp/graphics/objects/Quad2dOpenGl.cpp (1 hunks)
- android/src/main/cpp/graphics/objects/Quad2dStretchedInstancedOpenGl.cpp (5 hunks)
- android/src/main/cpp/graphics/objects/Quad2dStretchedInstancedOpenGl.h (2 hunks)
- android/src/main/cpp/graphics/objects/Text2dInstancedOpenGl.cpp (5 hunks)
- android/src/main/cpp/graphics/objects/Text2dInstancedOpenGl.h (2 hunks)
- android/src/main/cpp/graphics/objects/Text2dOpenGl.cpp (2 hunks)
- android/src/main/cpp/graphics/objects/Text2dOpenGl.h (1 hunks)
- android/src/main/cpp/graphics/shader/ColorLineGroup2dShaderOpenGl.cpp (1 hunks)
- android/src/main/cpp/graphics/shader/ColorPolygonGroup2dShaderOpenGl.cpp (2 hunks)
- shared/public/Tiled2dMapTileInfo.h (1 hunks)
- shared/public/Value.h (3 hunks)
- shared/src/map/layers/tiled/vector/sourcemanagers/Tiled2dMapVectorSourceRasterTileDataManager.cpp (1 hunks)
- shared/src/map/layers/tiled/vector/sourcemanagers/Tiled2dMapVectorSourceTileDataManager.cpp (1 hunks)
- shared/src/map/layers/tiled/vector/sourcemanagers/Tiled2dMapVectorSourceTileDataManager.h (1 hunks)
- shared/src/map/layers/tiled/vector/sourcemanagers/Tiled2dMapVectorSourceVectorTileDataManager.cpp (1 hunks)
- shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSourceSymbolDataManager.cpp (1 hunks)
- shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSymbolObject.cpp (1 hunks)
- shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSymbolObject.h (1 hunks)
- shared/src/map/scheduling/ThreadPoolSchedulerImpl.h (1 hunks)
Additional comments: 55
shared/public/Tiled2dMapTileInfo.h (1)
- 51-53: The addition of the
to_string_shortmethod provides a more concise string representation of theTiled2dMapTileInfoobject, which could be useful for logging or debugging purposes where the full class name is not required.android/src/main/cpp/graphics/objects/PolygonGroup2dOpenGl.h (1)
- 62-66: Initializing the
attribBufferandindexBufferto-1andglDataBuffersGeneratedtofalseis a good practice to ensure that these variables have a known state before they are used.shared/src/map/scheduling/ThreadPoolSchedulerImpl.h (1)
- 60-61: Increasing the
MAX_NUM_GRAPHICS_TASKSto128andMAX_TIME_GRAPHICS_TASKS_MSto6milliseconds is a reasonable change to potentially improve the handling of graphics tasks within the thread pool scheduler.android/src/main/cpp/graphics/objects/LineGroup2dOpenGl.h (1)
- 67-71: Initializing
vertexAttribBufferandindexBufferwith default values and adding theglDataBuffersGeneratedmember variable are good changes for managing OpenGL data buffers more efficiently.android/src/main/cpp/graphics/objects/Polygon2dOpenGl.h (1)
- 69-70: The addition of the
glDataBuffersGeneratedboolean member variable is a good practice to track the generation state of OpenGL data buffers, which can help prevent unnecessary buffer operations.android/src/main/cpp/graphics/objects/Text2dOpenGl.h (1)
- 77-77: Replacing the
hasVertexBufferandhasIndexBufferflags with a singleglDataBuffersGeneratedflag simplifies the logic related to OpenGL data buffer generation.android/src/main/cpp/graphics/objects/PolygonPatternGroup2dOpenGl.h (1)
- 84-84: The introduction of the
glDataBuffersGeneratedboolean member variable in thePolygonPatternGroup2dOpenGlclass is a good addition for tracking OpenGL buffer state.shared/src/map/layers/tiled/vector/sourcemanagers/Tiled2dMapVectorSourceTileDataManager.h (1)
- 76-76: Adding the
std::atomic_flagnamednoPendingUpdateMasksis a thread-safe way to manage the state of pending updates, which can improve the efficiency of the update mechanism.android/src/main/cpp/graphics/objects/Text2dInstancedOpenGl.h (1)
- 110-133: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [92-128]
The consolidation of buffer-related member variables into
dynamicInstanceDataBufferand the renaming of thewriteToBuffermethod towriteToDynamicInstanceDataBufferare good changes that simplify the buffer management strategy.android/src/main/cpp/graphics/shader/ColorPolygonGroup2dShaderOpenGl.cpp (2)
- 56-56: The modification of the
glUniform1fvfunction call to use the correct size parameters ensures that the correct number of style values are passed to the shader, which is crucial for rendering the polygons with the correct styles.- 69-69: Moving the assignment of
this->numStylesto before the end of thesetStylesmethod is a logical change that ensures the member variable is updated after the styles are copied.android/src/main/cpp/graphics/objects/Quad2dInstancedOpenGl.h (1)
- 115-135: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [97-130]
The addition of the
glDataBuffersGeneratedboolean member and thedynamicInstanceDataBuffer, along with the removal of specific buffer variables, are good changes that streamline the management of OpenGL data buffers.android/src/main/cpp/graphics/objects/PolygonGroup2dOpenGl.cpp (1)
- 58-75: The conditional generation and deletion of OpenGL buffers in the
setupandremoveGlBuffersmethods, along with the use of theglDataBuffersGeneratedflag, are good practices that can help prevent redundant OpenGL operations and optimize performance.android/src/main/cpp/graphics/objects/Quad2dStretchedInstancedOpenGl.h (1)
- 117-142: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [99-138]
The introduction of the
glDataBuffersGeneratedflag and thedynamicInstanceDataBuffer, as well as the removal of specific buffer variables and the addition of static constants for buffer offsets, are good changes that simplify the buffer management and improve the code structure.android/src/main/cpp/graphics/objects/Polygon2dOpenGl.cpp (1)
- 61-77: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [64-95]
The conditional generation and deletion of OpenGL buffers in the
prepareGlDataandremoveGlBuffersmethods, along with the introduction of theglDataBuffersGeneratedflag, are good practices that can help prevent redundant OpenGL operations and optimize performance.android/src/main/cpp/graphics/objects/LineGroup2dOpenGl.cpp (4)
- 63-65: The conditional generation of the vertex buffer is correctly implemented, ensuring it's only done if
glDataBuffersGeneratedis false.- 70-72: The conditional generation of the index buffer follows the same correct pattern as the vertex buffer.
- 81-81: Setting
glDataBuffersGeneratedto true after generating the buffers ensures that the buffers won't be unnecessarily regenerated.- 93-97: The conditional deletion of both vertex and index buffers is correctly implemented, with the flag being reset after deletion.
shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSymbolObject.h (2)
- 55-55: Ensure that the destructor definition for
Tiled2dMapVectorSymbolObjecthas been correctly moved to the implementation file.Verification successful
The output from the shell script confirms that the destructor for
Tiled2dMapVectorSymbolObjecthas been defined in the corresponding.cppfile, which isshared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSymbolObject.cpp. This verifies that the structural change mentioned in the review comment has been correctly implemented.* 57-57: Similarly, verify that the `placedInCache` method definition is present in the implementation file.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the destructor is defined in the corresponding .cpp file rg "Tiled2dMapVectorSymbolObject::~Tiled2dMapVectorSymbolObject" --type cppLength of output: 216
Verification successful
The
placedInCachemethod definition for theTiled2dMapVectorSymbolObjectclass is indeed present in the corresponding.cppfile as confirmed by the output of the executed script.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the placedInCache method is defined in the corresponding .cpp file rg "void Tiled2dMapVectorSymbolObject::placedInCache" --type cppLength of output: 194
android/src/main/cpp/graphics/objects/Quad2dOpenGl.cpp (1)
- 125-129: The conditional deletion of OpenGL buffers in
removeGlBuffersis correctly implemented, with the flag being reset after deletion.android/src/main/cpp/graphics/objects/PolygonPatternGroup2dOpenGl.cpp (3)
- 69-71: The conditional generation of the vertex buffer in
prepareGlDatais correctly implemented, ensuring it's only done ifglDataBuffersGeneratedis false.- 77-79: The conditional generation of the index buffer follows the same correct pattern as the vertex buffer.
- 99-103: The conditional deletion of both vertex and index buffers in
removeGlBuffersis correctly implemented, with the flag being reset after deletion.android/src/main/cpp/graphics/objects/Text2dOpenGl.cpp (3)
- 140-147: The conditional generation of vertex and index buffers in
prepareGlDatais correctly implemented, ensuring it's only done ifglDataBuffersGeneratedis false.- 161-161: Setting
glDataBuffersGeneratedto true after generating the buffers ensures that the buffers won't be unnecessarily regenerated.- 166-169: The conditional deletion of both vertex and index buffers in
removeGlBuffersis correctly implemented, with the flag being reset after deletion.android/src/main/cpp/graphics/objects/Quad2dInstancedOpenGl.cpp (7)
- 84-86: The conditional generation of the vertex buffer in
prepareGlDatais correctly implemented, ensuring it's only done ifglDataBuffersGeneratedis false.- 90-95: The conditional generation of the dynamic instance data buffer is correctly implemented, which is crucial for instanced rendering.
- 98-100: The conditional generation of the index buffer follows the same correct pattern as the vertex buffer.
- 113-113: Setting
glDataBuffersGeneratedto true after generating the buffers ensures that the buffers won't be unnecessarily regenerated.- 138-143: The conditional deletion of vertex, index, and dynamic instance data buffers in
removeGlBuffersis correctly implemented, with the flag being reset after deletion.- 225-238: The attribute pointer settings for position, rotation, texture coordinates, scales, and alphas are correctly updated to use dynamic instance data buffers.
- 302-341: The method
writeToDynamicInstanceDataBufferis correctly implemented to update the dynamic instance data buffer with new data.android/src/main/cpp/graphics/objects/Text2dInstancedOpenGl.cpp (4)
- 82-99: The conditional generation of OpenGL buffers using the
glDataBuffersGeneratedflag is correctly implemented to avoid redundant buffer generation. This should improve performance by ensuring buffers are only created once.- 138-144: The conditional deletion of OpenGL buffers using the
glDataBuffersGeneratedflag is correctly implemented to ensure buffers are deleted only if they have been created.- 219-232: The attribute pointer settings and buffer data writing methods have been updated correctly to use the
dynamicInstanceDataBuffer. This change is part of the refactoring for managing dynamic instance data buffers.- 297-331: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [300-352]
The
writeToDynamicInstanceDataBuffermethod is correctly used to update buffer data, and thebuffersNotReadyflag is updated accordingly. This ensures that buffer data is only written when the object is ready, and the flags are cleared when the data is successfully written.android/src/main/cpp/graphics/objects/Quad2dStretchedInstancedOpenGl.cpp (4)
- 85-101: The conditional generation of OpenGL buffers using the
glDataBuffersGeneratedflag is correctly implemented, similar to the previous file. This change is consistent and follows the same pattern for buffer management.- 140-145: The conditional deletion of OpenGL buffers is correctly implemented, ensuring that buffers are deleted only if they have been created, which is consistent with the changes in the
Text2dInstancedOpenGl.cppfile.- 224-246: The attribute pointer settings and buffer data writing methods have been updated correctly to use the
dynamicInstanceDataBuffer, which is part of the refactoring for managing dynamic instance data buffers.- 318-364: The
writeToDynamicInstanceDataBuffermethod is used correctly to update buffer data, and thebuffersNotReadyflag is updated accordingly. This ensures that buffer data is only written when the object is ready, and the flags are cleared when the data is successfully written.android/src/main/cpp/graphics/shader/ColorLineGroup2dShaderOpenGl.cpp (1)
- 53-53: The modification to the
glUniform1fvcall to usenumStyles * sizeLineValuesas the size parameter is correct. This change ensures that the correct size is used when setting uniform values for line styles in the shader.shared/src/map/layers/tiled/vector/sourcemanagers/Tiled2dMapVectorSourceRasterTileDataManager.cpp (1)
- 154-154: The direct clearing of
noPendingUpdateMaskswithout callingupdateMaskObjectsis a control flow change that simplifies the logic. This change is consistent with the PR objectives to streamline the update process for map vector sources and symbols.shared/src/map/layers/tiled/vector/sourcemanagers/Tiled2dMapVectorSourceVectorTileDataManager.cpp (1)
- 158-158: The direct call to
noPendingUpdateMasks.clear()replaces a previous message dispatching mechanism. This change simplifies the control flow and potentially improves performance by avoiding the overhead of message dispatching.shared/src/map/layers/tiled/vector/sourcemanagers/Tiled2dMapVectorSourceTileDataManager.cpp (1)
- 20-22: The addition of a conditional check using
noPendingUpdateMasks.test_and_set()before callingupdateMaskObjects()is a good practice to ensure that the update is only performed when necessary, which can help in avoiding redundant operations and improving performance.shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSourceSymbolDataManager.cpp (4)
- 561-561: The introduction of the
clearCoordinatorboolean variable is a good practice to avoid unnecessary operations iftilesToClearis empty.- 561-567: The logic to clear symbol groups only when
tilesToClearis not empty is correctly implemented.- 567-567: Clearing the
tilesToClearvector after processing is a good practice to prevent memory leaks and ensure that the data is up-to-date.- 569-569: Conditionally clearing animation coordinators based on the
clearCoordinatorflag is an efficient way to manage resources.shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSymbolObject.cpp (3)
- 164-172: The destructor correctly manages the lifecycle of the
animationCoordinatorby checking ownership and decreasing usage. This is important for proper resource management.- 174-181: The new method
placedInCacheis added to handle the case when the symbol object is placed in the cache. It properly clears the ownership flag if the object is the owner, which is consistent with the logic in the destructor.- 183-184: The
updateLayerDescriptionmethod's parameters have been reordered for consistency. This change should be cross-checked to ensure all calls to this method have been updated accordingly.shared/public/Value.h (1)
- 43-43: The inclusion of "Logger.h" is added to provide logging capabilities within this file. This is a standard practice for enabling logging.
zimmermannubique
approved these changes
Jan 16, 2024
…s in source tile data manager on update, allow more gl tasks on pre-render (within given time constraint), don't cache zoom-dependent values (prevent cache overflow)
…l object function implementations
maurhofer-ubique
force-pushed
the
bugfix/opengl-cleanup-improvements
branch
from
5561ef6 to
11ade9f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tiled2dMapTileInfoto provide more concise output.Performance Improvements
Bug Fixes
ValueEvaluatorclass to enhance reliability.Refactor
Tiled2dMapVectorSymbolObject.Documentation
Style
Tests
Chores
Revert