-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Add ImageSource.updateImage() #7342
Conversation
As pointed out by the original submitter of the bug, I need to test changing the image size before merging. Edit: looks like this is no problem. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking this on! It works well. I left some feedback on a few things. I'd also suggest updating the Animate a series of images example to use this new approach. I'd also love to have some unit tests for this functionality; we probably need one to test changing the URL, one to test changing the coordinates and one to test what happens when you change the image size.
src/source/image_source.js
Outdated
* They do not have to represent a rectangle. | ||
* @returns {ImageSource} this | ||
*/ | ||
updateImage(options) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add some Flow typing here. See here and refer to the setCoordinates
method in this file to what your coordinates
param would look like. Make sure that coordinates
is optional.
src/source/image_source.js
Outdated
|
||
var updateCoords = Boolean(options.coordinates); | ||
|
||
getImage(this.map._transformRequest(options.url, ResourceType.Image), (err, image) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a lot of repetition of code in this. We could DRY it up quite a bit by using the existing load
and _finishLoading
functions which has the added bonus of firing the appropriate events and updating this.url
, etc. You have to change _finishLoading
so it can accept the coordinates as a parameter and it looks like you'd have to change load
to accept a callback so you can pass in the necessary functionality like this.texture = null
but otherwise it seems to work pretty well.
src/source/image_source.js
Outdated
this.fire(new Event('data', {dataType:'source', sourceDataType: 'content'})); | ||
} | ||
|
||
if (updateCoords && this.map) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just check options.coordinates
here instead of casting to a Boolean and saving the result as a variable.
debug/update_image.html
Outdated
}); | ||
|
||
const path = "/test/integration/image/"; | ||
const images = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we need quite so many gifs to be added to the library. You can illustrate this with just a few. In fact, you can probably modify this to use 'https://www.mapbox.com/mapbox-gl-js/assets/radar' + currentImage + '.gif'
as seen here and eliminate the need for these gifs entirely.
debug/update_image.html
Outdated
map.addSource("radar", { | ||
type: "image", | ||
url: getPath(), | ||
coordinates: [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be good to also update the coordinates on each iteration so that can easily be debugged.
src/source/image_source.js
Outdated
* | ||
* // update url. | ||
* // Note: this is most effective if your layer has the `raster-fade-duration` paint property set to 0. | ||
* mySource.updateImage({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably only need one example here. Just show it with url
and coordinates
. The docs should note that coordinates
is optional.
@ryanhamley I think I got all of the comments addressed except for a unit test around an image update with a size change. Not really sure how to do that one. FWIW, the code in |
debug/update_image.html
Outdated
}); | ||
|
||
// To illustrate moving coordinates at the same time | ||
coordinates.forEach(c => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This moves the image off the screen pretty rapidly and eventually will cause an error when the latitude goes above 90°. I know I suggested this but maybe it would be better to remove it. Sorry about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem.
Hey @dcervelli sorry it took a few days to get back to you. I think it's looking pretty good now. I made a comment on the debug page. I think now that I see it, we should either remove the coordinate updates or loop through them so the image doesn't move off the page and eventually off the map. Other than that, I'm ready to give this a 👍 |
Fixes #4050.
H/t to the folks on the #4050 thread who showed the way to do this.
Launch Checklist
@mapbox/studio
and/or@mapbox/maps-design
if this PR includes style spec changes