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

Finalize MapView performance optimizations #802

Merged
merged 11 commits into from
Oct 19, 2023
Merged

Finalize MapView performance optimizations #802

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
bd20b3c
Revert "Add prop to improve performance of MapView (#800)"
YoussefHenna Oct 17, 2023
653cd0f
Memoize cluster components
YoussefHenna Oct 17, 2023
9256050
Revert "Memoize cluster components"
YoussefHenna Oct 17, 2023
975b7ea
Memoize default cluster view component
YoussefHenna Oct 17, 2023
628cee1
Revert "Memoize default cluster view component"
YoussefHenna Oct 17, 2023
ea910e2
Optimize auto clustering
YoussefHenna Oct 17, 2023
acedce8
More optimizations to auto clustering
YoussefHenna Oct 17, 2023
768a673
Allow a way to use default `icon` prop
YoussefHenna Oct 18, 2023
2f56a34
Fix condition
YoussefHenna Oct 18, 2023
1d15935
Disable tracksViewChanges on default cluster
YoussefHenna Oct 19, 2023
1f62cb6
Fix typo
YoussefHenna Oct 19, 2023
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
10 changes: 0 additions & 10 deletions packages/maps/src/__tests__/MapView.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,6 @@ describe("MapView tests", () => {
}
}
onPress={[Function]}
tracksViewChanges={true}
/>,
<Marker
coordinate={
Expand All @@ -196,7 +195,6 @@ describe("MapView tests", () => {
}
}
onPress={[Function]}
tracksViewChanges={true}
/>,
],
[
Expand All @@ -208,7 +206,6 @@ describe("MapView tests", () => {
}
}
onPress={[Function]}
tracksViewChanges={true}
/>,
<Marker
coordinate={
Expand All @@ -218,7 +215,6 @@ describe("MapView tests", () => {
}
}
onPress={[Function]}
tracksViewChanges={true}
/>,
],
]
Expand Down Expand Up @@ -251,7 +247,6 @@ describe("MapView tests", () => {
}
}
onPress={[Function]}
tracksViewChanges={true}
/>,
<Marker
coordinate={
Expand All @@ -261,7 +256,6 @@ describe("MapView tests", () => {
}
}
onPress={[Function]}
tracksViewChanges={true}
/>,
],
[
Expand All @@ -273,7 +267,6 @@ describe("MapView tests", () => {
}
}
onPress={[Function]}
tracksViewChanges={true}
/>,
<Marker
coordinate={
Expand All @@ -283,7 +276,6 @@ describe("MapView tests", () => {
}
}
onPress={[Function]}
tracksViewChanges={true}
/>,
],
]
Expand Down Expand Up @@ -340,7 +332,6 @@ describe("MapView tests", () => {
}
}
onPress={[Function]}
tracksViewChanges={true}
/>,
<Marker
coordinate={
Expand All @@ -350,7 +341,6 @@ describe("MapView tests", () => {
}
}
onPress={[Function]}
tracksViewChanges={true}
/>,
],
]
Expand Down
15 changes: 14 additions & 1 deletion packages/maps/src/components/MapMarker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
View,
StyleSheet,
Text,
Platform,
} from "react-native";
import { Marker as MapMarkerComponent } from "./react-native-maps";
import type {
Expand All @@ -20,6 +21,7 @@ export interface MapMarkerProps
longitude: number;
pinImage?: string | ImageSourcePropType;
pinImageSize?: number;
androidUseDefaultIconImplementation?: boolean;
onPress?: (latitude: number, longitude: number) => void;
}

Expand All @@ -40,6 +42,7 @@ export function renderMarker(
longitude,
pinImage,
pinImageSize = 50,
androidUseDefaultIconImplementation = false,
onPress,
children,
title,
Expand Down Expand Up @@ -76,6 +79,9 @@ export function renderMarker(
);
}

const shouldUseDefaultIconImplementation =
Platform.OS === "android" && androidUseDefaultIconImplementation;

return (
<MapMarkerComponent
ref={ref}
Expand All @@ -89,11 +95,18 @@ export function renderMarker(
const coordinate = event.nativeEvent.coordinate;
onPress?.(coordinate.latitude, coordinate.longitude);
}}
icon={
shouldUseDefaultIconImplementation
? typeof pinImage === "string"
? { uri: pinImage }
: (pinImage as any)
: undefined
}
{...rest}
>
{nonCalloutChildren}

