ignore animations that are only cached symbols #571
Conversation
|
Warning Rate Limit Exceeded@maerki has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 17 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 recent updates focus on enhancing tile visibility handling and symbol animation within a map rendering system. The 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: 1
Configuration used: CodeRabbit UI
Files selected for processing (9)
- shared/public/Tiled2dMapSource.h (1 hunks)
- shared/public/Tiled2dMapSourceImpl.h (1 hunks)
- shared/src/map/layers/tiled/vector/Tiled2dMapVectorLayer.cpp (2 hunks)
- shared/src/map/layers/tiled/vector/symbol/SymbolAnimationCoordinator.h (3 hunks)
- shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSourceSymbolDataManager.cpp (1 hunks)
- shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSymbolGroup.cpp (1 hunks)
- shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSymbolGroup.h (1 hunks)
- shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSymbolObject.cpp (1 hunks)
- shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSymbolObject.h (1 hunks)
Files skipped from review due to trivial changes (1)
- shared/public/Tiled2dMapSource.h
Additional comments: 12
shared/src/map/layers/tiled/vector/symbol/SymbolAnimationCoordinator.h (2)
55-65: The
isAnimatingmethod has been updated with a new condition to check ifcacheCountis greater than or equal tousageCount. This logic seems to be intended to prevent animations for symbols that are cached and not currently used. Ensure that the logic aligns with the intended behavior and thatusageCountis being correctly maintained elsewhere in the code.133-133: A new member variable
cacheCounthas been added to the class. This variable should be properly initialized and used throughout the class to track the number of times a symbol is cached. It's crucial to ensure that this variable is thread-safe if accessed concurrently, given that it's of typestd::atomic_int.shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSymbolObject.h (1)
- 74-75: A new method
removeFromCachehas been declared but not defined within this file. Ensure that the implementation of this method correctly handles the removal of symbols from the cache and that it is called in the appropriate places.Verification successful
The
removeFromCachemethod has been found in theTiled2dMapVectorSymbolObject.cppfile, confirming that the method is indeed implemented in the codebase. This aligns with the review comment that a new methodremoveFromCachehas been declared in theTiled2dMapVectorSymbolObjectclass.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the implementation of removeFromCache method ast-grep --lang cpp --pattern $'void removeFromCache() { $$$ }'Length of output: 64
Script:
#!/bin/bash # Search for the implementation of the removeFromCache method in .cpp files ast-grep --lang cpp --pattern $'void removeFromCache() { $$$ }'Length of output: 64
Script:
#!/bin/bash # Use ripgrep to search for the removeFromCache method definition in .cpp files rg 'void Tiled2dMapVectorSymbolObject::removeFromCache\(\)' -t cppLength of output: 198
shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSymbolGroup.h (1)
- 68-69: A new method
removeFromCache()has been added to theTiled2dMapVectorSymbolGroupclass. Ensure that this method is implemented to correctly handle the removal of symbol groups from the cache, and that it is called when the tile state changes as described in the PR objectives.Verification successful
The ast-grep command did not return any output, which suggests that the
removeFromCache()method implementation was not found in the codebase. This could mean that the method is declared but not defined within the codebase, or there might be an issue with the search pattern used.To verify the presence and implementation of the
removeFromCache()method, I will adjust the search pattern to be less restrictive and try again.
The ast-grep command again did not return any output, and this time it included a warning that the pattern contains an ERROR node, which may cause unexpected results. This suggests that the pattern used might not be correctly formulated or that the method definition is not present in the codebase.
Given the results, it would be prudent to try a different approach to verify the presence of the
removeFromCache()method. I will use therg(ripgrep) command to search for the method signature in the codebase, which can help confirm whether the method is defined somewhere in the files.
The output from the ripgrep command confirms that the
removeFromCache()method is indeed defined within theTiled2dMapVectorSymbolGroupclass in theTiled2dMapVectorSymbolGroup.cppfile. This verifies that the method has been implemented in the codebase, aligning with the PR objectives and the review comment.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the implementation of removeFromCache method ast-grep --lang cpp --pattern $'void removeFromCache() { $$$ }'Length of output: 64
Script:
#!/bin/bash # Search for the removeFromCache method definition in the codebase ast-grep --lang cpp --pattern $'class $_ { $$$ void removeFromCache() { $$$ } $$$ }'Length of output: 430
Script:
#!/bin/bash # Use ripgrep to search for the removeFromCache method signature in the codebase rg "void Tiled2dMapVectorSymbolGroup::removeFromCache\(\)"Length of output: 188
shared/public/Tiled2dMapSourceImpl.h (1)
- 81-84: The
onVisibleBoundsChangedmethod has been modified to resetlastVisibleTilesHashto0under certain conditions. This change is likely intended to force a refresh of visible tiles when needed. Verify that this new behavior is correctly triggered and that it does not introduce any performance issues due to unnecessary tile refreshes.shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSourceSymbolDataManager.cpp (1)
- 533-544: The added code block from lines 533 to 544 introduces logic to remove symbol groups from the cache when their tile state changes from
CACHEDto a different state. This is a critical change for the performance optimization mentioned in the PR objectives. Ensure that theremoveFromCachemethod called on thesymbolGroupobject is thread-safe and does not introduce race conditions, as it seems to be called within asyncAccesslambda which suggests concurrent access.Additionally, verify that the
tileStateMapandtileSymbolGroupMapare properly managed across threads, as they are being accessed and modified within a locked section. The use ofstd::lock_guardwithupdateMutexindicates an awareness of concurrency, but it's important to ensure that all access patterns to these shared resources are covered by appropriate synchronization mechanisms to prevent data races.Verification successful
The
removeFromCachemethod exists within theTiled2dMapVectorSymbolGroupandTiled2dMapVectorSymbolObjectclasses, as confirmed by the search results. While the implementation details and thread safety of the method itself are not provided, the original code snippet's use ofsyncAccesssuggests that it is intended to be thread-safe.The search results also show several instances where
tileStateMapandtileSymbolGroupMapare accessed or modified within the scope of alock_guardthat locksupdateMutex. This indicates that thread safety measures are in place for these shared resources in the provided instances. However, without an exhaustive verification of every access in the codebase, it cannot be guaranteed that all accesses are protected. The consistent pattern of usingupdateMutexin the shown instances is a positive indication of thread safety.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify thread safety of removeFromCache method ast-grep --lang cpp --pattern $'class Tiled2dMapVectorSymbolGroup { $$$ void removeFromCache() { $$$ } $$$ }' # Verify that all accesses to tileStateMap and tileSymbolGroupMap are protected by updateMutex rg --type cpp "tileStateMap|tileSymbolGroupMap" | rg --type cpp "lock_guard|updateMutex"Length of output: 558
Script:
#!/bin/bash # Refine the ast-grep pattern to correctly match the removeFromCache method within the Tiled2dMapVectorSymbolGroup class. ast-grep --lang cpp --pattern $'class Tiled2dMapVectorSymbolGroup { $$$ void removeFromCache() { $$$ } $$$ }' # Correct the rg command to search for occurrences of tileStateMap or tileSymbolGroupMap and ensure that these occurrences are within the scope of a lock_guard or updateMutex to verify thread safety. rg --type cpp -C 5 "tileStateMap|tileSymbolGroupMap" | rg "lock_guard|updateMutex"Length of output: 1762
Script:
#!/bin/bash # Use rg to search for the removeFromCache method within the cpp files to verify its existence and thread safety. rg --type cpp "void removeFromCache\(\)" # Use rg with a larger context to verify that all accesses to tileStateMap and tileSymbolGroupMap are protected by updateMutex without filtering out the context lines. rg --type cpp -C 10 "tileStateMap|tileSymbolGroupMap" | rg -C 10 "updateMutex"Length of output: 22016
shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSymbolObject.cpp (3)
164-171: The destructor properly checks if the
animationCoordinatoris not null and if the current object is the coordinate owner before clearing the ownership and decreasing usage. This is a good practice to prevent resource leaks and ensure proper cleanup.174-181: The
placedInCachemethod correctly checks for the existence of theanimationCoordinatorand clears the coordinate ownership if necessary before increasing the cache count. This method should ensure that the cache count is only increased when the symbol is actually placed in the cache to avoid miscounts.184-187: The
removeFromCachemethod decreases the cache count through theanimationCoordinator. It is important to ensure that this method is called in all cases where a symbol is removed from the cache to maintain an accurate cache count.shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSymbolGroup.cpp (1)
- 844-848: The new
removeFromCachemethod correctly iterates through eachsymbolObjectand calls itsremoveFromCachemethod. This should effectively remove each symbol object from the cache. Ensure that theremoveFromCachemethod on thesymbolObjectdoes the necessary cleanup to prevent memory leaks or dangling pointers.shared/src/map/layers/tiled/vector/Tiled2dMapVectorLayer.cpp (2)
541-543: The calculation of
zoomChangeandtimeDiffhas been moved before the conditional check, which is a logical change to ensure these values are updated before they are used in the condition. This change is approved.558-558: The conditional check for
now - lastCollitionCheckhas been changed from2000*0to3000. This seems to be a correction of what was likely a previous typo or placeholder (2000*0would always evaluate to0). The new value3000extends the time between collision checks to 3 seconds. Ensure that this new interval is consistent with the intended behavior and performance requirements.
| int increaseCache() {; | ||
| return ++cacheCount; | ||
| } | ||
|
|
||
| int decreaseCache() { | ||
| return --cacheCount; | ||
| } |
There was a problem hiding this comment.
The output from the rg command shows the implementation of the decreaseCache method in the SymbolAnimationCoordinator.h file. The method simply decrements the cacheCount and returns the new value. There are no additional checks shown in the output to prevent cacheCount from becoming negative.
Based on the information provided, it appears that there are no safeguards within the decreaseCache method to prevent cacheCount from being decremented below zero. This could potentially lead to incorrect behavior as mentioned in the review comment.
Given the evidence from the codebase, the review comment is consistent with the implementation of the decreaseCache method, and the concern about cacheCount becoming negative is valid.
Analysis chain
Two new methods, increaseCache and decreaseCache, have been added to manage the cacheCount. Ensure that these methods are called appropriately whenever the cache state of a symbol changes. It's also important to ensure that cacheCount cannot become negative, as this could lead to incorrect behavior in the isAnimating method.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that cacheCount is not decremented below zero
ast-grep --lang cpp --pattern $'int decreaseCache() {
return --cacheCount;
}'
Length of output: 80
Script:
#!/bin/bash
# Search for the decreaseCache method within the SymbolAnimationCoordinator class
ast-grep --lang cpp --pattern $'class $_ {
$$$
int decreaseCache() {
$$$
}
$$$
}'
Length of output: 439
Script:
#!/bin/bash
# Use ripgrep to search for the decreaseCache method within the codebase
rg "int decreaseCache\(\)"
Length of output: 125
Script:
#!/bin/bash
# Extract the implementation details of the decreaseCache method
rg -A 3 -B 3 "int decreaseCache\(\)" shared/src/map/layers/tiled/vector/symbol/SymbolAnimationCoordinator.h
Length of output: 207
| @@ -78,7 +78,10 @@ void Tiled2dMapSource<T, L, R>::onVisibleBoundsChanged(const ::RectCoord &visibl | |||
| if (!zoomInfo.underzoom | |||
| && (zoomLevelInfos.empty() || zoomLevelInfos[0].zoom * zoomInfo.zoomLevelScaleFactor * screenScaleFactor < zoom) | |||
| && (zoomLevelInfos.empty() || zoomLevelInfos[0].zoomLevelIdentifier != 0)) { // enable underzoom if the first zoomLevel is zoomLevelIdentifier == 0 | |||
| onVisibleTilesChanged({}); | |||
| if (lastVisibleTilesHash != 0) { | |||
There was a problem hiding this comment.
Diese Änderung ist nötig, weil die GeoJson-Source sonst immer onVisibletilesChanged auslöst.
maerki
force-pushed
the
bugfix/ignore-animations-in-cache
branch
from
a972ec6 to
9c9a37c
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (9)
- shared/public/Tiled2dMapSource.h (1 hunks)
- shared/public/Tiled2dMapSourceImpl.h (1 hunks)
- shared/src/map/layers/tiled/vector/Tiled2dMapVectorLayer.cpp (2 hunks)
- shared/src/map/layers/tiled/vector/symbol/SymbolAnimationCoordinator.h (3 hunks)
- shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSourceSymbolDataManager.cpp (1 hunks)
- shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSymbolGroup.cpp (1 hunks)
- shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSymbolGroup.h (1 hunks)
- shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSymbolObject.cpp (1 hunks)
- shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSymbolObject.h (1 hunks)
Additional comments: 9
shared/src/map/layers/tiled/vector/symbol/SymbolAnimationCoordinator.h (1)
- 96-102: The methods
increaseCacheanddecreaseCachehave been added without checks to preventcacheCountfrom becoming negative. This issue was previously flagged and still needs to be addressed.shared/public/Tiled2dMapSource.h (1)
- 162-162: The
lastVisibleTilesHashmember variable is now initialized to-1. This change is consistent with the PR objectives to improve tile visibility change handling.shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSymbolObject.h (1)
- 59-60: The method
removeFromCachehas been added to theTiled2dMapVectorSymbolObjectclass. Ensure that this method is called appropriately to remove symbols from the cache when they are no longer needed.Verification successful
The
removeFromCache()method exists within theTiled2dMapVectorSymbolObjectclass and is being called in the codebase, indicating that it is integrated and used appropriately. The verification process is concluded with the findings aligning with the original review comment.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that removeFromCache is called when symbols are no longer needed ast-grep --lang cpp --pattern $'class Tiled2dMapVectorSymbolObject { $$$ void removeFromCache() { $$$ } $$$ }'Length of output: 468
Script:
#!/bin/bash # First, verify the existence of the class Tiled2dMapVectorSymbolObject ast-grep --lang cpp --pattern $'class Tiled2dMapVectorSymbolObject { $$$ }' # Then, if the class exists, check for the existence of the removeFromCache method within it ast-grep --lang cpp --pattern $'void Tiled2dMapVectorSymbolObject::removeFromCache() { $$$ }' # Lastly, search for calls to removeFromCache to verify its usage rg 'removeFromCache\(\)' --type cppLength of output: 1270
shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSymbolGroup.h (1)
- 68-69: The method
removeFromCachehas been added to theTiled2dMapVectorSymbolGroupclass. This method should be used to manage the cache effectively, removing symbol groups when they are no longer visible.Verification successful
The
removeFromCachemethod is indeed present in theTiled2dMapVectorSymbolGroupclass as well as in theTiled2dMapVectorSymbolObjectclass, as confirmed by the output of thergcommand. However, the initial task was to verify thatremoveFromCacheis called for symbol groups when they are no longer visible, which has not been confirmed yet.To complete the verification, I will now generate a script to search for calls to the
removeFromCachemethod within the codebase to ensure it is being used as intended.
The search for calls to the
removeFromCachemethod within the codebase has produced results indicating that the method is implemented in bothTiled2dMapVectorSymbolObject.cppandTiled2dMapVectorSymbolGroup.cpp. Additionally, there is a call toremoveFromCacheon agroupobject withinTiled2dMapVectorSourceSymbolDataManager.cpp, and a call toremoveFromCacheon anobjectwithinTiled2dMapVectorSymbolGroup.cpp.This confirms that the
removeFromCachemethod is not only present but also utilized within the codebase, which aligns with the intent expressed in the review comment to manage the cache effectively by removing symbol groups when they are no longer visible.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that removeFromCache is called for symbol groups when they are no longer visible ast-grep --lang cpp --pattern $'class Tiled2dMapVectorSymbolGroup { $$$ void removeFromCache() { $$$ } $$$ }'Length of output: 467
Script:
#!/bin/bash # First, confirm the existence of the removeFromCache method in the codebase ast-grep --lang cpp --pattern 'void removeFromCache() { $$$ }'Length of output: 64
Script:
#!/bin/bash # Broad search for the string "removeFromCache" to confirm its presence in the codebase rg 'void removeFromCache\(\)'Length of output: 232
Script:
#!/bin/bash # Search for calls to the removeFromCache method within the codebase rg 'removeFromCache\(\)'Length of output: 742
shared/public/Tiled2dMapSourceImpl.h (1)
- 81-84: The conditional check for
lastVisibleTilesHashbefore callingonVisibleTilesChangedhas been added. This change is consistent with the PR objectives to avoid unnecessary updates when the visible tiles have not changed.shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSourceSymbolDataManager.cpp (1)
- 533-544: The conditional block added checks if the tile state changes from 'CACHED' to another state and removes symbol groups from the cache accordingly. This is a critical change for the performance optimization mentioned in the PR objectives. Ensure that the removal of symbol groups from the cache is safe and does not lead to dangling pointers or memory leaks.
shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSymbolObject.cpp (2)
- 180-182: The
placedInCachemethod correctly callsincreaseCacheon theanimationCoordinator. This is consistent with the intent to manage cache counts for symbol animations.- 184-186: The
removeFromCachemethod properly callsdecreaseCacheon theanimationCoordinator. This aligns with the goal of removing symbol groups from the cache when their tile state changes.shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSymbolGroup.cpp (1)
- 844-848: The
removeFromCachemethod has been added to iterate throughsymbolObjectsand callremoveFromCacheon each object. Ensure that theremoveFromCachemethod on theTiled2dMapVectorSymbolObjectclass handles the removal process correctly, including any necessary cleanup or state updates.
| double zoomChange = abs(newZoom-lastDataManagerZoom) / std::max(newZoom, 1.0); | ||
| double timeDiff = now - lastDataManagerUpdate; | ||
| if (zoomChange > 0.001 || timeDiff > 2000 || isAnimating || tilesChanged) { |
There was a problem hiding this comment.
The calculation of zoomChange and timeDiff has been moved before the conditional check, which is a good practice as it avoids duplicate calculations inside the condition. However, the magic number 0.001 used in the zoom change comparison should be defined as a constant to improve readability and maintainability.
const double ZOOM_CHANGE_THRESHOLD = 0.001;
...
double zoomChange = abs(newZoom - lastDataManagerZoom) / std::max(newZoom, 1.0);
double timeDiff = now - lastDataManagerUpdate;
if (zoomChange > ZOOM_CHANGE_THRESHOLD || timeDiff > 2000 || isAnimating || tilesChanged) {
...
}| @@ -556,7 +558,7 @@ void Tiled2dMapVectorLayer::update() { | |||
| newIsAnimating |= a; | |||
| } | |||
| isAnimating = newIsAnimating; | |||
| if (now - lastCollitionCheck > 2000 || tilesChanged) { | |||
| if (now - lastCollitionCheck > 3000 || tilesChanged) { | |||
There was a problem hiding this comment.
The time threshold for lastCollitionCheck has been increased from 2000 to 3000. This change seems to be in line with the PR objectives of optimizing performance by reducing the frequency of collision checks. However, the magic number 3000 should also be defined as a constant for clarity and ease of future changes.
const long long COLLISION_CHECK_THRESHOLD = 3000;
...
if (now - lastCollitionCheck > COLLISION_CHECK_THRESHOLD || tilesChanged) {
...
}| if (cacheCount >= usageCount) { | ||
| return false; | ||
|
|
||
| } | ||
| if (isIconAnimating() || isStretchIconAnimating() || isTextAnimating()) { | ||
| // printf("Animate %p: %d, %d, %d, %d: %f|%f\n", static_cast<void*>(this), isIconAnimating(), isStretchIconAnimating(), isTextAnimating(), isUsed(), coordinate.x, coordinate.y); | ||
| return true; | ||
| } | ||
| else { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The isAnimating method has been updated to include a check against cacheCount. This change is intended to prevent animations for symbols that are cached and not currently visible. However, there is no check to ensure cacheCount does not become negative, which could lead to incorrect behavior.
int decreaseCache() {
if (cacheCount <= 0) {
// Handle error or prevent decrement
return cacheCount;
}
return --cacheCount;
}|
|
||
| } | ||
| if (isIconAnimating() || isStretchIconAnimating() || isTextAnimating()) { | ||
| // printf("Animate %p: %d, %d, %d, %d: %f|%f\n", static_cast<void*>(this), isIconAnimating(), isStretchIconAnimating(), isTextAnimating(), isUsed(), coordinate.x, coordinate.y); |
Summary by CodeRabbit
Performance Improvements
New Features
Bug Fixes