fix(RenderWindow): Support custom sizes for screenshots #1907

scottwittenburg · 2021-05-11T20:34:58Z

Currently the screenshot capture functionality captures the render window image as it appears on the screen. However, some applications could benefit from the ability to specify an arbitrary size when capturing an image and have it rendered at full resolution rather than resampled.

I haven't tried this with WebGPU, but just provided an implementation the same as that for OpenGL.

scottwittenburg · 2021-05-11T20:37:04Z

@jourdain This is working for MIQA, but let me know what you think. Doing the resetCamera, but storing the current parameters first, seems to improve the situation when the requested screenshot size has a drastically different aspect ratio than whatever is displayed currently. But maybe that's not the right approach.

jourdain · 2021-05-11T21:37:26Z

Sources/Rendering/WebGPU/RenderWindow/index.js

+        } else {
+          // Create a stand-in image overlay while we resize and render
+          const tmpImg = document.createElement('img');
+          tmpImg.width = model.size[0];


is that needed?

Ah right, I think probably not needed. Will fix.

jourdain · 2021-05-11T21:37:32Z

Sources/Rendering/WebGPU/RenderWindow/index.js

+          // Create a stand-in image overlay while we resize and render
+          const tmpImg = document.createElement('img');
+          tmpImg.width = model.size[0];
+          tmpImg.height = model.size[1];


is that needed?

jourdain · 2021-05-11T21:39:26Z

Sources/Rendering/WebGPU/RenderWindow/index.js

+            // Without this resetCamera(), we can end up showing only a subset
+            // of the prop(s) depending on the difference between the aspect
+            // ratios of the original size and the custom capture size.
+            ren.resetCamera();


This should be optional and off by default.

Yeah, I was wondering about that. It's kind of best effort anyway. To really allow you to get what you want in your screenshot, we'd probably have to call you back before we render and let you compute/set what you want.

But you think if we just make it optional (and default off), the way it is now could be good enough?

What should we call that option? screenshotResetCamera or resetCameraOnResizeScreenshot? Or just resetCamera and document it?

opts = { resetCamera?: false, // Should only be needed if aspect ratio is different (Maybe auto on if size provided / but not sure) size?: [width, height], scale?: 1, // maintain aspect ratio but allow to adjust size based on original size? }

jourdain · 2021-05-11T21:44:07Z

Sources/Rendering/WebGPU/RenderWindow/index.js

+          publicAPI.modified();
+          // save camera parameters before resetCamera so we can restore
+          // them later
+          model.activeCamParamMap = new WeakMap();


I would not use a weak map but just rely on the order of the renderer because it really happen in a non-interactive setup.

You should use a private object on the model.

model._screenshot = { originalSize: [], imagePaceHolder: ..., cameras: [], };

jourdain · 2021-05-11T21:44:22Z

Sources/Rendering/WebGPU/RenderWindow/index.js

+          model.originalSize = [model.size[0], model.size[1]];
+
+          // hide it
+          model.canvas.style.display = 'none';


I don't see you reversing it?

Reversing it is done in the updateWindow method as a result of calling publicAPI.modified().

jourdain · 2021-05-11T21:49:31Z

Sources/Rendering/WebGPU/RenderWindow/index.js

+          // save camera parameters before resetCamera so we can restore
+          // them later
+          model.activeCamParamMap = new WeakMap();
+          model.renderable.getRenderers().forEach((ren) => {


The camera stuff is only required if we do the reset camera.

You can write it better like:

mode._screenshots.cameras = model.renderable.getRenderers().map( (renderer) => renderer.getActiveCamera().get('focalPoint', 'position', 'parallelScale') );

Then when you revert it you can do

model.renderable.getRenderers().forEach((renderer, idx) => { renderer.getActiveCamera().set(model._screenshot.cameras[idx]); });

You could even bundle the fn with it like

mode._screenshots.cameras = model.renderable.getRenderers().map((renderer) => { const camera = renderer.getActiveCamera(); const arg = camera.get('focalPoint', 'position', 'parallelScale'); const fn = camera.set; const resetCamera = renderer.resetCamera; return { fn, arg, resetCamera }; }); // resetCamera (after all the param capture as cross camera link could mess up your parameter capture) mode._screenshots.cameras.forEach(({ resetCamera }) => resetCamera()); // reset mode._screenshots.cameras.forEach(({ fn, arg }) => fn(arg));

you may have to clone the arg depending if you get a ref or a copy of each array.

clone = (obj) => JSON.parse(JSON.stringify(obj))

jourdain · 2021-05-11T21:53:50Z

Sources/Rendering/WebGPU/RenderWindow/index.js

+          model.canvas.style.display = 'none';
+
+          // resize it
+          model.size[0] = model.customCaptureSize[0];


model._screenshot.originalSize = model.size; model.size = model._screenshot.size; [...] model.size = model._screenshot.originalSize;

Sources/Rendering/WebGPU/RenderWindow/index.js

jourdain · 2021-05-11T21:55:46Z

Sources/Rendering/Core/RenderWindow/index.js

  };

-  publicAPI.captureImages = (format = 'image/png') => {
+  publicAPI.captureImages = (opts = {}) => {


I would add opts as a second argument to prevent a BREAKING_CHANGE...

jourdain · 2021-05-11T22:04:28Z

Sources/Rendering/WebGPU/RenderWindow/index.js

+          const tmpImg = document.createElement('img');
+          tmpImg.width = model.size[0];
+          tmpImg.height = model.size[1];
+          tmpImg.style = {


You may want to have that style as a const at the module level. tmpImg.style = SCREENSHOT_PLACE_HOLDER;

Sources/Rendering/WebGPU/RenderWindow/index.js

scottwittenburg · 2021-05-14T01:39:07Z

@jourdain Let me know if I should at least document the duplication between the OpenGL and WebGPU implementations or something. Or if there's anything else you think this needs. Otherwise maybe we can target merging it tomorrow? Thanks!

scottwittenburg · 2021-05-21T00:31:42Z

@jourdain Do you think this is still missing something? If just rebase and force-push the branch and the checks are still green, could we merge it?

github-actions · 2021-05-21T15:18:17Z

🎉 This PR is included in version 18.1.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

scottwittenburg · 2021-05-21T15:20:24Z

Thanks @jourdain 👍

scottwittenburg requested a review from jourdain May 11, 2021 20:35

scottwittenburg mentioned this pull request May 11, 2021

Fix issue with screenshot size OpenImaging/miqa-phase1#114

Open

jourdain reviewed May 11, 2021

View reviewed changes

Sources/Rendering/WebGPU/RenderWindow/index.js Outdated Show resolved Hide resolved

jourdain reviewed May 11, 2021

View reviewed changes

scottwittenburg force-pushed the resizable-screenshots branch 2 times, most recently from e22423e to 097074c Compare May 13, 2021 21:25

jourdain reviewed May 13, 2021

View reviewed changes

Sources/Rendering/WebGPU/RenderWindow/index.js Outdated Show resolved Hide resolved

scottwittenburg force-pushed the resizable-screenshots branch from 097074c to b4e0114 Compare May 13, 2021 21:32

fix(RenderWindow): Support custom sizes for screenshots

ab45a23

scottwittenburg force-pushed the resizable-screenshots branch from b4e0114 to ab45a23 Compare May 21, 2021 14:27

jourdain merged commit 12f97e6 into master May 21, 2021

jourdain deleted the resizable-screenshots branch May 21, 2021 15:09

github-actions bot added the released Automated label label May 21, 2021

Uh oh!

fix(RenderWindow): Support custom sizes for screenshots #1907

fix(RenderWindow): Support custom sizes for screenshots #1907

Uh oh!

Conversation

scottwittenburg commented May 11, 2021

Uh oh!

scottwittenburg commented May 11, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scottwittenburg May 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jourdain May 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

scottwittenburg commented May 14, 2021

Uh oh!

scottwittenburg commented May 21, 2021

Uh oh!

github-actions bot commented May 21, 2021

Uh oh!

scottwittenburg commented May 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

scottwittenburg May 11, 2021 •

edited

Loading

jourdain May 11, 2021 •

edited

Loading