{pinImage && (
{pinImage && !shouldUseDefaultIconImplementation && (
<Image
testID="map-marker-pin-image"
source={typeof pinImage === "string" ? { uri: pinImage } : pinImage}
Expand Down
95 changes: 46 additions & 49 deletions packages/maps/src/components/MapView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,6 @@ export interface MapViewProps<T>
longitude?: number;
autoClusterMarkers?: boolean;
autoClusterMarkersDistanceMeters?: number;
// Improves performance when panning by temporarily preventing markers from tracking view changes
// See `tracksViewChanges`: https://github.com/react-native-maps/react-native-maps/blob/master/docs/marker.md#props
disableTrackViewChangesWhenPanning?: boolean;
markersData?: T[];
keyExtractor?: (item: T, index: number) => string;
renderItem?: ({ item, index }: { item: T; index: number }) => JSX.Element;
Expand All @@ -62,7 +59,6 @@ const MapViewF = <T extends object>({
loadingEnabled = true,
autoClusterMarkers = false,
autoClusterMarkersDistanceMeters = 1000,
disableTrackViewChangesWhenPanning = true,
markersData,
keyExtractor,
renderItem,
Expand All @@ -77,8 +73,6 @@ const MapViewF = <T extends object>({
animateToLocation: (location: ZoomLocation) => void;
mapRef: React.RefObject<MapViewComponent>;
}) => {
const [markerTracksViewChanges, setMarkerTracksViewChanges] =
React.useState(true);
const [currentRegion, setCurrentRegion] = React.useState<Region | null>(null);
const delayedRegionValue = useDebounce(currentRegion, 300);

Expand Down Expand Up @@ -190,14 +184,20 @@ const MapViewF = <T extends object>({
const clusterMarkers = React.useCallback(
(
markers: React.ReactElement[],
clusters: React.ReactElement[],
distanceMeters: number,
clusterView?: React.ReactElement
) => {
const clusters = [];
const clusteredMarkers: React.ReactElement[] = [];

for (const marker of markers) {
const { latitude: lat, longitude: long } =
marker.props as MapMarkerProps;

if (clusteredMarkers.includes(marker)) {
continue;
}

const nearbyMarkers = getNearbyMarkers(
lat,
long,
Expand All @@ -207,7 +207,7 @@ const MapViewF = <T extends object>({

if (nearbyMarkers.length > 1) {
for (const nearbyMarker of nearbyMarkers) {
markers.splice(markers.indexOf(nearbyMarker), 1);
clusteredMarkers.push(nearbyMarker);
}
clusters.push(
<MapMarkerCluster>
Expand All @@ -217,6 +217,12 @@ const MapViewF = <T extends object>({
);
}
}

const unClusteredMarkers = markers.filter(
(marker) => !clusteredMarkers.includes(marker)
);

return { clusters, unClusteredMarkers };
},
[getNearbyMarkers]
);
Expand All @@ -241,36 +247,50 @@ const MapViewF = <T extends object>({
};

callOnRegionChange();

// onRegionChange excluded to prevent calling on every rerender when using an anonymous function (which is most common)
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [delayedRegionValue]);

const markers = React.useMemo(
() => getChildrenForType(MapMarker),
[getChildrenForType]
);
const circles = React.useMemo(
() => getChildrenForType(MapCircle),
[getChildrenForType]
);
const clusters = React.useMemo(

const markers = React.useMemo(
() => getChildrenForType(MapMarker),
[getChildrenForType]
);

const manualClusters = React.useMemo(
() => getChildrenForType(MapMarkerCluster),
[getChildrenForType]
);

const clusterView = React.useMemo(() => {
const clusterViews = getChildrenForType(MapMarkerClusterView);
return clusterViews.length ? clusterViews[0] : undefined; //Only take the first, ignore any others
}, [getChildrenForType]);

if (autoClusterMarkers) {
clusterMarkers(
markers,
clusters,
autoClusterMarkersDistanceMeters,
clusterView
);
}
const { clusters, unClusteredMarkers } = React.useMemo(() => {
if (autoClusterMarkers) {
const { clusters, unClusteredMarkers } = clusterMarkers(
markers,
autoClusterMarkersDistanceMeters,
clusterView
);

return { clusters: clusters.concat(manualClusters), unClusteredMarkers };
} else {
return { clusters: manualClusters, unClusteredMarkers: markers };
}
}, [
autoClusterMarkers,
autoClusterMarkersDistanceMeters,
markers,
manualClusters,
clusterView,
clusterMarkers,
]);

const memoizedMapView = useDeepCompareMemo(
() => (
Expand All @@ -290,31 +310,16 @@ const MapViewF = <T extends object>({
initialCamera={camera}
loadingEnabled={loadingEnabled}
onRegionChange={setCurrentRegion}
onTouchStart={() => {
if (disableTrackViewChangesWhenPanning) {
setMarkerTracksViewChanges(false);
}
}}
onTouchEnd={() => {
if (disableTrackViewChangesWhenPanning) {
setMarkerTracksViewChanges(true);
}
}}
onPress={(event) => {
const coordinate = event.nativeEvent.coordinate;
onPress?.(coordinate.latitude, coordinate.longitude);
}}
style={[styles.map, style]}
{...rest}
>
{markers.map((marker, index) =>
{unClusteredMarkers.map((marker, index) =>
renderMarker(
{
...marker.props,
tracksViewChanges: disableTrackViewChangesWhenPanning
? markerTracksViewChanges
: undefined,
},
marker.props,
index,
getMarkerRef(getMarkerIdentifier(marker.props)),
() => dismissAllOtherCallouts(getMarkerIdentifier(marker.props))
Expand All @@ -333,13 +338,7 @@ const MapViewF = <T extends object>({
}}
>
{clusters.map((cluster, index) => (
<React.Fragment key={index}>
{React.cloneElement(cluster, {
tracksViewChanges: disableTrackViewChangesWhenPanning
? markerTracksViewChanges
: undefined,
})}
</React.Fragment>
<React.Fragment key={index}>{cluster}</React.Fragment>
))}
</MapMarkerContext.Provider>

Expand All @@ -357,7 +356,7 @@ const MapViewF = <T extends object>({
loadingEnabled,
longitude,
mapRef,
markers,
unClusteredMarkers,
onPress,
onRegionChange,
provider,
Expand All @@ -366,8 +365,6 @@ const MapViewF = <T extends object>({
showsCompass,
style,
zoom,
markerTracksViewChanges,
disableTrackViewChangesWhenPanning,
]
);

Expand Down
22 changes: 9 additions & 13 deletions packages/maps/src/components/marker-cluster/MapMarkerCluster.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,19 +9,15 @@ import { MapViewContext } from "../MapViewCommon";
import { flattenReactFragments } from "@draftbit/ui";
import { MapMarkerContext } from "../MapView";

interface MapMarkerClusterProps {
tracksViewChanges?: boolean;
}

/**
* Component that clusters all markers provided in as children to a single point when zoomed out, and shows the markers themselves when zoomed in
* Renders a default component that shows the number of components inside cluster
*
* Also accepts MapMarkerClusterView to override the rendered cluster component
*/
const MapMarkerCluster: React.FC<
React.PropsWithChildren<MapMarkerClusterProps>
> = ({ children: childrenProp, tracksViewChanges }) => {
const MapMarkerCluster: React.FC<React.PropsWithChildren> = ({
children: childrenProp,
}) => {
const { region, animateToLocation } = React.useContext(MapViewContext);

const children = React.useMemo(
Expand Down Expand Up @@ -79,18 +75,18 @@ const MapMarkerCluster: React.FC<
longitude,
children: clusterView,
onPress,
tracksViewChanges,
tracksViewChanges:
clusterView.type === DefaultMapMarkerClusterView
? false
: clusterView.props.tracksViewChanges,
Comment on lines +78 to +81
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
tracksViewChanges:
clusterView.type === DefaultMapMarkerClusterView
? false
: clusterView.props.tracksViewChanges,
tracksViewChanges:
clusterView.type !== DefaultMapMarkerClusterView || clusterView.props.tracksViewChanges,

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This would result in it always being true if not the DefaultMapMarkerClusterView, we want to instead use the clusterView.props.tracksViewChanges. Or am I misunderstanding?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh good point, it should have been clusterView.type !== DefaultMapMarkerClusterView && clusterView.props.tracksViewChanges, tricky conditional.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is tricky 😄. I kept it as is because it's easier to understand what's going on.

})}
</MapMarkerClusterContext.Provider>
);
}}
>
{markers.map((marker, index) =>
renderMarker(
{ ...marker.props, tracksViewChanges },
index,
getMarkerRef(marker.props),
() => onMarkerPress(marker.props)
renderMarker(marker.props, index, getMarkerRef(marker.props), () =>
onMarkerPress(marker.props)
)
)}
</MarkerClusterer>
Expand Down
1 change: 1 addition & 0 deletions packages/maps/src/components/marker-cluster/MapMarkerClusterView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ interface MapMarkerClusterViewProps {
zoomOnPress?: boolean;
onPress?: (latitude: number, longitude: number) => void;
renderItem?: ({ markerCount }: { markerCount: number }) => JSX.Element;
tracksViewChanges?: boolean;
style?: StyleProp<ViewStyle>;
}

Expand Down
Loading