diff --git a/README.md b/README.md index 33aa806..5d7cd4d 100644 --- a/README.md +++ b/README.md @@ -267,7 +267,7 @@ See the [docs](https://gmi-software.github.io/react-native-better-clustering/doc | New Architecture errors | Confirm New Architecture is enabled and rebuild. | | Does not work in Expo Go | Use a [development build](https://docs.expo.dev/develop/development-builds/introduction/). | | `native ClusterEngine module is not available` | Rebuild the native app after installing (`pod install` / `npx expo prebuild --clean`). See [troubleshooting](https://gmi-software.github.io/react-native-better-clustering/docs/troubleshooting#the-native-clusterengine-module-is-not-available). | -| Markers flicker on zoom | Memoize marker components; give each point a stable `id`. | +| Markers flicker on zoom | Give each `Marker` a stable `key` (not the array index); memoize marker components. | More in [troubleshooting](https://gmi-software.github.io/react-native-better-clustering/docs/troubleshooting). diff --git a/docs/docs/api/types.md b/docs/docs/api/types.md index 76c7b21..9d0c126 100644 --- a/docs/docs/api/types.md +++ b/docs/docs/api/types.md @@ -73,5 +73,9 @@ import { } from 'react-native-better-clustering/engine' ``` +Each `EngineClusterNode` has `minLeafId`: the smallest point id among its leaves +(its own `id` for a point). A cluster keeps it across zoom levels while it only +gains or loses other leaves, so it can key the cluster's marker. + **Lifecycle:** `setOptions` → `setPoints` (`packPoints()` buffer) → `build()` → query. Query methods throw when `isBuilt` is `false`; `setPoints` throws on invalid buffers. diff --git a/docs/docs/troubleshooting.md b/docs/docs/troubleshooting.md index 4e3043e..8920cdf 100644 --- a/docs/docs/troubleshooting.md +++ b/docs/docs/troubleshooting.md @@ -71,8 +71,14 @@ const geoJson = useMemo( ## Markers flicker on zoom -Use `stabilizeClusterFeatures` from `/hooks`, memoize marker components, and -ensure each point has a stable `id`. +Give every `Marker` a stable `key` (its id, not its array index). `MapView` +keeps that key, so adding or removing a marker doesn't remount the others, and +it keys each cluster bubble by its first marker's key, so a cluster that splits +or merges on zoom, or survives a data update, keeps its native marker instead +of being removed and re-added. Memoize custom marker components too. + +With `/hooks`, use `stabilizeClusterFeatures` and key your markers by a stable +point `id`. ## Some markers never appear on the map diff --git a/package/cpp/ClusterEngineCore.hpp b/package/cpp/ClusterEngineCore.hpp index fbb62ed..050f1ca 100644 --- a/package/cpp/ClusterEngineCore.hpp +++ b/package/cpp/ClusterEngineCore.hpp @@ -237,6 +237,7 @@ class ClusterEngineCore { node.pointIndex = _points[i].id; node.isCluster = false; node.zoom = _options.maxZoom + 1; + node.minLeafId = node.id; if (np > 0) { node.values.assign(np, 0.0); const auto& v = _points[i].values; @@ -296,6 +297,7 @@ class ClusterEngineCore { cluster.pointIndex = -1; cluster.isCluster = true; cluster.zoom = z; + cluster.minLeafId = current[i].minLeafId; if (np > 0) { // Seed the accumulator with the origin node's values, then fold. cluster.values = current[i].values; @@ -310,6 +312,7 @@ class ClusterEngineCore { const int32_t np2 = current[nb].count; wx += current[nb].x * np2; wy += current[nb].y * np2; + cluster.minLeafId = std::min(cluster.minLeafId, current[nb].minLeafId); for (size_t k = 0; k < np; k++) { const double v = k < current[nb].values.size() ? current[nb].values[k] @@ -384,6 +387,7 @@ class ClusterEngineCore { node.pointIndex = p.id; node.isCluster = false; node.zoom = 0; + node.minLeafId = p.id; node.values = p.values; result.push_back(std::move(node)); } diff --git a/package/cpp/ClusterEngineCore.test.cpp b/package/cpp/ClusterEngineCore.test.cpp index 745da85..cdf6ebd 100644 --- a/package/cpp/ClusterEngineCore.test.cpp +++ b/package/cpp/ClusterEngineCore.test.cpp @@ -335,6 +335,46 @@ static void testMemorySizeReflectsNativeAllocations() { assert(engine.memorySize() > afterPoints); } +static void testMinLeafIdIsTheSmallestLeafAtEveryZoom() { + ClusterEngineCore engine; + ClusterEngineConfig config; + config.radius = 40.0; + config.minPoints = 2; + config.minZoom = 0; + config.maxZoom = 16; + engine.setOptions(config); + + // Pairs that merge into ever larger clusters as the zoom drops. + const int32_t count = 64; + std::vector ids(count); + std::vector lats(count); + std::vector lngs(count); + for (int32_t i = 0; i < count; i++) { + ids[i] = i; + lats[i] = 52.0 + (i % 8) * 0.002 + (i / 8) * 0.00001; + lngs[i] = 21.0 + (i / 8) * 0.003; + } + engine.setPoints(ids.data(), lats.data(), lngs.data(), static_cast(count)); + engine.build(); + + int32_t clustersChecked = 0; + for (int32_t z = config.minZoom; z <= config.maxZoom; z++) { + for (const auto& node : engine.getClusters({85.0, -85.0, 180.0, -180.0, static_cast(z)})) { + if (!node.isCluster) { + assert(node.minLeafId == node.id); + continue; + } + int32_t smallest = count; + for (const auto& leaf : engine.getLeaves(node.id, 0, 0)) { + smallest = std::min(smallest, leaf.pointIndex); + } + assert(node.minLeafId == smallest); + clustersChecked++; + } + } + assert(clustersChecked > 10); +} + int main() { testGetLeavesReturnsAllPointsInCluster(); testGetLeavesPagination(); @@ -347,6 +387,7 @@ int main() { testGetLeavesUnlimited(); testInvalidBufferRejected(); testMemorySizeReflectsNativeAllocations(); + testMinLeafIdIsTheSmallestLeafAtEveryZoom(); std::printf("ClusterEngineCore tests passed.\n"); return 0; } diff --git a/package/cpp/GeoUtils.hpp b/package/cpp/GeoUtils.hpp index 7f764b9..adc75d1 100644 --- a/package/cpp/GeoUtils.hpp +++ b/package/cpp/GeoUtils.hpp @@ -113,6 +113,10 @@ struct ClusterNode { int32_t pointIndex; // -1 for cluster, >= 0 for leaf point index bool isCluster; int32_t zoom; + // Smallest point id among the node's leaves (its own id for a point). A + // cluster keeps it while it only gains or loses other leaves, so it + // identifies the cluster across zoom levels and rebuilds. + int32_t minLeafId = -1; // Aggregated values, one per configured reducer. For a leaf this mirrors the // point's raw values; for a cluster it is the reduced result over its leaves. std::vector values; diff --git a/package/cpp/HybridClusterEngine.cpp b/package/cpp/HybridClusterEngine.cpp index 8488064..81ef67a 100644 --- a/package/cpp/HybridClusterEngine.cpp +++ b/package/cpp/HybridClusterEngine.cpp @@ -81,6 +81,7 @@ EngineClusterNode HybridClusterEngine::toFeature(const ClusterNode& node) const node.isCluster, static_cast(node.parentId), static_cast(node.pointIndex), + static_cast(node.minLeafId), node.values ); } diff --git a/package/src/__tests__/fakes/fakeClusterEngine.ts b/package/src/__tests__/fakes/fakeClusterEngine.ts index 0f1c760..89efc63 100644 --- a/package/src/__tests__/fakes/fakeClusterEngine.ts +++ b/package/src/__tests__/fakes/fakeClusterEngine.ts @@ -75,6 +75,7 @@ interface PointProps { interface ClusterProps { values: number[] + minLeafId: number } type Feature = Supercluster.PointFeature @@ -134,6 +135,7 @@ function toNode(feature: Result): EngineClusterNode { isCluster: true, parentId: -1, pointIndex: -1, + minLeafId: properties.minLeafId, values: properties.values ?? [], } } @@ -146,6 +148,7 @@ function toNode(feature: Result): EngineClusterNode { isCluster: false, parentId: -1, pointIndex: point.index, + minLeafId: point.index, values: point.values, } } @@ -193,11 +196,12 @@ export function createFakeClusterEngine(): FakeClusterEngine { maxZoom: options.maxZoom, minPoints: options.minPoints, nodeSize: options.nodeSize, - map: (props) => ({ values: [...props.values] }), + map: (props) => ({ values: [...props.values], minLeafId: props.index }), reduce: (acc, props) => { acc.values = acc.values.map((value, k) => reduceValue(reducers[k]!, value, props.values[k] ?? 0) ) + acc.minLeafId = Math.min(acc.minLeafId, props.minLeafId) }, }).load(points) built = true diff --git a/package/src/__tests__/mapview/MapView.stability.test.tsx b/package/src/__tests__/mapview/MapView.stability.test.tsx index 63b277a..8ad7ca6 100644 --- a/package/src/__tests__/mapview/MapView.stability.test.tsx +++ b/package/src/__tests__/mapview/MapView.stability.test.tsx @@ -444,6 +444,64 @@ describe('MapView engine failures (#15)', () => { }) }) +describe('MapView marker keys (#28)', () => { + it('does not remount other markers when one is inserted at the start', async () => { + const { rerender } = render() + await flush() + const start = mountLog.length + + rerender() + await flush() + + expect(unmountsSince(start)).toEqual([]) + // Index keys used to hand the shifted markers' instances to other points. + expect(retargetsSince(start)).toEqual([]) + }) + + const clusterUnmountsSince = (start: number) => + unmountsSince(start).filter((id) => id.startsWith('cluster@')) + + it('keeps a cluster bubble mounted while it splits on zoom', async () => { + // GROUP is one cluster of 5 at camera zoom 17 and loses g4 at 18. + const { container } = render( + + ) + await flush() + expect(clusterLabels(container)).toEqual(['5']) + const start = mountLog.length + + await settleRegion(regionAt(GROUP[2]!, 18)) + + expect(clusterLabels(container)).toEqual(['4']) + expect(clusterUnmountsSince(start)).toEqual([]) + }) + + it('keeps cluster bubbles mounted when a rebuild inserts a marker', async () => { + const { container, rerender } = render() + await flush() + const start = mountLog.length + + rerender() + await flush() + + expect(fakeClusterEngineStats.created).toBe(2) + expect(clusterLabels(container)).toEqual(['5']) + expect(clusterUnmountsSince(start)).toEqual([]) + }) + + it('unmounts only the marker removed from the start', async () => { + const { rerender } = render() + await flush() + const start = mountLog.length + + rerender() + await flush() + + expect(unmountsSince(start)).toEqual(['x0']) + expect(retargetsSince(start)).toEqual([]) + }) +}) + describe('Marker cluster prop (#14)', () => { const clusterable = (points: TestPoint[]) => points.map((point) => ({ ...point, cluster: true })) @@ -518,20 +576,4 @@ describe('MapView known issues', () => { expect(clusterLabels(container)).toEqual([]) expect(renderedMarkerIds(container)).toHaveLength(SAME_SPOT.length) }) - - it.failing( - '#28: inserting a marker at the start does not remount the others', - async () => { - const { rerender } = render() - await flush() - const start = mountLog.length - - rerender() - await flush() - - expect(unmountsSince(start)).toEqual([]) - // Index keys hand the shifted markers' instances to other points. - expect(retargetsSince(start)).toEqual([]) - } - ) }) diff --git a/package/src/compat/ClusterMarker.test.tsx b/package/src/compat/ClusterMarker.test.tsx index facfc87..e9c45aa 100644 --- a/package/src/compat/ClusterMarker.test.tsx +++ b/package/src/compat/ClusterMarker.test.tsx @@ -1,4 +1,6 @@ -import { describe, expect, it, jest, mock } from 'bun:test' +import { beforeEach, describe, expect, it, jest, mock } from 'bun:test' +import { render } from '@testing-library/react' +import React from 'react' import type { ClusterFeature } from '../geojson/types' import type { ClusterMarkerProps } from './ClusterMarker' @@ -20,11 +22,19 @@ mock.module('react-native-reanimated', () => ({ withTiming: (value: unknown) => value, })) +const redraw = jest.fn() + mock.module('react-native-maps', () => ({ - Marker: 'Marker', + Marker: class Marker extends React.Component<{ children?: React.ReactNode }> { + redraw = redraw + render() { + return React.createElement('rn-marker', null, this.props.children) + } + }, })) -const { areClusterMarkerPropsEqual } = await import('./ClusterMarker') +const { default: ClusterMarker, areClusterMarkerPropsEqual } = + await import('./ClusterMarker') const CLUSTER: ClusterFeature = { type: 'Feature', @@ -56,6 +66,79 @@ function baseProps( } } +describe('ClusterMarker redraw', () => { + const withCount = (count: number): ClusterFeature => ({ + ...CLUSTER, + properties: { ...CLUSTER.properties, point_count: count }, + }) + + beforeEach(() => { + redraw.mockClear() + }) + + it('redraws a mounted bubble when its count or colour changes', () => { + const onPress = jest.fn() + const { rerender } = render( + + ) + expect(redraw).not.toHaveBeenCalled() + + rerender( + + ) + expect(redraw).toHaveBeenCalledTimes(1) + + rerender( + + ) + expect(redraw).toHaveBeenCalledTimes(2) + }) + + it('does not redraw when only the position changes', () => { + const onPress = jest.fn() + const { rerender } = render() + + rerender( + + ) + + expect(redraw).not.toHaveBeenCalled() + }) + + it('leaves redrawing to the map while tracksViewChanges is on', () => { + const onPress = jest.fn() + const { rerender } = render( + + ) + + rerender( + + ) + + expect(redraw).not.toHaveBeenCalled() + }) +}) + describe('areClusterMarkerPropsEqual', () => { it('returns true when feature and stable onPress reference are unchanged', () => { const onPress = jest.fn() diff --git a/package/src/compat/ClusterMarker.tsx b/package/src/compat/ClusterMarker.tsx index 8f53c65..af68ea7 100644 --- a/package/src/compat/ClusterMarker.tsx +++ b/package/src/compat/ClusterMarker.tsx @@ -1,11 +1,11 @@ -import React, { memo, useEffect } from 'react' +import React, { memo, useEffect, useRef } from 'react' import { Platform, StyleSheet, Text, View } from 'react-native' import Animated, { useAnimatedProps, useSharedValue, withTiming, } from 'react-native-reanimated' -import { Marker } from 'react-native-maps' +import { Marker, type MapMarker } from 'react-native-maps' import type { ClusterFeature } from '../geojson/types' const BUBBLE_BORDER_WIDTH = 2.5 @@ -13,9 +13,10 @@ const HALO_SCALE = 1.18 const SHADOW_PADDING = 6 /** - * `react-native-maps` markers are native annotations: on zoom the cluster set is - * rebuilt with new `cluster_id`s, so React unmounts/remounts every bubble and the - * native side removes/adds annotations in one commit, producing a visible blink. + * `react-native-maps` markers are native annotations: a bubble that unmounts and + * remounts is removed and re-added in one commit, a visible blink. `MapView` + * keys bubbles by their cluster's smallest marker, so most survive zoom steps; + * those that genuinely appear or disappear fade. * * Animating the bubble `` opacity does not help because `tracksViewChanges` * is `false` (a single bitmap snapshot is taken). The only lever that affects the @@ -127,10 +128,27 @@ function ClusterMarker({ const animatedProps = useAnimatedProps(() => ({ opacity: opacity.value })) + // The bubble keeps its native marker while its cluster splits, merges or + // changes colour, but with `tracksViewChanges` off Google Maps keeps showing + // the bitmap it took first, so ask for a new one when the look changes. + const markerRef = useRef(null) + const appearance = `${points}|${clusterColor}|${clusterTextColor}|${clusterFontFamily ?? ''}` + const drawnAppearance = useRef(appearance) + useEffect(() => { + if (drawnAppearance.current === appearance) { + return + } + drawnAppearance.current = appearance + if (!tracksViewChanges) { + markerRef.current?.redraw() + } + }, [appearance, tracksViewChanges]) + const MarkerComponent = fadeEnabled ? AnimatedMarker : Marker return ( onPress(feature)} tracksViewChanges={tracksViewChanges} diff --git a/package/src/compat/MapView.tsx b/package/src/compat/MapView.tsx index c17dd88..c53dcfb 100644 --- a/package/src/compat/MapView.tsx +++ b/package/src/compat/MapView.tsx @@ -621,15 +621,27 @@ const CompatMapView = forwardRef(function CompatMapView( return typeof index === 'number' ? propsChildren[index] : null } + // A cluster is keyed by its smallest leaf, which it keeps across zoom + // levels and rebuilds while it only gains or loses other leaves, so its + // bubble updates one native marker instead of removing and re-adding it. + // Until a props-only change swaps in the newest features, the leaf is the + // previous object, whose child index is the same by construction. With no + // element for the leaf (mid-rebuild), it falls back to the engine id. + const clusterKeyOf = (cluster: ClusterFeature): string => { + const minLeaf = supercluster.getClusterMinLeaf(cluster) + const element = minLeaf == null ? null : markerFor(minLeaf) + return React.isValidElement(element) && element.key != null + ? `cluster:${element.key}` + : `cluster-${cluster.id}` + } + for (const feature of clusters) { if (!isClusterFeature(feature)) { + // Keeps the key `Children.toArray` gave it, so each marker stays one + // React instance when others are inserted or removed before it. const child = markerFor(feature) if (isMarker(child)) { - immediate.push( - React.cloneElement(child, { - key: `marker-${feature.properties.index}`, - }) - ) + immediate.push(child) } continue } @@ -649,7 +661,7 @@ const CompatMapView = forwardRef(function CompatMapView( continue } - const clusterKey = `cluster-${cluster.id}` + const clusterKey = clusterKeyOf(cluster) const selected = selectedClusterId != null && String(clusterId) === String(selectedClusterId) diff --git a/package/src/engine/Supercluster.test.ts b/package/src/engine/Supercluster.test.ts index 79deb76..6151770 100644 --- a/package/src/engine/Supercluster.test.ts +++ b/package/src/engine/Supercluster.test.ts @@ -9,7 +9,7 @@ import { spyOn, } from 'bun:test' -import type { PointFeature } from '../geojson' +import type { ClusterFeature, PointFeature } from '../geojson' import type { EngineClusterNode } from '../specs/EngineClusterNode' const mockEngine = { @@ -154,6 +154,7 @@ describe('Supercluster pointIndex identity', () => { isCluster: false, parentId: -1, pointIndex: 1, + minLeafId: 1, values: [], }, ]) @@ -228,6 +229,7 @@ describe('Supercluster coordinate validation', () => { isCluster: false, parentId: -1, pointIndex: 0, + minLeafId: 0, values: [], }, { @@ -238,6 +240,7 @@ describe('Supercluster coordinate validation', () => { isCluster: false, parentId: -1, pointIndex: 1, + minLeafId: 1, values: [], }, ]) @@ -262,6 +265,7 @@ describe('Supercluster coordinate validation', () => { isCluster: false, parentId: -1, pointIndex: 0, + minLeafId: 0, values: [], }, ]) @@ -278,6 +282,75 @@ describe('Supercluster coordinate validation', () => { }) }) +describe('Supercluster.getClusterMinLeaf', () => { + beforeEach(() => { + jest.clearAllMocks() + mockEngine.isBuilt = false + }) + + const point = (id: string, longitude: number): PointFeature => ({ + type: 'Feature', + geometry: { type: 'Point', coordinates: [longitude, 52] }, + properties: { id }, + }) + const clusterNode = (minLeafId: number): EngineClusterNode => ({ + id: 10, + latitude: 52, + longitude: 21, + count: 2, + isCluster: true, + parentId: -1, + pointIndex: -1, + minLeafId, + values: [], + }) + + it('resolves the min leaf index the engine reports to the loaded feature', () => { + const warn = spyOn(console, 'warn').mockImplementation(() => {}) + mockEngine.getClusters.mockReturnValueOnce([clusterNode(0)]) + const unindexable = point('broken', Number.NaN) + const first = point('first', 21) + const clusterer = new Supercluster().load([ + unindexable, + first, + point('second', 21.0001), + ]) + + const [cluster] = clusterer.getClusters([...WORLD_BBOX], 10) + + // Index 0 is the first indexable point, after the skipped one. + expect(clusterer.getClusterMinLeaf(cluster as ClusterFeature)).toBe(first) + warn.mockRestore() + }) + + it('returns the newer feature after replaceLoadedFeatures', () => { + mockEngine.getClusters.mockReturnValueOnce([clusterNode(1)]) + const clusterer = new Supercluster().load([ + point('a', 21), + point('b', 21.0001), + ]) + const [cluster] = clusterer.getClusters([...WORLD_BBOX], 10) + const newerB = point('b', 21.0001) + + clusterer.replaceLoadedFeatures([point('a', 21), newerB]) + + expect(clusterer.getClusterMinLeaf(cluster as ClusterFeature)).toBe(newerB) + }) + + it('returns undefined for a cluster another instance returned', () => { + mockEngine.getClusters.mockReturnValueOnce([clusterNode(0)]) + const [cluster] = new Supercluster() + .load([point('a', 21)]) + .getClusters([...WORLD_BBOX], 10) + + expect( + new Supercluster() + .load([point('a', 21)]) + .getClusterMinLeaf(cluster as ClusterFeature) + ).toBeUndefined() + }) +}) + describe('Supercluster.getAllLeaves', () => { beforeEach(() => { jest.clearAllMocks() diff --git a/package/src/engine/Supercluster.ts b/package/src/engine/Supercluster.ts index 2095bed..50887c3 100644 --- a/package/src/engine/Supercluster.ts +++ b/package/src/engine/Supercluster.ts @@ -44,6 +44,7 @@ function abbreviateCount(count: number): string { export class Supercluster

{ private engine: ClusterEngine | null = null private loadedFeatures: PointFeature

[] = [] + private readonly minLeafIndices = new WeakMap, number>() private destroyed = false private readonly options: Required @@ -254,6 +255,19 @@ export class Supercluster

{ return this.engine!.getClusterExpansionZoom(clusterId) } + /** + * The leaf of `cluster` with the smallest point index. A cluster keeps it + * across zoom levels, and across rebuilds that keep that leaf first, while + * it only gains or loses other leaves. + * + * @internal The compat `MapView` keys cluster markers by it. Returns + * `undefined` for a cluster this instance did not return. + */ + getClusterMinLeaf(cluster: ClusterFeature

): PointFeature

| undefined { + const index = this.minLeafIndices.get(cluster) + return index == null ? undefined : this.loadedFeatures[index] + } + /** * {@linkcode MapRegion} that fits all leaves in a cluster. * @@ -357,7 +371,7 @@ export class Supercluster

{ ) : {} - return { + const cluster: ClusterFeature

= { type: 'Feature', id: clusterId, geometry: { @@ -373,5 +387,7 @@ export class Supercluster

{ ...aggregated, } as ClusterFeature

['properties'], } + this.minLeafIndices.set(cluster, feature.minLeafId) + return cluster } } diff --git a/package/src/specs/EngineClusterNode.ts b/package/src/specs/EngineClusterNode.ts index c2d10b1..bb74716 100644 --- a/package/src/specs/EngineClusterNode.ts +++ b/package/src/specs/EngineClusterNode.ts @@ -18,6 +18,12 @@ export interface EngineClusterNode { parentId: number /** Index into the loaded point array for leaf nodes. */ pointIndex: number + /** + * Smallest point id among this node's leaves; its own {@linkcode id} for a + * point. A cluster keeps it across zoom levels and rebuilds while it only + * gains or loses other leaves, so it can key the cluster's marker. + */ + minLeafId: number /** Aggregated numeric values, one per configured {@linkcode ReducerKind}. */ values: number[] }