Add Window.takeScreenshot API #225

OhadRau · 2019-01-20T20:43:01Z

Great news -- took another quick look at this and figured out what I was doing wrong! Was getting some build errors after I rebased this but I think that's just due to some weird build artifacts since it had to do with fonts being duplicated for some reason... (Fixes #185)

OhadRau · 2019-01-21T01:41:10Z

Realized I really fucked up my git history before somehow but just implemented this in the example with a "Take screenshot" button. Looks to be working great on my end but would appreciate other people trying this out on their PCs (& especially the WebGL version in different browsers/OSes).

Btw, one thing I noticed (which I don't think is related to this) is a really big decrease in performance when I maximize the examples window on my laptop. Takes the FPS from ~70 to ~15 and looks very very choppy. Looks like we're still very far away from the performance we're shooting for :/

bryphe · 2019-01-21T21:45:01Z

src/Core/Window.re

+  let image = Image.create(~width, ~height, ~numChannels=4, ~channelSize=1);
+  let buffer = Image.getBuffer(image);
+
+  /* WebGL is weird in that we can't capture with glReadPixels during


Thanks for the comment here! I was wondering why we needed the extra render call.

bryphe · 2019-01-21T21:48:42Z

examples/Examples.re

@@ -116,7 +133,14 @@ let init = app => {
      />;
    };

-    let buttons = List.map(renderButton, s.examples);
+    let buttons = [
+      <ExampleButton


The UX for the example is confusing to me. All the other examples behave as sort of a 'tab' - you click the example, and the example is rendered. The 'Take Screenshot' behaves differently - it takes a screenshot when you press it, and it always looks highlighted.

Instead - could we have the 'Take Screenshot' example be like the other examples, but have a 'Take Screenshot' button - similiar to the DefaultButton example, but instead of just a counter, it would actually run the screenshot code.

When that button is clicked, it would show the path to where the screenshot is saved. Right now - there is no feedback when the 'Take Screenshot' button is pressed, so it's a bit confusing.

I'm open to other ideas too to showcase this. But the current behavior isn't clear about what's happening - I want to make sure that we show the user where it is saved, so they know something happened. In WebGL - it's not too big of deal, since you see the download pop-up. But for native, it's important to know that something actually happened and where to find the screenshot for inspection.

Just an idea, but wouldn't it be fun to show the screenshot as an image in the example itself? Of course the path would be important too.

It would also be fun to take screenshots from the other examples, like from the game of Life :-). Not sure how to make that into a friendly UI

Cool idea @tcoopman !

Little confused about what you mean. Do you want a whole example tab for the screenshot button? I added this as a separate button so that you can take screenshots from any tab, so I want to keep the button globally accessible if possible. But I do agree that the UX isn't great right now. In terms of feedback, I could try to do a modal using a second window to spice it up. This could also preview the image in that window I guess.

Do you want a whole example tab for the screenshot button?

Yes, this is what I was thinking. For the example app, its more about showing the capabilities than anything else - so I think this is reasonable. Longer-term, I want to publish the examples to a website, and as you click on the tabs, it'd show the source code for the selected tab next to the example app - so in this case, they could click the 'Screenshot' tab, and see how to use the Window.takeScreenshot API.

I added this as a separate button so that you can take screenshots from any tab, so I want to keep the button globally accessible if possible. But I do agree that the UX isn't great right now. In terms of feedback, I could try to do a modal using a second window to spice it up. This could also preview the image in that window I guess.

If you prefer this, we could add a 'button section' at the bottom of the left pane and put the screenshot button there. At least differentiate it so that it is clear that it triggers an 'action' and its not a 'tab'.

Ok thanks for the feedback. Any tips on how I can get a reference to the current window in the other component (needed to call the Window.takeScreenshot function)? I guess I would have to pass it in as a prop of some kind...

Ya actually a few of the examples need the window anyway - it gets passed as a parameter to the render function, like:

{name: "Input", render: w => InputExample.render(w)},

And then in the InputExample.re, it passes the window as a prop:

let render = window => <Example window />;

bryphe · 2019-01-21T21:59:09Z

Thanks @OhadRau for integrating the screenshot API w/ Revery! The Window.takeScreenshot API looks great 👍

I found the UX for the example a bit confusing (it broke the convention of the other examples, and it wasn't clear that anything happened in native when clicking 'Take Screenshot') - I left more specific feedback in the comments. Let me know what you think.

Btw, one thing I noticed (which I don't think is related to this) is a really big decrease in performance when I maximize the examples window on my laptop. Takes the FPS from ~70 to ~15 and looks very very choppy. Looks like we're still very far away from the performance we're shooting for :/

Could you please log an issue for this? It would be helpful to know the exact environment - I think you mentioned you're on a SurfaceBook 2 + WSL? It would also be helpful in case other people are running into this.

In addition - if you run with the REVERY_DEBUG=1 environment variable, we do some perf logging - it can help us see where the bottleneck might be.

I tried to reproduce this (I have a SurfaceBook 1), and I tried a bunch of different things - I put it on Battery Saver mode, etc and ran the app fullscreen - it stayed at a smooth 60 FPS for me.

However, one thing I found, was that, it seems in WSL, a lot of people have issues where it's using software OpenGL instead of hardware drivers. Some references:

I'd be curious if you've done anything to set up hardware GPU drivers or LibGL pass through to the host - otherwise, it's likely software emulation the OpenGL APIs is taking place, and that would be very slow on a high res display like the SurfaceBook 2.

Another thing to try would be building & running Revery outside of WSL, just via Win32 command - that should have better driver support and perform better than a WSL build. Curious if that's better!

bryphe

I'd like to see if we could make the 'Screenshot' example similiar to the other ones, or improve the UX so that it's clear that 'Take Screenshot' is a button and that we message in some way where the screenshot is when running via native.

OhadRau · 2019-01-22T20:50:10Z

@bryphe I'm having some trouble loading the images that are dynamically generated. I've tracked down the issue to how the paths are generated when loading image files in ImageRenderer.re. Saving the file follows the Unix.getcwd() result (directory you run esy x Examples from) whereas loading uses the Environment.getExecutingDirectory() result (directory where the exe is located). Because this is automatically prepended to the path of the image, it's not enough to just give the absolute path of an image, instead you'd have to pass something like ../../.. if you wanted to go up to a different directory. I don't think this makes for a great UX here but I'm not sure what the optimal solution is here, maybe it would make sense to check multiple paths? Note this is broader than just this PR specifically, since you wouldn't be able to do <Image src="~/.myapp/res/img.png"/> or <Image src="/var/myapp/res/img.png"/>, you'd have to expand that into a relative path from the executing directory.

bryphe · 2019-01-23T00:27:12Z

Because this is automatically prepended to the path of the image, it's not enough to just give the absolute path of an image, instead you'd have to pass something like ../../.. if you wanted to go up to a different directory.

Good catch. Yes, I think the src parameter of Image should be smart enough to know if its an absolute path - if it is, it shouldn't prepend Environment.getExecutingDirectory().

bryphe · 2019-01-23T01:03:14Z

I created a PR to check if the path is absolute in #242 . Not sure if we can handle this show-screenshot case well in WebGL (I guess we could create a blob URL for it) - but I'm OK making that piece only-native for now.

OhadRau · 2019-01-24T21:04:12Z

@bryphe Changed the example to be just another tab and display the image. Let me know if there's anything else you think I should change.

Also, any thoughts about exposing the x,y,width,height parameters for glReadPixels? I think this might be useful but it also seems very low-level to include in Revery.

tcoopman · 2019-01-24T21:11:40Z

examples/Examples.re

@@ -31,6 +31,7 @@ let state: state = {
    {name: "Focus", render: _ => Focus.render()},
    {name: "Stopwatch", render: _ => Stopwatch.render()},
    {name: "Input", render: w => InputExample.render(w)},
+    {name: "Screen Capture", render: w => ScreenCapture.render(w)},


wouldn't it be useful to put the Screen Capture button at the bottom of the screen? For example with something like this:

style=Style.[ position(`Absolute), top(0), left(0), width(175), bottom(0), backgroundColor(bgColor), ]> ...buttons </View> <View style=Style.[ position(`Absolute), bottom(0), left(0), width(175), backgroundColor(bgColor), ]> screenCaptureButton </View>

Hm, do you want me to go back and do that? Right now this is setup to be in its own tab as bryan pointed out. This is a little weird because you can't get screenshots of everything but it's more of a proof of concept than anything else so I'm not too concerned about it. One of the advantages of having it in its own tab is that we can display the image right there so we can really show it in action. @tcoopman

I'm OK with it is, for now - what I'd like to do with the example app is host it on a website, and show the example code for the tab side-by-side with the selected example.

For the screenshot example - it'd be nice to see how easy it is to implement this behavior w/ that code snippet alongside. We can always revisit down the road though if we want to shuffle it around.

bryphe · 2019-01-30T17:05:42Z

Just tried it in both native and JS - looks great @OhadRau ! Thanks for the work on this 👍

OhadRau · 2019-01-31T04:52:57Z

Awesome guess I'll merge this in then!

OhadRau added 2 commits January 20, 2019 20:22

Integrate Window.takeScreenshot into examples

e763bbe

Add Window.takeScreenshot

d8be26e

OhadRau force-pushed the take-screenshot branch from daeabf8 to d8be26e Compare January 21, 2019 01:27

OhadRau requested a review from bryphe January 21, 2019 20:46

bryphe reviewed Jan 21, 2019

View reviewed changes

bryphe requested changes Jan 21, 2019

View reviewed changes

bryphe mentioned this pull request Jan 23, 2019

Asset Loading: Allow absolute paths #242

Closed

3 tasks

Change screen capture example

6575d1d

tcoopman reviewed Jan 24, 2019

View reviewed changes

OhadRau added 2 commits January 24, 2019 20:43

Merge in new styles

f4913b4

Fix formatting

3ad4e55

Merge branch 'master' into take-screenshot

a7c682b

bryphe approved these changes Jan 30, 2019

View reviewed changes

OhadRau merged commit f773acd into revery-ui:master Jan 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Window.takeScreenshot API #225

Add Window.takeScreenshot API #225

OhadRau commented Jan 20, 2019

OhadRau commented Jan 21, 2019

bryphe Jan 21, 2019

bryphe Jan 21, 2019

tcoopman Jan 21, 2019

bryphe Jan 21, 2019

OhadRau Jan 21, 2019

bryphe Jan 22, 2019

OhadRau Jan 22, 2019

bryphe Jan 22, 2019

bryphe commented Jan 21, 2019

bryphe left a comment

OhadRau commented Jan 22, 2019

bryphe commented Jan 23, 2019

bryphe commented Jan 23, 2019

OhadRau commented Jan 24, 2019 •

edited

Loading

tcoopman Jan 24, 2019

OhadRau Jan 25, 2019 •

edited

Loading

bryphe Jan 30, 2019

bryphe commented Jan 30, 2019

OhadRau commented Jan 31, 2019

Add Window.takeScreenshot API #225

Add Window.takeScreenshot API #225

Conversation

OhadRau commented Jan 20, 2019

OhadRau commented Jan 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryphe commented Jan 21, 2019

bryphe left a comment

Choose a reason for hiding this comment

OhadRau commented Jan 22, 2019

bryphe commented Jan 23, 2019

bryphe commented Jan 23, 2019

OhadRau commented Jan 24, 2019 • edited Loading

Choose a reason for hiding this comment

OhadRau Jan 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryphe commented Jan 30, 2019

OhadRau commented Jan 31, 2019

OhadRau commented Jan 24, 2019 •

edited

Loading

OhadRau Jan 25, 2019 •

edited

Loading