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

More dynamic UX for cluster click #423

Closed
wbazant opened this issue Jul 13, 2024 · 6 comments · Fixed by #478
Closed

More dynamic UX for cluster click #423

wbazant opened this issue Jul 13, 2024 · 6 comments · Fixed by #478
Labels
enhancement New feature request

Comments

@wbazant
Copy link
Collaborator

wbazant commented Jul 13, 2024

Zooming by 1 after a click results in slow, frustrating UX.

After playing around, zooming in by 3 (so into 1/8 of the original screen) seems to be just on the edge, sometimes it's slightly too far into the cluster (so we don't see all the dots corresponding to the cluster, just most of them) but mostly results in e.g. zooming into the right country or city.

Also, if there's just one location somewhere remote, we might as well zoom into it directly.

The formula for delta zoom given cluster count, how many other clusters there are near it, etc. could probably get very complicated if we wanted it to, but for now I'm proposing this:

-    mapRef.current?.setZoom(view.zoom + 1)
+    mapRef.current?.setZoom(cluster.count === 1 ? VISIBLE_CLUSTER_ZOOM_LIMIT + 1 : Math.min(VISIBLE_CLUSTER_ZOOM_LIMIT + 1, view.zoom + 3))
@wbazant
Copy link
Collaborator Author

wbazant commented Jul 17, 2024

I just noticed, I probably also need to change this in fetchClusters:

src/redux/mapSlice.js
export const fetchMapClusters = createAsyncThunk(
  'map/fetchMapClusters',
  async (_, { getState }) => {
    const state = getState()
    return await getClusters(               
      selectParams(state, { 
        zoom: state.map.view?.zoom ? state.map.view.zoom + 1 : null,
      }),                                             
    )                                
  },                      
)

@ezwelty ezwelty added the enhancement New feature request label Aug 16, 2024
@ezwelty
Copy link
Collaborator

ezwelty commented Aug 16, 2024

@wbazant Your suggested setZoom is a good start, but it would be possible to do better by using setBounds rather than setZoom. The clusters are computed on a nested quadtree which, for a given zoom level, divides the Earth into a 2^zoom x 2^zoom grid of equal-sized cells in the Web Mercator projection (EPSG:3857). Since we currently use zoom+1 clusters for a given map zoom, this means each Google Map tile (which use the same quadtree) is split into 4 cluster cells.

What this means is that the latitude, longitude of the cluster (what is returned by the API) is sufficient to determine the map bounds of the corresponding grid cell. The logic is worked out in https://github.com/falling-fruit/falling-fruit-api/blob/main/helpers.js, see wgs84_to_mercator, mercator_to_gridcell, and the commented out gridcell_to_mercator and mercator_to_wgs84 – I could write up a function for this if desired. But this would be the maximum bounds. Alternatively, I could look into adjusting the API to return the actual bounds of the locations within the cluster.

@wbazant
Copy link
Collaborator Author

wbazant commented Aug 16, 2024

That would be better, since the zoom heuristic also depends on the viewport - I was thinking zooming in by a factor of 3 will work when the cluster grid is at least 8x8 on the screen, as you point out that needs two tiles.

I had a go at having your suggestion implemented by an AI assistant this morning, would you check 42f7793 ?

The math seems legit - at least as a series of transformations, I didn't try to understand them, but I gave Claude your file and the requirements, and it came up with:

 _.cluster_to_bounds = function({lat, lng, zoom}) {                                                                                                                                           
   const mercator = _.wgs84_to_mercator({x: lng, y: lat})                                                                                                                                     
   const cell = _.mercator_to_gridcell(mercator, zoom)                                                                                                                                        
   const sw = _.gridcell_to_mercator(cell)                                                                                                                                                    
   const ne = _.gridcell_to_mercator({x: cell.x + 1, y: cell.y + 1, zoom: cell.zoom})                                                                                                         
   return {                                                                                                                                                                                   
     sw: _.mercator_to_wgs84(sw),                                                                                                                                                             
     ne: _.mercator_to_wgs84(ne)                                                                                                                                                              
   }                                                                                                                                                                                          
 } 

and then we unpacked the implementations of your functions. I also tested it a bit and it seems to zoom in quite sensibly.

Returning the cluster bounds from the API would tie up with our proposed resolution for #378 and be much better for sparsely located locations!

@ezwelty
Copy link
Collaborator

ezwelty commented Aug 16, 2024

Neat, the AI did a good job refactoring my code! I simplified and added a comment, but the math was right. (a8da6cb)

@wbazant
Copy link
Collaborator Author

wbazant commented Sep 25, 2024

I think the live site works as proposed now, and when a cluster ends up splitting into several clusters it's very nice and dynamic - maybe the during the zoom-in the clusters could be made opaque that shows they're being discarded, instead of being just replaced.

If I'm going after a small cluster I end up having to 'chase' it around the map. I'm not sure what to do about it but I'll keep the issue open for a bit more

@ezwelty
Copy link
Collaborator

ezwelty commented Oct 3, 2024

@wbazant Until I look into adding bounding boxes to the API's GET /clusters (if performance allows, since this would have to be computed on the fly for each type filter), does it make sense to bring back the special deep zoom for count-1 clusters?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants