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.

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

Marker animation mostly fixed #1257

Merged
merged 5 commits into from
Nov 7, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
47 changes: 11 additions & 36 deletions packages/libs/components/src/map/SemanticMarkers.tsx
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain how removing enqueueZoom fixes the issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

To be honest, not entirely sure. I suspect React 18-related timing changes messed something up. There was nothing else to "blame" as far as I could see in the animation-related source files.

Although there's a remaining niggle with the z-index ordering of old vs new markers during animation, I am happy to remove code.

The enqueued setTimeout isn't necessary because on the next render (which is now debounced to the same milliseconds as animation (300ms)) the animation function will basically be a no-op because the geohash level is the same for prevRecenteredMarkers and recenteredMarkers - so it returns recenteredMarkers which will replace the previous concatenated old+new markers.

Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ export default function SemanticMarkers({
// 2023-11: it does seem to be needed for zoom-in animation to work.
const debouncedUpdateMarkerPositions = debounce(
updateMarkerPositions,
1000
animation ? animation.duration : 0
);
// call it at least once at the beginning of the life cycle
debouncedUpdateMarkerPositions();
Expand Down Expand Up @@ -126,27 +126,23 @@ export default function SemanticMarkers({
) {
// get the position-modified markers from `animationFunction`
// see geohash.tsx for example
const animationValues = animation.animationFunction({
prevMarkers: prevRecenteredMarkers,
markers: recenteredMarkers,
});
const { markers: oldAndNewRepositionedMarkers } =
animation.animationFunction({
prevMarkers: prevRecenteredMarkers,
markers: recenteredMarkers,
});
// set them as current
// any marker that already existed will move to the modified position
if (
!isEqual(
animationValues.markers.map(({ props }) => props),
oldAndNewRepositionedMarkers.map(({ props }) => props),
consolidatedMarkers.map(({ props }) => props)
)
)
setConsolidatedMarkers(animationValues.markers);
// then set a timer to remove the old markers when zooming out
// or if zooming in, switch to just the new markers straight away
// (their starting position was set by `animationFunction`)
// It's complicated but it works!
timeoutVariable = enqueueZoom(
animationValues.zoomType,
recenteredMarkers
);
setConsolidatedMarkers(oldAndNewRepositionedMarkers);

// we used to set a timer to remove the old markers when zooming out
// but now we just let the next render cycle do it.
} else {
/** First render of markers **/
if (
Expand All @@ -171,27 +167,6 @@ export default function SemanticMarkers({
)
setPrevRecenteredMarkers(recenteredMarkers);
}

function enqueueZoom(
zoomType: string | null,
nextMarkers: ReactElement<BoundsDriftMarkerProps>[]
) {
/** If we are zooming in then reset the marker elements. When initially rendered
* the new markers will start at the matching existing marker's location and here we will
* reset marker elements so they will animated to their final position
**/
if (zoomType === 'in') {
setConsolidatedMarkers(nextMarkers);
} else if (zoomType === 'out') {
/** If we are zooming out then remove the old markers after they finish animating. **/
return window.setTimeout(
() => {
setConsolidatedMarkers(nextMarkers);
},
animation ? animation.duration : 0
);
}
}
}, [
animation,
map,
Expand Down
15 changes: 7 additions & 8 deletions packages/libs/components/src/map/animation_functions/geohash.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ export default function geohashAnimation({
prevMarkers,
markers,
}: geoHashAnimation) {
const prevGeoHash = prevMarkers[0].key as string;
const currentGeohash = markers[0].key as string;
const prevGeoHash = prevMarkers[0].props.id as string;
const currentGeohash = markers[0].props.id as string;
let zoomType, consolidatedMarkers;

/** Zoom Out - Move existing markers to new position
Expand All @@ -22,9 +22,9 @@ export default function geohashAnimation({
if (prevGeoHash.length > currentGeohash.length) {
zoomType = 'out';
const hashDif = prevGeoHash.length - currentGeohash.length;
// Get a new array of existing markers with new position property
// Get array of old markers with new positions
const cloneArray = updateMarkers(prevMarkers, markers, hashDif);
// Combine the new and existing markers
// Combine the new and old markers
consolidatedMarkers = [...markers, ...cloneArray];
} else if (prevGeoHash.length < currentGeohash.length) {
/** Zoom In - New markers start at old position
Expand All @@ -33,10 +33,9 @@ export default function geohashAnimation({
**/
zoomType = 'in';
const hashDif = currentGeohash.length - prevGeoHash.length;
// Get a new array of new markers with existing position property
// Set final render markers to the cloneArray which holds the new markers with
// their new starting location
consolidatedMarkers = updateMarkers(markers, prevMarkers, hashDif);
// Get array of new markers with old positions
const cloneArray = updateMarkers(markers, prevMarkers, hashDif);
consolidatedMarkers = [...prevMarkers, ...cloneArray];
Copy link
Member Author

Choose a reason for hiding this comment

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

Reversing the order of the spreads here does not fix the "old marker hidden under new markers" problem.

} else {
/** No difference in geohashes - Render markers as they are **/
zoomType = null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,12 @@ export default function updateMarkers(
) {
return toChangeMarkers.map((markerObj) => {
// Calculate the matching geohash
const sourceKey = markerObj.key as string;
const sourceKey = markerObj.props.id as string;
const sourceHash = sourceKey.slice(0, -hashDif);

// Find the object with the matching geohash
const matchingMarkers = sourceMarkers.filter((obj) => {
return obj.key === sourceHash;
return obj.props.id === sourceHash;
});

// Clone marker element with new position
Expand All @@ -22,6 +22,9 @@ export default function updateMarkers(
// Clone marker element with new position
markerCloneProps = {
position: matchingMarkers[0].props.position,
// ideally we would put the modified markers on top
// but this doesn't seem to work:
// zIndexOffset: -1000, // or +1000
};
}

Expand Down
Loading