From dbcc269844e99b04af73f55c846d49a09665952f Mon Sep 17 00:00:00 2001 From: Aparna Mohan Date: Tue, 4 Aug 2026 20:08:13 -0500 Subject: [PATCH 1/2] Fix stale QuadItem removal after ClusterItem position updates --- .../algo/NonHierarchicalDistanceBasedAlgorithm.kt | 11 +++++++---- .../google/maps/android/clustering/QuadItemTest.java | 5 ++++- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/clustering/src/main/java/com/google/maps/android/clustering/algo/NonHierarchicalDistanceBasedAlgorithm.kt b/clustering/src/main/java/com/google/maps/android/clustering/algo/NonHierarchicalDistanceBasedAlgorithm.kt index 7a8901081..3bfc6cda7 100644 --- a/clustering/src/main/java/com/google/maps/android/clustering/algo/NonHierarchicalDistanceBasedAlgorithm.kt +++ b/clustering/src/main/java/com/google/maps/android/clustering/algo/NonHierarchicalDistanceBasedAlgorithm.kt @@ -47,6 +47,7 @@ open class NonHierarchicalDistanceBasedAlgorithm : AbstractAlgo */ @JvmField protected val mItems: MutableCollection> = LinkedHashSet() + protected val mItemMap = HashMap>() /** * Any modifications should be synchronized on mQuadTree. @@ -61,6 +62,7 @@ open class NonHierarchicalDistanceBasedAlgorithm : AbstractAlgo synchronized(mQuadTree) { val result = mItems.add(quadItem) if (result) { + mItemMap[item] = quadItem mQuadTree.add(quadItem) } return result @@ -81,6 +83,7 @@ open class NonHierarchicalDistanceBasedAlgorithm : AbstractAlgo override fun clearItems() { synchronized(mQuadTree) { mItems.clear() + mItemMap.clear() mQuadTree.clear() } } @@ -88,9 +91,9 @@ open class NonHierarchicalDistanceBasedAlgorithm : AbstractAlgo override fun removeItem(item: T): Boolean { // QuadItem delegates hashcode() and equals() to its item so, // removing any QuadItem to that item will remove the item - val quadItem = QuadItem(item) synchronized(mQuadTree) { - val result = mItems.remove(quadItem) + val quadItem = mItemMap.remove(item) + val result = quadItem != null && mItems.remove(quadItem) if (result) { mQuadTree.remove(quadItem) } @@ -104,8 +107,8 @@ open class NonHierarchicalDistanceBasedAlgorithm : AbstractAlgo for (item in items) { // QuadItem delegates hashcode() and equals() to its item so, // removing any QuadItem to that item will remove the item - val quadItem = QuadItem(item) - val individualResult = mItems.remove(quadItem) + val quadItem = mItemMap.remove(item) + val individualResult = quadItem != null && mItems.remove(quadItem) if (individualResult) { mQuadTree.remove(quadItem) result = true diff --git a/clustering/src/test/java/com/google/maps/android/clustering/QuadItemTest.java b/clustering/src/test/java/com/google/maps/android/clustering/QuadItemTest.java index 7661da02a..a0010af8e 100644 --- a/clustering/src/test/java/com/google/maps/android/clustering/QuadItemTest.java +++ b/clustering/src/test/java/com/google/maps/android/clustering/QuadItemTest.java @@ -98,7 +98,7 @@ public void testInsertionOrder() { } private static class TestingItem implements ClusterItem { - private final LatLng mPosition; + private LatLng mPosition; private String mTitle; TestingItem(String title, double lat, double lng) { @@ -110,6 +110,9 @@ private static class TestingItem implements ClusterItem { mTitle = ""; mPosition = new LatLng(lat, lng); } + public void setPosition(double lat, double lng) { + mPosition = new LatLng(lat, lng); + } @NonNull @Override From fb83466ec8dfb254da8777bec92da6728935ba8d Mon Sep 17 00:00:00 2001 From: Dale Hawkins <107309+dkhawk@users.noreply.github.com> Date: Tue, 4 Aug 2026 15:22:45 -0600 Subject: [PATCH 2/2] test(clustering): add regression tests and demo for stale QuadItem removal (#1730) - Expand QuadItemTest with comprehensive unit tests for single removal, bulk removal, clearing, and coordinate-boundary updates after a mutable ClusterItem changes position. - Add fallback to QuadItem(item) in NonHierarchicalDistanceBasedAlgorithm.kt removeItem/removeItems for subclass robustness if mItemMap is bypassed. - Enhance ClusteringDiffDemoActivity to mutate ClusterItem coordinates in-place and include background filler markers to trigger PointQuadTree quadrant splitting, demonstrating the fix on-device. --- .../NonHierarchicalDistanceBasedAlgorithm.kt | 8 +- .../maps/android/clustering/QuadItemTest.java | 109 ++++++++++++++++++ .../demo/ClusteringDiffDemoActivity.java | 9 +- .../maps/android/utils/demo/model/Person.java | 6 +- 4 files changed, 126 insertions(+), 6 deletions(-) diff --git a/clustering/src/main/java/com/google/maps/android/clustering/algo/NonHierarchicalDistanceBasedAlgorithm.kt b/clustering/src/main/java/com/google/maps/android/clustering/algo/NonHierarchicalDistanceBasedAlgorithm.kt index 3bfc6cda7..91db577e8 100644 --- a/clustering/src/main/java/com/google/maps/android/clustering/algo/NonHierarchicalDistanceBasedAlgorithm.kt +++ b/clustering/src/main/java/com/google/maps/android/clustering/algo/NonHierarchicalDistanceBasedAlgorithm.kt @@ -92,8 +92,8 @@ open class NonHierarchicalDistanceBasedAlgorithm : AbstractAlgo // QuadItem delegates hashcode() and equals() to its item so, // removing any QuadItem to that item will remove the item synchronized(mQuadTree) { - val quadItem = mItemMap.remove(item) - val result = quadItem != null && mItems.remove(quadItem) + val quadItem = mItemMap.remove(item) ?: QuadItem(item) + val result = mItems.remove(quadItem) if (result) { mQuadTree.remove(quadItem) } @@ -107,8 +107,8 @@ open class NonHierarchicalDistanceBasedAlgorithm : AbstractAlgo for (item in items) { // QuadItem delegates hashcode() and equals() to its item so, // removing any QuadItem to that item will remove the item - val quadItem = mItemMap.remove(item) - val individualResult = quadItem != null && mItems.remove(quadItem) + val quadItem = mItemMap.remove(item) ?: QuadItem(item) + val individualResult = mItems.remove(quadItem) if (individualResult) { mQuadTree.remove(quadItem) result = true diff --git a/clustering/src/test/java/com/google/maps/android/clustering/QuadItemTest.java b/clustering/src/test/java/com/google/maps/android/clustering/QuadItemTest.java index a0010af8e..8d6b12ef4 100644 --- a/clustering/src/test/java/com/google/maps/android/clustering/QuadItemTest.java +++ b/clustering/src/test/java/com/google/maps/android/clustering/QuadItemTest.java @@ -97,6 +97,115 @@ public void testInsertionOrder() { } } + @Test + public void testUpdateItemAfterPositionChange() { + NonHierarchicalDistanceBasedAlgorithm algo = + new NonHierarchicalDistanceBasedAlgorithm<>(); + TestingItem item = new TestingItem("title1", 0.0, 0.0); + algo.addItem(item); + assertEquals(1, algo.getItems().size()); + + // Update the position of the mutable item + item.setPosition(10.0, 10.0); + + // Call updateItem + assertTrue("updateItem should return true after position change", algo.updateItem(item)); + assertEquals(1, algo.getItems().size()); + + // Verify that the old QuadItem at (0, 0) was removed from the tree + // and only the new position (10, 10) is indexed + java.util.Set> clusters = algo.getClusters(4.0f); + assertEquals(1, clusters.size()); + Cluster cluster = clusters.iterator().next(); + assertEquals(10.0, cluster.getPosition().latitude, 0.001); + assertEquals(10.0, cluster.getPosition().longitude, 0.001); + } + + @Test + public void testRemoveItemAfterPositionChange() { + NonHierarchicalDistanceBasedAlgorithm algo = + new NonHierarchicalDistanceBasedAlgorithm<>(); + TestingItem item = new TestingItem("title1", 0.0, 0.0); + algo.addItem(item); + assertEquals(1, algo.getItems().size()); + + // Update the position of the mutable item + item.setPosition(10.0, 10.0); + + // Removing the item should succeed and remove it from the tree + assertTrue("removeItem should return true after position change", algo.removeItem(item)); + assertEquals(0, algo.getItems().size()); + assertEquals(0, algo.getClusters(4.0f).size()); + } + + @Test + public void testUpdateItemPreventsStaleQuadTreeEntries() { + TestAlgorithm algo = new TestAlgorithm<>(); + + // Add 60 filler items to force PointQuadTree to split (MAX_ELEMENTS = 50) + for (int i = 0; i < 60; i++) { + algo.addItem(new TestingItem("filler" + i, 10.0 + i * 0.001, 10.0 + i * 0.001)); + } + + // Add item1 in top-left quadrant + TestingItem item1 = new TestingItem("item1", 1.0, 1.0); + algo.addItem(item1); + + assertEquals("QuadTree should contain item1 at (1, 1)", 1, algo.getQuadTreeItemCount(1.0, 1.0, 0.001)); + + // Move item1 far across quadrant boundary to (50.0, 50.0) and update + item1.setPosition(50.0, 50.0); + algo.updateItem(item1); + + // Without fix, old QuadItem remains at (1.0, 1.0) in mQuadTree because remove traversed the new coordinates + assertEquals("QuadTree should NOT contain stale entry at (1, 1) after update", 0, algo.getQuadTreeItemCount(1.0, 1.0, 0.001)); + assertEquals("QuadTree should contain item1 at (50, 50)", 1, algo.getQuadTreeItemCount(50.0, 50.0, 0.001)); + } + + @Test + public void testRemoveItemsAfterPositionChange() { + NonHierarchicalDistanceBasedAlgorithm algo = + new NonHierarchicalDistanceBasedAlgorithm<>(); + TestingItem item1 = new TestingItem("title1", 0.0, 0.0); + TestingItem item2 = new TestingItem("title2", 1.0, 1.0); + algo.addItems(java.util.Arrays.asList(item1, item2)); + assertEquals(2, algo.getItems().size()); + + // Update the position of both items + item1.setPosition(10.0, 10.0); + item2.setPosition(20.0, 20.0); + + assertTrue("removeItems should return true after position change", + algo.removeItems(java.util.Arrays.asList(item1, item2))); + assertEquals(0, algo.getItems().size()); + assertEquals(0, algo.getClusters(4.0f).size()); + } + + @Test + public void testClearItemsAfterPositionChange() { + NonHierarchicalDistanceBasedAlgorithm algo = + new NonHierarchicalDistanceBasedAlgorithm<>(); + TestingItem item1 = new TestingItem("title1", 0.0, 0.0); + algo.addItem(item1); + item1.setPosition(10.0, 10.0); + + algo.clearItems(); + assertEquals(0, algo.getItems().size()); + assertEquals(0, algo.getClusters(4.0f).size()); + } + + private static class TestAlgorithm extends NonHierarchicalDistanceBasedAlgorithm { + private static final com.google.maps.android.projection.SphericalMercatorProjection PROJ = + new com.google.maps.android.projection.SphericalMercatorProjection(1.0); + + public int getQuadTreeItemCount(double lat, double lng, double span) { + com.google.maps.android.geometry.Point p = PROJ.toPoint(new LatLng(lat, lng)); + com.google.maps.android.geometry.Bounds bounds = new com.google.maps.android.geometry.Bounds( + p.x - span, p.x + span, p.y - span, p.y + span); + return mQuadTree.search(bounds).size(); + } + } + private static class TestingItem implements ClusterItem { private LatLng mPosition; private String mTitle; diff --git a/demo/src/main/java/com/google/maps/android/utils/demo/ClusteringDiffDemoActivity.java b/demo/src/main/java/com/google/maps/android/utils/demo/ClusteringDiffDemoActivity.java index 900bdaa3a..da6e0bb0f 100644 --- a/demo/src/main/java/com/google/maps/android/utils/demo/ClusteringDiffDemoActivity.java +++ b/demo/src/main/java/com/google/maps/android/utils/demo/ClusteringDiffDemoActivity.java @@ -161,6 +161,13 @@ protected void startDemo(boolean isRestore) { } private void addItems() { + // Add 60 filler background markers across Greater London to force PointQuadTree to split into quadrants (MAX_ELEMENTS = 50) + for (int i = 0; i < 60; i++) { + double lat = 51.3 + (i % 10) * 0.05; + double lng = -0.4 + (i / 10) * 0.08; + mClusterManager.addItem(new Person(new LatLng(lat, lng), "Citizen " + i, R.drawable.john)); + } + // Marker in Enfield mClusterManager.addItem(new Person(City.ENFIELD.latLng, "John", R.drawable.john)); @@ -181,7 +188,7 @@ private void rotateLocation() { Log.d("ClusterTest", "Item rotated to: " + newLocation.toString() + ", City: " + cityName); if (itemToUpdate != null) { - itemToUpdate = new Person(newLocation, "Teach", R.drawable.teacher); + itemToUpdate.setPosition(newLocation); mClusterManager.updateItem(itemToUpdate); // Update the marker mClusterManager.cluster(); } diff --git a/demo/src/main/java/com/google/maps/android/utils/demo/model/Person.java b/demo/src/main/java/com/google/maps/android/utils/demo/model/Person.java index 95a63f4cc..b68cf5b48 100644 --- a/demo/src/main/java/com/google/maps/android/utils/demo/model/Person.java +++ b/demo/src/main/java/com/google/maps/android/utils/demo/model/Person.java @@ -24,7 +24,7 @@ public class Person implements ClusterItem { public final String name; public final int profilePhoto; - private final LatLng mPosition; + private LatLng mPosition; public Person(LatLng position, String name, int pictureResource) { this.name = name; @@ -32,6 +32,10 @@ public Person(LatLng position, String name, int pictureResource) { mPosition = position; } + public void setPosition(LatLng position) { + mPosition = position; + } + @NonNull @Override public LatLng getPosition() {