Conversation
|
Also I wasn't sure how to handle if the user provides center or zoom. Right now it will simply not do box fitting if either of those params are provided. |
|
Hi @palmerusaf ! Sincere apologies for my delay in getting back to you. I think this approach makes sense at a high level. My only concern is the performance; could you do a basic benchmark of the time to compute If there's a noticeable performance impact, maybe we could still pursue this approach but provide a way to disable auto-fitting of bounds. Or maybe the fact that it can be skipped by manually setting I'll leave a few other small comments on the code. The implementation isn't expected to be working quite yet, correct? |
| var { minLat, maxLat } = getMinBoundLat(lat); | ||
| // this param is called bounds in mapLibre ctor | ||
| // not to be confused with maxBounds aliased below | ||
| containerOut.fitBounds = { |
There was a problem hiding this comment.
Prefix this property with an underscore since it's an internal property, not part of the plot schema.
| containerOut.fitBounds = { | |
| containerOut._fitBounds = { |
| containerOut._input = containerIn; | ||
| } | ||
|
|
||
| function getMinBoundLon(lon) { |
There was a problem hiding this comment.
| function getMinBoundLon(lon) { | |
| function getLonBounds(lon) { |
| } | ||
| } | ||
|
|
||
| function getMinBoundLat(lat) { |
There was a problem hiding this comment.
| function getMinBoundLat(lat) { | |
| function getLatBounds(lat) { |
| pitch: opts.pitch, | ||
| bounds: fitBounds, | ||
| fitBoundsOptions: { | ||
| padding: 20, |
There was a problem hiding this comment.
Since this padding is used twice, define it as a constant in map/constants.js.
Will these fitBoundsOptions have any effect if bounds is null?
|
@palmerusaf are you still interested in working on this PR? |
These work with anti-meridian unlike turf.js.
5dc6b40 to
2ade804
Compare
2ade804 to
7e08880
Compare
|
@camdecoster @emilykl I apologize for the late reply. I've made all the changes suggested above. Also, I have rebased this branch on top of the current master branch. I also performed the suggested performance tests in the browser console. The performance hit of the longitudinal sorting does become quite noticeable when points are in the 1e7 range. Below are the results of those tests. I've went ahead and added a flag to disable dynamic centering. However, by default it is enabled. Would you prefer this to be reversed where dynamic centering is an opt-in option? (function() {
function static(data) {
const gd = document.createElement("div");
Plotly.newPlot(
gd,
[
{
type: "scattermap",
mode: "markers",
lat: data.lat,
lon: data.lon,
marker: {
color: "#636efa",
},
},
],
{
map: {
disableDynamicCentering: true,
},
},
);
Plotly.purge(gd);
gd.remove();
}
function dynamic(data) {
const gd = document.createElement("div");
Plotly.newPlot(gd, [
{
type: "scattermap",
mode: "markers",
lat: data.lat,
lon: data.lon,
marker: {
color: "#636efa",
},
},
]);
Plotly.purge(gd);
gd.remove();
}
// generate lat/lon data
function generateLatLon(n) {
return {
lat: Array.from({ length: n }, () => Math.random() * 180 - 90),
lon: Array.from({ length: n }, () => Math.random() * 360 - 180),
};
}
const testList = [1e2, 1e3, 1e4, 1e5, 1e6, 1e7];
for (let i = 0; i < testList.length; i++) {
console.log(`Test with 1e${i + 2} data.`);
// if you run 1e7 more than once you run out of memory
const numOfRuns = i < testList.length - 1 ? 10 : 1;
const data = generateLatLon(testList[i]);
window.timeit(static, numOfRuns, 1, data);
window.timeit(dynamic, numOfRuns, 1, data);
}
})(); |
This is for issue #7674. This uses a custom algo to get the bounds, then uses mapLibre to set the zoom/center via fitBounds. Turf.js doesn't handle the anti-meridian correctly. If you'd like I can add the helper functions to geo location utils to knock out a TODO there. I just need to refactor the signatures.