Skip to content

also lock access to camerazoomconfig #893

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

Draft
maerki wants to merge 12 commits into
base: main
Choose a base branch
from
Draft

also lock access to camerazoomconfig #893

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
32e3776
also lock access to camerazoomconfig
maerki Dec 3, 2025
14212b6
Initial plan
Copilot Dec 4, 2025
7db85ec
Fix nested locking by creating unlocked helper methods
Copilot Dec 4, 2025
5137a0f
Changes before error encountered
Copilot Dec 4, 2025
338eea5
freeze camera during de-init to stop potential animations
maerki Dec 10, 2025
fffb3ce
delete codeql
maerki Dec 10, 2025
1a071da
delete codeql
maerki Dec 10, 2025
363ad6a
delete codeql
maerki Dec 10, 2025
1e3f522
Delete _codeql_build_dir/_deps directory
maerki Dec 10, 2025
a8a3734
Merge pull request #894 from openmobilemaps/copilot/sub-pr-893
maerki Dec 10, 2025
3839d45
Merge branch 'main' into bugfix/lock-all-config-acces
maerki Dec 10, 2025
2486038
fix build
maerki Dec 10, 2025
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
5 changes: 5 additions & 0 deletions shared/src/map/camera/MapCamera2d.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,11 @@ MapCamera2d::MapCamera2d(const std::shared_ptr<MapInterface> &mapInterface, floa
zoom = zoomMax;
}

MapCamera2d::~MapCamera2d() {
// Stop any ongoing animations to avoid callbacks after destruction.
freeze(true);
}

