-
-
Notifications
You must be signed in to change notification settings - Fork 2k
fix: Dynamically compute default zoom/center for map traces #7884
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
base: v4.0
Are you sure you want to change the base?
Changes from all commits
ceecc2a
8e33f3f
a888c2f
fd7650b
be253fa
6999db8
c124086
906b1db
2737396
7e48373
1b3b284
e260f36
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| - Fix `scattermap`, `densitymap` traces not showing all points by dynamically computing `center`, `zoom` values [[#7884](https://github.com/plotly/plotly.js/pull/7884)], with thanks to @palmerusaf and @DhruvGarg111 for the contributions! |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| import { getFitboundsLonRange } from '../../lib/geo_location_utils'; | ||
| import type { MapLayout, ScattermapData } from '../../types/generated/schema'; | ||
|
|
||
| // Same shape as the user-facing `map.bounds` attribute, but with all fields required | ||
| type LonLatBox = Required<NonNullable<MapLayout['bounds']>>; | ||
|
|
||
| // Minimal shape of the fullData entries this helper reads | ||
| interface FitBoundsTrace extends Pick<ScattermapData, 'subplot' | 'visible'> { | ||
| // Tighten lat/lon to be more specific than default | ||
| lat?: ArrayLike<number>; | ||
| lon?: ArrayLike<number>; | ||
| // Broaden type since this could run against multiple trace types | ||
| type?: string; | ||
| } | ||
|
|
||
| /** | ||
| * Compute a lon/lat bounding box from lonlat-bearing traces (`scattermap`, | ||
| * `densitymap`) on the given map subplot. | ||
| * | ||
| * Returns null when: | ||
| * - no fittable data exists on the subplot; | ||
| * - a location-based trace (`choroplethmap`) is present — those carry | ||
| * `locations`/`geojson`, not raw lon/lat, and need geojson bbox handling | ||
| * that isn't implemented here. | ||
| * | ||
| * @param fullData - The full data array (post supply-defaults) | ||
| * @param subplotId - e.g. `'map'`, `'map2'` | ||
| */ | ||
| export function getMapFitBounds(fullData: FitBoundsTrace[], subplotId: string): LonLatBox | null { | ||
| const validLons: number[] = []; | ||
| let minLon = Infinity; | ||
| let maxLon = -Infinity; | ||
| let minLat = Infinity; | ||
| let maxLat = -Infinity; | ||
|
|
||
| for (const trace of fullData) { | ||
| if (trace.subplot !== subplotId || trace.visible !== true) continue; | ||
|
|
||
| // choroplethmap traces carry locations/geojson, not raw lon/lat; bail | ||
| // out rather than frame around a subset of the subplot's data. | ||
| if (trace.type === 'choroplethmap') return null; | ||
|
|
||
| const { lat, lon } = trace; | ||
| if (!lon || !lat) continue; | ||
|
|
||
| const len = Math.min(lon.length, lat.length); | ||
| for (let j = 0; j < len; j++) { | ||
| const lo = lon[j]; | ||
| const la = lat[j]; | ||
| if (Number.isFinite(lo) && Number.isFinite(la)) { | ||
| validLons.push(lo); | ||
| // Track longitude bounds | ||
| if (lo < minLon) minLon = lo; | ||
| if (lo > maxLon) maxLon = lo; | ||
| // Track latitude bounds | ||
| if (la < minLat) minLat = la; | ||
| if (la > maxLat) maxLat = la; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (!validLons.length) return null; | ||
|
|
||
| let west = minLon; | ||
| let east = maxLon; | ||
| // Only handle antimeridian if it actually gets crossed | ||
| if (maxLon - minLon > 180) { | ||
| const lonRange = getFitboundsLonRange(validLons); | ||
| if (lonRange) { | ||
| west = lonRange[0]; | ||
| east = lonRange[1]; | ||
| } | ||
| } | ||
|
|
||
| return { west, east, south: minLat, north: maxLat }; | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -5,18 +5,19 @@ var Lib = require('../../lib'); | |||||
| var handleSubplotDefaults = require('../subplot_defaults'); | ||||||
| var handleArrayContainerDefaults = require('../array_container_defaults'); | ||||||
| var layoutAttributes = require('./layout_attributes'); | ||||||
|
|
||||||
| const { getMapFitBounds } = require('./get_map_fit_bounds'); | ||||||
|
|
||||||
| module.exports = function supplyLayoutDefaults(layoutIn, layoutOut, fullData) { | ||||||
| handleSubplotDefaults(layoutIn, layoutOut, fullData, { | ||||||
| type: 'map', | ||||||
| attributes: layoutAttributes, | ||||||
| handleDefaults: handleDefaults, | ||||||
| partition: 'y' | ||||||
| handleDefaults, | ||||||
| partition: 'y', | ||||||
| fullData | ||||||
| }); | ||||||
| }; | ||||||
|
|
||||||
| function handleDefaults(containerIn, containerOut, coerce) { | ||||||
| function handleDefaults(containerIn, containerOut, coerce, opts) { | ||||||
| coerce('style'); | ||||||
| coerce('center.lon'); | ||||||
| coerce('center.lat'); | ||||||
|
|
@@ -28,12 +29,7 @@ function handleDefaults(containerIn, containerOut, coerce) { | |||||
| var east = coerce('bounds.east'); | ||||||
| var south = coerce('bounds.south'); | ||||||
| var north = coerce('bounds.north'); | ||||||
| if( | ||||||
| west === undefined || | ||||||
| east === undefined || | ||||||
| south === undefined || | ||||||
| north === undefined | ||||||
| ) { | ||||||
| if (west === undefined || east === undefined || south === undefined || north === undefined) { | ||||||
| delete containerOut.bounds; | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -42,6 +38,12 @@ function handleDefaults(containerIn, containerOut, coerce) { | |||||
| handleItemDefaults: handleLayerDefaults | ||||||
| }); | ||||||
|
|
||||||
| // Auto-frame the initial view to the data | ||||||
| if (containerIn.center === undefined && containerIn.zoom === undefined) { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Also, I'm not positive, but I think we should test Edit: Never mind, I misunderstood the
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we test
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh yeah of course we can't use But that does mean auto-bounds won't be applied if the user supplies junk values for (That seems like the wrong behavior to me, but I could be convinced otherwise.)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm deferring to the user in this case. If they explicitly set values that would otherwise be set by auto-fitting, I opted to skip auto-fitting. If you want to provide garbage values, that's up to you. We could update that behavior in the future, but I think we should trust the user for now. |
||||||
| const fitBounds = getMapFitBounds(opts.fullData, opts.id); | ||||||
| if (fitBounds) containerOut._fitBounds = fitBounds; | ||||||
| } | ||||||
|
|
||||||
| // copy ref to input container to update 'center' and 'zoom' on map move | ||||||
| containerOut._input = containerIn; | ||||||
| } | ||||||
|
|
@@ -52,27 +54,27 @@ function handleLayerDefaults(layerIn, layerOut) { | |||||
| } | ||||||
|
|
||||||
| var visible = coerce('visible'); | ||||||
| if(visible) { | ||||||
| if (visible) { | ||||||
| var sourceType = coerce('sourcetype'); | ||||||
| var mustBeRasterLayer = sourceType === 'raster' || sourceType === 'image'; | ||||||
|
|
||||||
| coerce('source'); | ||||||
| coerce('sourceattribution'); | ||||||
|
|
||||||
| if(sourceType === 'vector') { | ||||||
| if (sourceType === 'vector') { | ||||||
| coerce('sourcelayer'); | ||||||
| } | ||||||
|
|
||||||
| if(sourceType === 'image') { | ||||||
| if (sourceType === 'image') { | ||||||
| coerce('coordinates'); | ||||||
| } | ||||||
|
|
||||||
| var typeDflt; | ||||||
| if(mustBeRasterLayer) typeDflt = 'raster'; | ||||||
| if (mustBeRasterLayer) typeDflt = 'raster'; | ||||||
|
|
||||||
| var type = coerce('type', typeDflt); | ||||||
|
|
||||||
| if(mustBeRasterLayer && type !== 'raster') { | ||||||
| if (mustBeRasterLayer && type !== 'raster') { | ||||||
| type = layerOut.type = 'raster'; | ||||||
| Lib.log('Source types *raster* and *image* must drawn *raster* layer type.'); | ||||||
| } | ||||||
|
|
@@ -83,20 +85,20 @@ function handleLayerDefaults(layerIn, layerOut) { | |||||
| coerce('minzoom'); | ||||||
| coerce('maxzoom'); | ||||||
|
|
||||||
| if(type === 'circle') { | ||||||
| if (type === 'circle') { | ||||||
| coerce('circle.radius'); | ||||||
| } | ||||||
|
|
||||||
| if(type === 'line') { | ||||||
| if (type === 'line') { | ||||||
| coerce('line.width'); | ||||||
| coerce('line.dash'); | ||||||
| } | ||||||
|
|
||||||
| if(type === 'fill') { | ||||||
| if (type === 'fill') { | ||||||
| coerce('fill.outlinecolor'); | ||||||
| } | ||||||
|
|
||||||
| if(type === 'symbol') { | ||||||
| if (type === 'symbol') { | ||||||
| coerce('symbol.icon'); | ||||||
| coerce('symbol.iconsize'); | ||||||
|
|
||||||
|
|
@@ -105,7 +107,7 @@ function handleLayerDefaults(layerIn, layerOut) { | |||||
| noFontVariant: true, | ||||||
| noFontShadow: true, | ||||||
| noFontLineposition: true, | ||||||
| noFontTextcase: true, | ||||||
| noFontTextcase: true | ||||||
| }); | ||||||
| coerce('symbol.textposition'); | ||||||
| coerce('symbol.placement'); | ||||||
|
|
||||||
Uh oh!
There was an error while loading. Please reload this page.