Geo refactor and lasso/select-box selections #2030
Conversation
Temporarily committed as a new file to help reviewers New stuff: - d3-idiomatic base layers updates - better layer scheme (inspired by ternary) - persistent projection updates - cleaner projection scale/translate logic - faster render (using cached d3 selections)
- no need for separate files.
- `geo.center` was required for persistent map interactions - make `projection.rotation.lon` default value depend on lonaxis range (to get correct view for e.g. #1698) - use lonaxis range OR rotation lon to determine the `geo.center.lon` default - use lataxis range to determine `geo.center.lat` default
- as now all interactions can be describe using `geo` attributes - it's about time 🎉
- and emit mocked `plotly_relayout` event on mouseup - I chose to not call Plotly.relayout directly for performance reasons.
- note that we now bind a calcdata item with a ref to geojson to the scattergeo line nodes, instead of stitching calcdata things onto geojson feature.
- make choropleth trace go though a calc step - use lib/polygon on hover 🎉, instead of old ad hoc mouse-event-on-svg-node way) - use clean enter/exit/merge d3 pattern instead of inner node purgery - bind 'real' calcdata items to selections, instead of patched geojson objects.
- make sure dragmode is set to 'pan' by default for geo plots - add select and lasso mode bar button to geo plots - add updateFx fast relayout pass - init dragElement instance *just for* lasso and select drag modes, note that 'pan' still uses d3's drag abstaction
- with test support.
- with test support. - 'simply' use the location's centroid to determine if it is selected or not.
- while #292 was fixed thanks to `geo.center`, it is 🔒 here.
- while #1698 was fixed by the improve geo defaults, this 🔒 it down.
- fixed earlier, 🔒 here.
- side effect of new geo center logic
- that were affected by the new `geo.center` logic
- showing new way of doing things with `geo.center`
- fixing an issue-less bug discovered while working on this PR.
- which are mostly just offset by a few pixels.
- move enter-only methods to enter-block - and use '.style()` instead of `.attr()` for consistency with Color and Drawing methods.
- using stashed geo layers
- to 🔒 west <-> east antimeridian crossings
... for hover when out-of-range pts are present in *albers usa* map
- making doModebar update subroutine much more manageable
test/jasmine/tests/geo_test.js
Outdated
| }); | ||
| }); | ||
|
|
||
| it('should not throw during hover when out-of-range pts are present in *albers usa* map', function(done) { |
There was a problem hiding this comment.
which ✅ #2030 (comment)
Note that I kept the scattergeo-out-of-usa because the image test does 🔒 down the bug fix.
See the rogue point(s) in the upper-left corner:
this thing ⏬
But yeah, adding a jasmine test documents the behavior better, so here it is.
... when dragging off-screen - attach handlers to bgRect (not framework), doesn't change any thing, but is more consistent.
... to not confuse positions on either side of the globe
- in that case, call Plotly.relayout to reset all attributes that could cause the invalid bounds.
- these countries are Russia and Fiji. - Note that other countries have polygons on either side of the antimeridian (e.g. some Aleutian island for the USA), but those don't confuse the 'contains' method.
- instead of having components making <g .clips> before appending their <clipPath>s - IMPORTANT: annotation put is clips in fulLayout._defs, even though annotation draw in `toppaper`. Now, annotations use fullLayout._topclips
| @@ -194,7 +194,7 @@ function drawRaw(gd, options, index, subplotId, xa, ya) { | |||
|
|
|||
| var isSizeConstrained = options.width || options.height; | |||
|
|
|||
| var annTextClip = fullLayout._defs.select('.clips') | |||
| var annTextClip = fullLayout._topclips | |||
There was a problem hiding this comment.
@alexcjohnson IMPORTANT: Annotation used to put its clips in fulLayout._defs, even though Annotation draws in toppaper. Clip ids are global to the page, so things still work fine (at in Chrome and FF). But, who knows maybe some other browsers don't apply clip ids outside the <svg> where the <defs> is located.
There was a problem hiding this comment.
Nice find! Definitely not the intent, thanks for cleaning that up!
|
@etpinard and I discussed bubble opacity a bit more - I was on the fence but he won me over with keeping symmetry with regular |
- ++ cleaner hover polygon logic - no mutation when offsetting RUS and FJI longitudes - do not offset RUS and FJI polygons that do not need modification for hover!
src/plots/geo/geo2.js
Outdated
|
|
||
| for(k in this.dataPoints) { | ||
| this.dataPoints[k] | ||
| .attr('display', hideShowPoints) |
There was a problem hiding this comment.
marker.opacity wasn't applied properly on scattergeo bubbles
Was it just the default being applied improperly, or was opacity not supported before?
This here is why suddenly marker.opacity is applied correctly in scattergeo traces. Previously we used opacity 0 Drawing.pointStyle set. See #1861 (comment) for why we now use display hide/show points.
@etpinard and I discussed bubble opacity a bit more - I was on the fence but he won me over with keeping symmetry with regular scatter. So the behavior introduced here is great.
👌 let's leave it as is.
(Referencing commits in: geo-refactor+lasso...try-bigger-clippad) FYI I was able to reproduce the bug using a simple The problem can be fixed by increasing the @alexcjohnson you can uncomment this line to draw the lon/lat range-box polygon on the map if you feel like investigating further. Bumping There might be other situations where |
I'm uncomfortable with this solution. For example, what if you zoom in sufficiently that 0.1 (or even 0.01) is a noticeable fraction of the range? I think we need to step back and look into where this is coming from a little closer. The bug exists on master (in fact it's worse, you just get the top or bottom half of the viewport rather than a centered half, see below) so lets ignore it for this PR and make a new bug report for it. |
|
💃 🕺 💃 🕺 Fantastic work - so much more solid and well-reasoned! |
In brief, this PR brings geo subplots up-to-speed with other subplots. 🚀
Geo subplots have long suffered from a few design mistakes (e.g. as discussed #292 and #1486). This PR attempts to solve these issues - which are required to make lasso/select-box selections work.
Since the geo subplot constructor code was mostly rewritten (see 4c1ddb8), I decided to include the new lasso/select-box logic in this PR as well. I hope the reviewers won't mind. You may consider geo as a new subplot type and this its introduction PR.
What's inside this PR:
Stuff implemented:
geo.centera new attribute which should've been there from the start Addgeo.centerattribute to specify map view without rescaling #1486scattergeoandchoropleth(selections about centroid) lasso and select boxStuff fixed:
geo.center) 🔒 in 3bd89d3scattergeomarkers outside 'usa' scope now render correctly geo.scope='usa' with points outside USA gives console errors on hover #1996 🔒 in eaf44a8lonaxisranges now behave correctly Can't set longitude range between [160, -160] #1698 🔒 in e59c90bmarker.opacitywasn't applied properly onscattergeobubbles 🔒 in 7e5bb41Baseline updates:
Unfortunately, all geo baselines had to be updated.
geo_fillwas adapted to showcase the newgeo.centerattribute (see 36833d4)Stuff I could do in this PR, but could wait for v2:
cleanLayoutmap)geo.projection.scale➡️geo.zoom(as in mapbox subplots)cleanDatamap) choroplethz➡️values(similar to pie traces)Open items:
master). But really, we should make the base layer ordering consistent for all trace type combinations in v2. But in the meantime, we should make the base layer ordering customizable (similar toaxis.layer).dragElement(i.e. get rid ofd3.behavior.drag) and implement dragmode: 'zoom'` zoomboxscattergeo/plot.jsupdate inner nodes instead of purging them. This is required for smooth animations and should be a performance boost.Stuff I believe should wait for v2:
scrollZoomconfig option No way to disable mouse-scroll zoom for geo subplots #143. Unfortunately. geo assumesscrollZoom: true, but thescrollZoomdefault isfaise. We could add some logic to makescrollZoomdefault totrueif a geo subplot is present to preserve backward compatibility, but that hardly sounds worth it (to me at least).PlotlyGeoAssetsglobal with some field onPlotly. No need to have another global variables, but this isn't a huge priority.cc @alexcjohnson @rreusser @chriddyp @jackparmer @cldougl
and @cpsievert (this here might make him start using
geoin the R API 😝 )