Skip to content
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.

Sign up for GitHub

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

ignore animations that are only cached symbols #571

Merged
merged 2 commits into from
Jan 24, 2024
Merged

ignore animations that are only cached symbols #571

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9c9a37c
ignore animations that are only cached symbols
maerki Jan 13, 2024
47cec33
Update SymbolAnimationCoordinator.h
maerki Jan 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion shared/public/Tiled2dMapSource.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ class Tiled2dMapSource :
float screenDensityPpi;
std::set<Tiled2dMapTileInfo> readyTiles;

size_t lastVisibleTilesHash;
size_t lastVisibleTilesHash = -1;

void onVisibleTilesChanged(const std::vector<VisibleTilesLayer> &pyramid, int keepZoomLevelOffset = 0);

Expand Down
5 changes: 4 additions & 1 deletion shared/public/Tiled2dMapSourceImpl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Diese Änderung ist nötig, weil die GeoJson-Source sonst immer onVisibletilesChanged auslöst.

lastVisibleTilesHash = 0;
onVisibleTilesChanged({});
}
return;
}

Expand Down
6 changes: 4 additions & 2 deletions shared/src/map/layers/tiled/vector/Tiled2dMapVectorLayer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -541,7 +541,9 @@ void Tiled2dMapVectorLayer::update() {
auto now = DateHelper::currentTimeMillis();
bool newIsAnimating = false;
bool tilesChanged = !tilesStillValid.test_and_set();
if (abs(newZoom-lastDataManagerZoom) / std::max(newZoom, 1.0) > 0.001 || now - lastDataManagerUpdate > 1000 || isAnimating || tilesChanged) {
double zoomChange = abs(newZoom-lastDataManagerZoom) / std::max(newZoom, 1.0);
double timeDiff = now - lastDataManagerUpdate;
if (zoomChange > 0.001 || timeDiff > 2000 || isAnimating || tilesChanged) {
Comment on lines +544 to +546
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
    ...
}

lastDataManagerUpdate = now;
lastDataManagerZoom = newZoom;

Expand All @@ -556,7 +558,7 @@ void Tiled2dMapVectorLayer::update() {
newIsAnimating |= a;
}
isAnimating = newIsAnimating;
if (now - lastCollitionCheck > 2000 || tilesChanged) {
if (now - lastCollitionCheck > 3000 || tilesChanged) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
    ...
}

lastCollitionCheck = now;
bool enforceUpdate = !prevCollisionStillValid.test_and_set();
collisionManager.syncAccess([&vpMatrix, &viewportSize, viewportRotation, enforceUpdate](const auto &manager) {
Expand Down
21 changes: 20 additions & 1 deletion shared/src/map/layers/tiled/vector/symbol/SymbolAnimationCoordinator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,17 @@ class SymbolAnimationCoordinator {
}

bool isAnimating() {
return isIconAnimating() || isStretchIconAnimating() || isTextAnimating();
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

debug log

return true;
}
else {
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}

}

bool isIconAnimating() {
Expand Down Expand Up @@ -83,6 +93,14 @@ class SymbolAnimationCoordinator {
return --usageCount;
}

int increaseCache() {;
return ++cacheCount;
}

int decreaseCache() {
return --cacheCount;
}
Comment on lines +95 to +101
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


std::atomic_flag isOwned = ATOMIC_FLAG_INIT;

// returns true if the value was changed
Expand Down Expand Up @@ -112,6 +130,7 @@ class SymbolAnimationCoordinator {
float lastTextAlpha = 0;

std::atomic_int usageCount = 0;
std::atomic_int cacheCount = 0;

bool collides = true;

Expand Down
12 changes: 12 additions & 0 deletions shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSourceSymbolDataManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -530,6 +530,18 @@ void Tiled2dMapVectorSourceSymbolDataManager::updateSymbolGroups() {
if (tileStateIt == tileStateMap.end()) {
tileStateMap[tile] = state;
} else {
if (tileStateIt->second == TileState::CACHED && state != TileState::CACHED) {
auto tileSymbolGroupMapIt = tileSymbolGroupMap.find(tile);
if (tileSymbolGroupMapIt != tileSymbolGroupMap.end()) {
for (const auto &[layerIdentifier, symbolGroups]: tileSymbolGroupMapIt->second) {
for (auto &symbolGroup: std::get<1>(symbolGroups)) {
symbolGroup.syncAccess([&](auto group) {
group->removeFromCache();
});
}
}
}
}
tileStateIt->second = state;
}
if (state == TileState::CACHED) {
Expand Down
6 changes: 6 additions & 0 deletions shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSymbolGroup.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -841,6 +841,12 @@ void Tiled2dMapVectorSymbolGroup::placedInCache() {
}
}

void Tiled2dMapVectorSymbolGroup::removeFromCache() {
for (auto const object: symbolObjects) {
object->removeFromCache();
}
}


std::vector<std::shared_ptr< ::RenderObjectInterface>> Tiled2dMapVectorSymbolGroup::getRenderObjects() {
std::vector<std::shared_ptr< ::RenderObjectInterface>> renderObjects;
Expand Down
2 changes: 2 additions & 0 deletions shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSymbolGroup.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ class Tiled2dMapVectorSymbolGroup : public ActorObject, public std::enable_share

void placedInCache();

void removeFromCache();

void clear();

void updateLayerDescription(const std::shared_ptr<SymbolVectorLayerDescription> layerDescription);
Expand Down
7 changes: 7 additions & 0 deletions shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSymbolObject.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,13 @@ void Tiled2dMapVectorSymbolObject::placedInCache() {
animationCoordinator->isOwned.clear();
isCoordinateOwner = false;
}
animationCoordinator->increaseCache();
}
}

void Tiled2dMapVectorSymbolObject::removeFromCache() {
if (animationCoordinator) {
animationCoordinator->decreaseCache();
}
}

Expand Down
2 changes: 2 additions & 0 deletions shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSymbolObject.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ class Tiled2dMapVectorSymbolObject {

void placedInCache();

void removeFromCache();

struct SymbolObjectInstanceCounts { int icons, textCharacters, stretchedIcons; };

const SymbolObjectInstanceCounts getInstanceCounts() const;
Expand Down
Loading