Fix copyImageToCanvas bug and add ReadImageToCanvasOptions #7539
Conversation
andremig-bentley
requested review from
a team,
bbastings,
aruniverse and
ben-polinsky
as code owners
Canvas decorations are not the only kinds of decorations. The application should be able to configure what decorations are or not displayed (just as they can control what other content is displayed) in the viewport, separately from capturing an image from the viewport's contents.
|
Arguably, you have introduced a different bug by changing the existing behavior. Do we know if anyone was relying on the existing behavior? |
Based on the GitHub issue and discussions: basically the app that raised this issue relies on this bug to be able to disable canvas-decorations-only when reading a viewport. However, this bug-reliance does not work if more than one viewport exists, so it's not a good behavior to rely on anyway. The bug only happens in the single viewport case. The idea here was to fix the bug so they no longer have to work around the bug by calling internal API and messing around with our direct-to-screen logic from their app. Then, we provide them with a way to truly disable canvas decorations as they desired. |
If they truly only care about canvas decorations (I think there's some confusion about what is a canvas decoration and what is not), a dead-simple solution = interface ReadImageToCanvasOptions {
includeCanvasDecorations?: boolean;
}
class Viewport {
public readImageToCanvas(options?: ReadImageToCanvasOptions): HTMLCanvasElement { ... }
}If I would be reluctant to provide a |
@ChrisBAtBentley @jason-crow Can you verify our understanding? |
andremig-bentley
changed the title
|
The changes necessary for the @jason-crow can you confirm whether the simple solution outlined in this comment above will satisfy what you need? Or, do you think you need something with more control over the decorations akin to the view flags solution (although not the view flags solution as discussed above)? Let me know if you have any other questions. |
|
@markschlosseratbentley @pmconne apologies for the delay.... The proposed solution for allowing a prop to specify whether to include decorations: interface ReadImageToCanvasOptions {
includeCanvasDecorations?: boolean;
}
class Viewport {
public readImageToCanvas(options?: ReadImageToCanvasOptions): HTMLCanvasElement { ... }
}Is a perfect solution for us. We do not actually want to turn off decorations in the viewport, we just want a way to capture a screenshot of the viewport without them using readImageToCanvas. This is preferred for us since a saved view has no mechanism to restore the forms' markers that were possibly active when the screenshot was taken, so ideally the saved view thumbnail would keep them hidden so it doesn't give the user a false expectation about how it will appear when applied. Since there was a bug that prevented the decorations from showing when a single viewport was active, versus 2, I don't really care if the default is for includeCanvasDecorations to be off or on, since I suppose either way there is a change in behavior; however, the suggested solution of making includeCanvasDecorations off by default probably preserves the most since most consumers likely work with a single viewport. Thanks for the work on this! |
andremig-bentley
changed the title
|
As of the latest commit, this PR now includes the proposed |
DTA docs have been updated to include the new include decorations functionality on the existing save image tool |
A NextVersion.md entry has been added. |
…s-core into andremig/decoration
core/frontend/src/Viewport.ts
Outdated
| */ | ||
| export interface ReadImageToCanvasOptions { | ||
| /** If defined, canvas decorations will be included in the saved image. | ||
| * @note This only affects single viewport applications. For multi-viewport applications, the canvas decorations are always included. |
There was a problem hiding this comment.
No. Make the currently inconsistent behavior consistent.
There was a problem hiding this comment.
Do you mean when includeCanvasDecorations is undefined, remove decorations for both single and multi viewport applications in the saved image?
There was a problem hiding this comment.
If I say { includeCanvasDecorations: false }, the canvas decorations should be omitted, regardless of how many viewports I have open. Your @note says they will be included even if I tell you to omit them, if more than one viewport is open.
There was a problem hiding this comment.
Is your note wrong? The comment above it is wrong (it claims any value other than undefined will cause the decorations to be included, but the property is a boolean).
There was a problem hiding this comment.
The comment above it is wrong, should say if true they will be included. The note was correct with the current behavior - basically, if includeCanvasDecorations !== true then current behavior will persist, this meaning if one viewport, no decorations, if multiple viewports, decorations included. You're right that this is inconsistent, working on changing it to:
if includeCanvasDecorations === true, then canvas decorations will be included, regardless of number of viewports. Otherwise, no canvas decorations will be included, regardless of number of viewports.
There was a problem hiding this comment.
My recommendation is to make includeCanvasDecorations a non-optional property. The new options argument to readImageToCanvas is necessarily optional. So if you don't supply any options, you get the current dumb behavior for backwards compatibility. If you supply the options, you must choose whether or not to include canvas decorations. Then you only need to document the dumb "how many viewports" logic in one place (on readImageToCanvas).
There was a problem hiding this comment.
Probably better:
/** [document dumb behavior here].
* @deprecated in 5.0 Use the overload accepting a ReadImageToCanvasOptions.
*/
public readImageToCanvas(): HTMLCanvasElement;
/** [document sane behavior here] */
public readImageToCanvas(options: ReadImageToCanvasOptions): HTMLCanvasElement;
/** @internal */
public readImageToCanvas(options: ReadImageToCanvasOptions | undefined): HTMLCanvasElement {
// implementation goes here.
}Then you can make includeCanvasDecorations optional and give it a sane default behavior (include the decorations by default, in which case you should probably invert its name to omitCanvasDecorations since undefined is falsy), and we'll be able to remove the dumb behavior eventually.
…sistent behavior for multi vps
| }); | ||
|
|
||
| it("should not include canvas decorations if omitCanvasDecorations is true", () => { | ||
| const vp = createViewport(); |
There was a problem hiding this comment.
The goal of these tests is to: Open a viewport, place a canvas decoration, use readImageToCanvas to get a copied canvas, check if the decoration is still there. This works and passes when the decoration is supposed to be included in the saved image (when omitCanvasDecorations is false). I am looking for feedback for the case in which the decorations should not be included in the saved image.
That result is relying on the fact that we render directly to the webgl canvas for a single viewport, and canvas decorations are placed on an overlaying second canvas. For decorations to be included in the saved image, the overlaying canvas must be combined with the webgl canvas. When testing this, despite no other viewports present and no combination taking place (confirmed through debug), the decorations remain present in this case, meaning the test is not using the standard single viewport behavior.
Any suggestions on forcing the test to use the direct render to screen methodology? Other things I've tried are written out in a comment in the code, but I'll add them here as well:
* Use openBlankViewport to get a blank viewport instead of creating a screen viewport directly
* Set vp.rendersToScreen to true before calling vp.renderFrame (for both vp types)
* Use vp.target.SetRenderToScreen to do the same thing
* Making sure all Viewports are removed before adding a new one at the beginning of the test
* Fully shutting down IModel App and then restarting it at the beginning of the test
|
Still To Do:
|
This PR is to address this issue: https://github.com/iTwin/itwinjs-backlog/issues/1310
This PR has two main goals. First, to address the bug outlined in the above issue. Currently, when there is only a single viewport open and that canvas is copied, such as with
copyImageToCanvas, canvas decorations do not show up on the resulting image. This is because the single view is rendered onto an on-screen webgl canvas for higher performance, while the canvas decorations were rendered onto a separate 2d canvas then laid on top. When saving, only the webgl canvas was being copied. This bug is fixed by combining the two canvases before saving them in this single viewport case.Secondly, the issue above lays out a desire for a way to control whether or not decorations are in the image saved through
Viewport.ReadImageToCanvas. This is accomplished through the addition ofReadImageToCanvasOptions. If undefined, or ifReadImageToCanvasOptions.includeCanvasDecorationsisfalse, then the original behavior will persist. IfincludeCanvasDecorationsis set to true, then the saved image will include the canvas decorations.Also included in this PR is the
SaveImageWithCanvasDecorations Toolfor Display Test App. This tool gives an example of how to use the newReadImageToCanvasOptionsby allowing you to save an image of the canvas with the decorations either on or off.