void MapCamera2d::viewportSizeChanged() {
Vec2I viewportSize = mapInterface->getRenderingContext()->getViewportSize();
if (viewportSize.x > 0 && viewportSize.y > 0 && zoomMin < 0) {
Expand Down
2 changes: 1 addition & 1 deletion shared/src/map/camera/MapCamera2d.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ class MapCamera2d : public MapCameraInterface,
public:
MapCamera2d(const std::shared_ptr<MapInterface> &mapInterface, float screenDensityPpi);

~MapCamera2d(){};
~MapCamera2d();

void freeze(bool freeze) override;

Expand Down
71 changes: 62 additions & 9 deletions shared/src/map/camera/MapCamera3d.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,11 @@ MapCamera3d::MapCamera3d(const std::shared_ptr<MapInterface> &mapInterface, floa
updateZoom(GLOBE_MIN_ZOOM);
}

MapCamera3d::~MapCamera3d() {
// Ensure no animation callback touches a partially destroyed camera.
freeze(true);
}

void MapCamera3d::viewportSizeChanged() {
auto mapInterface = this->mapInterface;
auto renderingContext = mapInterface ? mapInterface->getRenderingContext() : nullptr;
Expand Down Expand Up @@ -114,8 +119,14 @@ void MapCamera3d::moveToCenterPositionZoom(const ::Coord &centerPosition, double
if (animated) {
std::weak_ptr<MapCamera3d> weakSelf = std::static_pointer_cast<MapCamera3d>(shared_from_this());
std::lock_guard<std::recursive_mutex> lock(animationMutex);
long long animationDurationMs;
{
std::lock_guard<std::recursive_mutex> lock(paramMutex);
animationDurationMs = cameraZoomConfig.animationDurationMs;
Comment on lines 116 to +125
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot Dec 3, 2025

Choose a reason for hiding this comment

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

P1 Badge Align mutex locking order to avoid deadlock

The new animation-duration read in moveToCenterPositionZoom acquires animationMutex and then paramMutex, while other animated entry points such as setZoom (lines 293–300) and setRotation (lines 337–344) take paramMutex before animationMutex. If two threads call these functions concurrently, one can hold animationMutex while waiting for paramMutex and the other the reverse, producing a deadlock. Lock the mutexes in a consistent order or avoid the nested locking to keep concurrent calls safe.

Useful? React with 👍 / 👎.

}

coordAnimation = std::make_shared<CoordAnimation>(
cameraZoomConfig.animationDurationMs, focusPointPosition, focusPosition, centerPosition,
animationDurationMs, focusPointPosition, focusPosition, centerPosition,
InterpolatorFunction::EaseInOut,
[weakSelf](Coord positionMapSystem) {
if (auto selfPtr = weakSelf.lock()) {
Expand Down Expand Up @@ -187,8 +198,14 @@ void MapCamera3d::moveToCenterPosition(const ::Coord &centerPosition, bool anima
if (animated) {
std::weak_ptr<MapCamera3d> weakSelf = std::static_pointer_cast<MapCamera3d>(shared_from_this());
std::lock_guard<std::recursive_mutex> lock(animationMutex);
long long animationDurationMs;
{
std::lock_guard<std::recursive_mutex> lock(paramMutex);
animationDurationMs = cameraZoomConfig.animationDurationMs;
}

coordAnimation = std::make_shared<CoordAnimation>(
cameraZoomConfig.animationDurationMs, focusPointPosition, focusPosition, centerPosition,
animationDurationMs, focusPointPosition, focusPosition, centerPosition,
InterpolatorFunction::EaseInOut,
[weakSelf](Coord positionMapSystem) {
if (auto selfPtr = weakSelf.lock()) {
Expand Down Expand Up @@ -251,7 +268,11 @@ void MapCamera3d::moveToBoundingBox(const RectCoord &boundingBox, float paddingP
// the user of moveToBoundingBox wants that the bounding box is centered,
// if you have camera verticaldisplacement, this is not the case, this should
// be fixed for this function as it only works with 0
assert(valueForZoom(cameraZoomConfig.verticalDisplacementInterpolationValues, zoom) == 0);
{
// Protect against concurrent config updates while reading interpolation values
std::lock_guard<std::recursive_mutex> lock(paramMutex);
assert(valueForZoom(cameraZoomConfig.verticalDisplacementInterpolationValues, zoom) == 0);
}

auto x = 0.5 * boundingBox.topLeft.x + 0.5 * boundingBox.bottomRight.x;
auto y = 0.5 * boundingBox.topLeft.y + 0.5 * boundingBox.bottomRight.y;
Expand All @@ -275,9 +296,15 @@ void MapCamera3d::setZoom(double zoom, bool animated) {

if (animated) {
std::weak_ptr<MapCamera3d> weakSelf = std::static_pointer_cast<MapCamera3d>(shared_from_this());
long long animationDurationMs;
{
std::lock_guard<std::recursive_mutex> lock(paramMutex);
animationDurationMs = cameraZoomConfig.animationDurationMs;
}

std::lock_guard<std::recursive_mutex> lock(animationMutex);
zoomAnimation = std::make_shared<DoubleAnimation>(
cameraZoomConfig.animationDurationMs, this->zoom, targetZoom, InterpolatorFunction::EaseIn,
animationDurationMs, this->zoom, targetZoom, InterpolatorFunction::EaseIn,
[weakSelf](double zoom) {
if (auto selfPtr = weakSelf.lock()) {
selfPtr->setZoom(zoom, false);
Expand Down Expand Up @@ -313,9 +340,15 @@ void MapCamera3d::setRotation(float angle, bool animated) {
newAngle -= 360.0;
}
std::weak_ptr<MapCamera3d> weakSelf = std::static_pointer_cast<MapCamera3d>(shared_from_this());
long long animationDurationMs;
{
std::lock_guard<std::recursive_mutex> lock(paramMutex);
animationDurationMs = cameraZoomConfig.animationDurationMs;
}

std::lock_guard<std::recursive_mutex> lock(animationMutex);
rotationAnimation = std::make_shared<DoubleAnimation>(
cameraZoomConfig.animationDurationMs, currentAngle, newAngle, InterpolatorFunction::Linear,
animationDurationMs, currentAngle, newAngle, InterpolatorFunction::Linear,
[weakSelf](double angle) {
if (auto selfPtr = weakSelf.lock()) {
selfPtr->setRotation(angle, false);
Expand Down Expand Up @@ -1169,9 +1202,16 @@ bool MapCamera3d::onTwoFingerMoveComplete() {

if (config.snapToNorthEnabled && !cameraFrozen && (angle < ROTATION_LOCKING_ANGLE || angle > (360 - ROTATION_LOCKING_ANGLE))) {
std::weak_ptr<MapCamera3d> weakSelf = std::static_pointer_cast<MapCamera3d>(shared_from_this());

long long animationDurationMs;
{
std::lock_guard<std::recursive_mutex> lock(paramMutex);
animationDurationMs = cameraZoomConfig.animationDurationMs;
}

std::lock_guard<std::recursive_mutex> lock(animationMutex);
rotationAnimation = std::make_shared<DoubleAnimation>(
cameraZoomConfig.animationDurationMs, this->angle, angle < ROTATION_LOCKING_ANGLE ? 0 : 360,
animationDurationMs, this->angle, angle < ROTATION_LOCKING_ANGLE ? 0 : 360,
InterpolatorFunction::EaseInOut,
[weakSelf](double angle) {
if (auto selfPtr = weakSelf.lock()) {
Expand Down Expand Up @@ -1769,7 +1809,10 @@ void MapCamera3d::setCameraConfig(const Camera3dConfig &config, std::optional<fl
}
}

Camera3dConfig MapCamera3d::getCameraConfig() { return cameraZoomConfig; }
Camera3dConfig MapCamera3d::getCameraConfig() {
std::lock_guard<std::recursive_mutex> lock(paramMutex);
return cameraZoomConfig;
}

void MapCamera3d::notifyListenerBoundsChange() { notifyListeners(ListenerType::BOUNDS); }

Expand All @@ -1779,15 +1822,25 @@ void MapCamera3d::updateZoom(double zoom_) {

std::lock_guard<std::recursive_mutex> lock(paramMutex);
zoom = std::clamp(zoom_, zoomMax, zoomMin);
cameraVerticalDisplacement = getCameraVerticalDisplacement();
cameraPitch = getCameraPitch();
cameraVerticalDisplacement = getCameraVerticalDisplacementUnlocked();
cameraPitch = getCameraPitchUnlocked();
}

double MapCamera3d::getCameraVerticalDisplacement() {
std::lock_guard<std::recursive_mutex> lock(paramMutex);
return getCameraVerticalDisplacementUnlocked();
}

double MapCamera3d::getCameraVerticalDisplacementUnlocked() {
return valueForZoom(cameraZoomConfig.verticalDisplacementInterpolationValues, zoom);
}

double MapCamera3d::getCameraPitch() {
std::lock_guard<std::recursive_mutex> lock(paramMutex);
return getCameraPitchUnlocked();
}

double MapCamera3d::getCameraPitchUnlocked() {
return valueForZoom(cameraZoomConfig.pitchInterpolationValues, zoom);
Comment on lines 1830 to 1844
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

The new lock added here creates unnecessary nested locking. The function updateZoom() already holds paramMutex (line 1818), but now getCameraVerticalDisplacement() and getCameraPitch() (called at lines 1820-1821) also try to acquire it.

While this is safe due to std::recursive_mutex, it's inefficient and indicates a design issue. Consider either:

  1. Making private helper versions of these functions that assume the lock is already held, OR
  2. Releasing the lock before calling these functions and re-acquiring it after

This would improve both performance and code clarity.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@maerki maerki Dec 4, 2025

Choose a reason for hiding this comment

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

@copilot open a new pull request to apply changes based on this feedback

}

Expand Down
9 changes: 6 additions & 3 deletions shared/src/map/camera/MapCamera3d.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,7 @@ class MapCamera3d : public MapCameraInterface,
public std::enable_shared_from_this<MapCamera3d> {
public:
MapCamera3d(const std::shared_ptr<MapInterface> &mapInterface, float screenDensityPpi);

~MapCamera3d(){};
~MapCamera3d();

void freeze(bool freeze) override;

Expand Down Expand Up @@ -195,6 +194,11 @@ class MapCamera3d : public MapCameraInterface,
double getCameraVerticalDisplacement();
double getCameraPitch();

private:
// Unlocked versions - caller must hold paramMutex
double getCameraVerticalDisplacementUnlocked();
double getCameraPitchUnlocked();

Vec2F calculateDistance(double latTopLeft, double lonTopLeft, double latBottomRight, double lonBottomRight);
double haversineDistance(double lat1, double lon1, double lat2, double lon2);
double zoomForMeterWidth(Vec2I sizeViewport, Vec2F sizeMeters);
Expand All @@ -213,7 +217,6 @@ class MapCamera3d : public MapCameraInterface,
Coord focusPointPosition;
double cameraVerticalDisplacement = 0.0;
double cameraPitch = 0; // looking up or down
double fieldOfView = 10.0f;

double zoom;
double angle = 0;
Expand Down