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

Add a map.screenshot() function. #665

Merged
merged 4 commits into from
Feb 9, 2017
Merged

Add a map.screenshot() function. #665

merged 4 commits into from
Feb 9, 2017

Conversation

manthey
Copy link
Contributor

@manthey manthey commented Feb 6, 2017

This returns either a data url or an HTMLCanvasElement.

You do not need to preserve drawing buffers for this to work -- that doesn't alway work in Chrome in any case, so WebGL layers are rerendered as part of the screenshot process.

This needs testing.

This returns either a data url or an HTMLCanvasElement.

You do not need to preserve drawing buffers for this to work -- that doesn't alway work in Chrome in any case, so WebGL layers are rerendered as part of the screenshot process.

This needs testing.
@manthey
Copy link
Contributor Author

manthey commented Feb 6, 2017

@aashish24 This provides as screenshot of all canvas layers (or a subset of canvas layers). It doesn't require preserveDrawingBuffer to be set ahead of time.

I still need to add tests and remove the functionally duplicated code in the testing utilities.

If we want to get non-canvas layers, it looks like there is a javascript library that can render html to canvas to some degree that we can look into.

@codecov-io
Copy link

codecov-io commented Feb 6, 2017

Codecov Report

Merging #665 into master will increase coverage by 0.02%.

@@            Coverage Diff             @@
##           master     #665      +/-   ##
==========================================
+ Coverage   90.59%   90.62%   +0.02%     
==========================================
  Files          84       84              
  Lines        8578     8615      +37     
==========================================
+ Hits         7771     7807      +36     
- Misses        807      808       +1
Impacted Files Coverage Δ
src/map.js 98.84% <100%> (+0.06%)
src/mapInteractor.js 95.05% <ø> (-0.35%)
src/layer.js 83.33% <ø> (+0.53%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update edebe10...8f82e75. Read the comment docs.

Attach a data element to the map's node so that the map is reachable.  If a second map is attached to the same node, the first map is destroyed via map.exit().

Improve robustness of testing the blog-lines example.  Make sure foreign libraries have a chance to load.
@manthey manthey changed the title [WIP] Add a map.screenshot() function. Add a map.screenshot() function. Feb 7, 2017
@aashish24
Copy link
Member

@aashish24 This provides as screenshot of all canvas layers (or a subset of canvas layers). It doesn't require preserveDrawingBuffer to be set ahead of time.

Cool! I will have my first pass on it today.

If we want to get non-canvas layers, it looks like there is a javascript library that can render html to canvas to some degree that we can look into.

that would be desirable but I am okay with if that is done in another PR.

src/map.js Outdated
* @returns {string|HTMLCanvasElement}: data URL with the result or the
* HTMLCanvasElement with the result.
*/
this.screenshot = function (layers, type, encoderOptions) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the API

src/map.js Outdated
result.height = m_height;
var context = result.getContext('2d');
// start with a white background
context.fillStyle = 'white';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@manthey do we not need to set the transparency? I believe the later drawImage call will update the final value to the combined value of all opacities.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The drawImage properly uses the opacity of individual pixels in the canvas, so layers composite properly. But... I did forget that we adjust the opacity of the containing div of the layer when you set the layer opacity, so I'll need to take that into account.

src/map.js Outdated
// with a library such as rasterizehtml (avialable on npm).
layers.forEach(function (layer) {
$('canvas', layer.node()).each(function () {
if (layer.renderer().api() === 'vgl') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to have it not check for vgl but I understand why we have to do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I original thought that if we preserved the drawing buffers, this wouldn't be necessary, but they aren't preserved when the browser tab is switched in any case, so this ends up being necessary.

Copy link
Member

@aashish24 aashish24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great.. Just has some minor notes.

@aashish24
Copy link
Member

The drawImage properly uses the opacity of individual pixels in the canvas, so layers composite properly. But... I did forget that we adjust the opacity of the containing div of the layer when you set the layer opacity, so I'll need to take that into account.

Awesome. I was wondering about that.

@aashish24
Copy link
Member

One another think I can think of is a GeoJS logo or something of that sort (it could be also used for other PR purposes as well) in the future. What you think?

I haven't tried running the branch locally but the changes looks reasonable to me now.

@aashish24
Copy link
Member

Also a widget might be useful for screencapture (along with widget to change the basemap).

@danlamanna
Copy link
Contributor

One another think I can think of is a GeoJS logo or something of that sort (it could be also used for other PR purposes as well) in the future. What you think?

You mean watermarking it? As a consumer of open source software, leaving a watermark of your brand on my images is what would make me not use the software anymore.

@kotfic
Copy link

kotfic commented Feb 9, 2017

It seems like adding a widget could be handled in a separate PR?

@manthey
Copy link
Contributor Author

manthey commented Feb 9, 2017

Adding a logo on top of stuff is tricky, as it has to be small to not obscure anything but large enough to be visible, and it has to work on a variety of background. I'd rather see that done as a optional map widget, and then we'd just get that as part of the screenshot if was deliberately present. It should definitely be opt-in, not opt-out.

@aashish24
Copy link
Member

It seems like adding a widget could be handled in a separate PR?

yes, all of these features @manthey and I are discussing is for another PR (and to add to the list of issues).

  • screenshot for non canvas layers
  • wartermarking
  • widgets for screencapture / basemaps

@manthey manthey merged commit 6686104 into master Feb 9, 2017
@manthey manthey deleted the canvas-screenshot branch February 9, 2017 17:01
@aashish24
Copy link
Member

You mean watermarking it? As a consumer of open source software, leaving a watermark of your brand on my images is what would make me not use the software anymore.

@danlamanna wartermaking is not necessarily of geojs logo but for a particular project or company logo. This is something used by the PR folks mostly for presentations or for branding purposes. We can have it off by default.

@aashish24
Copy link
Member

@danlamanna
Copy link
Contributor

Whatever, as long as we don't force it upon the user or leave it as the default or anything like that. #stillOpposed

@aashish24
Copy link
Member

Whatever, as long as we don't force it upon the user or leave it as the default or anything like that. #stillOpposed

+1, totally agree.

@manthey
Copy link
Contributor Author

manthey commented Feb 9, 2017

We should create issues for any additional features. I'll ask one more question, though: since you can't copy an image to the clipboard via javascript, what would a screenshot widget do? Does it copy an image to a div so the user can then copy the image to the clipboard themselves? Does it copy the image as a base64-encoded string to the clipboard? Something else?

@aashish24
Copy link
Member

@manthey I was hoping (not sure if possible in pure JS environment as I haven't looked) but plottly does have this UI for downloading the plot as png. https://plot.ly/~ElPolloFrio/1911

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

Successfully merging this pull request may close these issues.

5 participants