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

Fix copyImageToCanvas bug and add ReadImageToCanvasOptions #7539

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
d10e008
init commit
andremig-bentley Dec 19, 2024
cb438a7
create canvasDecorations view flag, fix copy image bug
andremig-bentley Jan 10, 2025
b9d78c3
cleanup
andremig-bentley Jan 10, 2025
18694a8
Merge branch 'master' into andremig/decoration
andremig-bentley Jan 10, 2025
8e49ea3
rush change
andremig-bentley Jan 10, 2025
1066412
update rush change
andremig-bentley Jan 10, 2025
e39235e
Merge branch 'master' into andremig/decoration
andremig-bentley Jan 21, 2025
e38bfa3
rm view flag changes
andremig-bentley Jan 21, 2025
570759b
cleanup, extract-api
andremig-bentley Jan 21, 2025
5b86b52
Add ReadImageToCanvasOptions, SaveImageWithCanvasDecorations Tool
andremig-bentley Jan 22, 2025
a19ad85
Merge branch 'master' into andremig/decoration
andremig-bentley Jan 22, 2025
b3b93be
extract api
andremig-bentley Jan 22, 2025
0807157
rm SaveImageWithDecorations tool, update dta readme
andremig-bentley Jan 22, 2025
40fa886
Merge branch 'master' into andremig/decoration
andremig-bentley Jan 22, 2025
1231e41
cleanup
andremig-bentley Jan 22, 2025
d683c4d
Merge branch 'andremig/decoration' of https://github.com/iTwin/itwinj…
andremig-bentley Jan 22, 2025
ae3eac8
NextVersion
andremig-bentley Jan 23, 2025
b600995
Merge branch 'master' into andremig/decoration
andremig-bentley Jan 23, 2025
802c215
next version cleanup
andremig-bentley Jan 23, 2025
42b90c9
Merge branch 'andremig/decoration' of https://github.com/iTwin/itwinj…
andremig-bentley Jan 23, 2025
e593f01
Merge branch 'master' into andremig/decoration
andremig-bentley Jan 24, 2025
6b71824
test progress, includeCanvasDecorations -> omitCanvasDecorations, con…
andremig-bentley Jan 24, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions common/api/core-frontend.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -8230,7 +8230,7 @@ export class OffScreenTarget extends Target {
// (undocumented)
onResized(): void;
// (undocumented)
readImageToCanvas(): HTMLCanvasElement;
readImageToCanvas(options?: ReadImageToCanvasOptions): HTMLCanvasElement;
// (undocumented)
setViewRect(rect: ViewRect, temporary: boolean): void;
// (undocumented)
Expand Down Expand Up @@ -8314,7 +8314,7 @@ export class OnScreenTarget extends Target {
// (undocumented)
pickOverlayDecoration(pt: XAndY): CanvasDecoration | undefined;
// (undocumented)
readImageToCanvas(): HTMLCanvasElement;
readImageToCanvas(options?: ReadImageToCanvasOptions): HTMLCanvasElement;
// (undocumented)
setRenderToScreen(toScreen: boolean): HTMLCanvasElement | undefined;
// (undocumented)
Expand Down Expand Up @@ -9066,6 +9066,11 @@ export interface ReadImageBufferArgs {
upsideDown?: boolean;
}

// @public
export interface ReadImageToCanvasOptions {
includeCanvasDecorations?: boolean;
}

// @internal (undocumented)
export function readImdlContent(args: ImdlReaderCreateArgs & {
parseDocument?: (parseOpts: ImdlParserOptions) => Promise<ImdlModel.Document | ImdlParseError>;
Expand Down Expand Up @@ -10347,7 +10352,7 @@ export abstract class RenderTarget implements IDisposable, RenderMemory.Consumer
// @internal (undocumented)
readImageBuffer(_args?: ReadImageBufferArgs): ImageBuffer | undefined;
// @internal (undocumented)
readImageToCanvas(): HTMLCanvasElement;
readImageToCanvas(_options?: ReadImageToCanvasOptions): HTMLCanvasElement;
// @internal (undocumented)
abstract readPixels(rect: ViewRect, selector: Pixel.Selector, receiver: Pixel.Receiver, excludeNonLocatable: boolean, excludedElements?: Iterable<Id64String>): void;
// @internal (undocumented)
Expand Down Expand Up @@ -11608,7 +11613,7 @@ export abstract class Target extends RenderTarget implements RenderTargetDebugCo
// (undocumented)
computeEdgeWeight(pass: RenderPass, baseWeight: number): number;
// (undocumented)
copyImageToCanvas(): HTMLCanvasElement;
copyImageToCanvas(options?: ReadImageToCanvasOptions): HTMLCanvasElement;
// (undocumented)
createPlanarClassifier(properties?: ActiveSpatialClassifier): PlanarClassifier;
// (undocumented)
Expand Down Expand Up @@ -14744,7 +14749,7 @@ export abstract class Viewport implements IDisposable, TileUser {
// @deprecated
readImage(rect?: ViewRect, targetSize?: Point2d, flipVertically?: boolean): ImageBuffer | undefined;
readImageBuffer(args?: ReadImageBufferArgs): ImageBuffer | undefined;
readImageToCanvas(): HTMLCanvasElement;
readImageToCanvas(options?: ReadImageToCanvasOptions): HTMLCanvasElement;
readPixels(rect: ViewRect, selector: Pixel.Selector, receiver: Pixel.Receiver, excludeNonLocatable?: boolean): void;
readPixels(args: ReadPixelsArgs): void;
// @internal
Expand Down
1 change: 1 addition & 0 deletions common/api/summary/core-frontend.exports.csv
Original file line number Diff line number Diff line change
Expand Up @@ -582,6 +582,7 @@ public;function;readGltfGraphics
public;interface;ReadGltfGraphicsArgs
beta;function;readGltfTemplate
public;interface;ReadImageBufferArgs
public;interface;ReadImageToCanvasOptions
internal;function;readImdlContent
public;interface;ReadMeshArgs
internal;class;ReadonlyTileUserSet
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"changes": [
{
"packageName": "@itwin/core-frontend",
"comment": "Add ReadImageToCanvasOptions",
"type": "none"
}
],
"packageName": "@itwin/core-frontend"
}
14 changes: 12 additions & 2 deletions core/frontend/src/Viewport.ts
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,16 @@ export interface ReadPixelsArgs {
excludedElements?: Iterable<Id64String>;
}

/** Arguments supplied to [[Viewport.readImageToCanvas]].
* @public
*/
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.
Copy link
Member

Choose a reason for hiding this comment

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

No. Make the currently inconsistent behavior consistent.

Copy link
Contributor Author

@andremig-bentley andremig-bentley Jan 23, 2025

Choose a reason for hiding this comment

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

Do you mean when includeCanvasDecorations is undefined, remove decorations for both single and multi viewport applications in the saved image?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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).

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 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.

Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

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.

*/
includeCanvasDecorations?: boolean;
}

/** A Viewport renders the contents of one or more [GeometricModel]($backend)s onto an `HTMLCanvasElement`.
*
* It holds a [[ViewState]] object that defines its viewing parameters; the ViewState in turn defines the [[DisplayStyleState]],
Expand Down Expand Up @@ -2703,8 +2713,8 @@ export abstract class Viewport implements IDisposable, TileUser {
/** Reads the current image from this viewport into an HTMLCanvasElement with a Canvas2dRenderingContext such that additional 2d graphics can be drawn onto it.
* @see [[readImageBuffer]] to obtain the image as an array of RGBA pixels.
*/
public readImageToCanvas(): HTMLCanvasElement {
return this.target.readImageToCanvas();
public readImageToCanvas(options?: ReadImageToCanvasOptions): HTMLCanvasElement {
return this.target.readImageToCanvas(options);
}

/** Used internally by `waitForSceneCompletion`.
Expand Down
4 changes: 2 additions & 2 deletions core/frontend/src/render/RenderTarget.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { Point2d, XAndY } from "@itwin/core-geometry";
import { IModelConnection } from "../IModelConnection";
import { HiliteSet } from "../SelectionSet";
import { SceneContext } from "../ViewContext";
import { ReadImageBufferArgs, Viewport } from "../Viewport";
import { ReadImageBufferArgs, ReadImageToCanvasOptions, Viewport } from "../Viewport";
import { ViewRect } from "../common/ViewRect";
import { CanvasDecoration } from "./CanvasDecoration";
import { Decorations } from "./Decorations";
Expand Down Expand Up @@ -176,7 +176,7 @@ export abstract class RenderTarget implements IDisposable, RenderMemory.Consumer
/** @internal */
public readImageBuffer(_args?: ReadImageBufferArgs): ImageBuffer | undefined { return undefined; }
/** @internal */
public readImageToCanvas(): HTMLCanvasElement { return document.createElement("canvas"); }
public readImageToCanvas(_options?: ReadImageToCanvasOptions): HTMLCanvasElement { return document.createElement("canvas"); }
/** @internal */
public collectStatistics(_stats: RenderMemory.Statistics): void { }

Expand Down
25 changes: 19 additions & 6 deletions core/frontend/src/render/webgl/Target.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import { ViewRect } from "../../common/ViewRect";
import { canvasToImageBuffer, canvasToResizedCanvasWithBars, imageBufferToCanvas } from "../../common/ImageUtil";
import { HiliteSet, ModelSubCategoryHiliteMode } from "../../SelectionSet";
import { SceneContext } from "../../ViewContext";
import { ReadImageBufferArgs, Viewport } from "../../Viewport";
import { ReadImageBufferArgs, ReadImageToCanvasOptions, Viewport } from "../../Viewport";
import { IModelConnection } from "../../IModelConnection";
import { CanvasDecoration } from "../CanvasDecoration";
import { Decorations } from "../Decorations";
Expand Down Expand Up @@ -64,6 +64,7 @@ import { FrameStatsCollector } from "../FrameStats";
import { ActiveSpatialClassifier } from "../../SpatialClassifiersState";
import { AnimationNodeId } from "../../common/internal/render/AnimationNodeId";
import { _implementationProhibited } from "../../common/internal/Symbols";
import { IModelApp } from "../../IModelApp";

function swapImageByte(image: ImageBuffer, i0: number, i1: number) {
const tmp = image.data[i0];
Expand Down Expand Up @@ -1177,12 +1178,24 @@ export abstract class Target extends RenderTarget implements RenderTargetDebugCo
return image;
}

public copyImageToCanvas(): HTMLCanvasElement {
public copyImageToCanvas(options?: ReadImageToCanvasOptions): HTMLCanvasElement {
const image = this.readImageBuffer();
const canvas = undefined !== image ? imageBufferToCanvas(image, false) : undefined;
const retCanvas = undefined !== canvas ? canvas : document.createElement("canvas");
const pixelRatio = this.devicePixelRatio;
retCanvas.getContext("2d")!.scale(pixelRatio, pixelRatio);

const vp = IModelApp.viewManager.selectedView;
// vp.rendersToScreen tells us if the view is being rendered directly to the webgl canvas on screen, which happens in the case of a single viewport.
// In that case, we need to combine the 2d canvas with the webgl canvas or we will lose canvas decorations in the copied image.
// options.includeCanvasDecorations will be true if the caller wants to include the decorations in the copied image.
if (vp && vp.rendersToScreen && options?.includeCanvasDecorations) {
const twoDCanvas = vp.canvas;
const ctx = retCanvas.getContext("2d")!;
ctx.drawImage(twoDCanvas, 0, 0);
}


return retCanvas;
}

Expand Down Expand Up @@ -1512,8 +1525,8 @@ export class OnScreenTarget extends Target {
return toScreen ? this._webglCanvas.canvas : undefined;
}

public override readImageToCanvas(): HTMLCanvasElement {
return this._usingWebGLCanvas ? this.copyImageToCanvas() : this._2dCanvas.canvas;
public override readImageToCanvas(options?: ReadImageToCanvasOptions): HTMLCanvasElement {
return this._usingWebGLCanvas ? this.copyImageToCanvas(options) : this._2dCanvas.canvas;
}
}

Expand Down Expand Up @@ -1556,8 +1569,8 @@ export class OffScreenTarget extends Target {
this.renderSystem.frameBufferStack.pop();
}

public override readImageToCanvas(): HTMLCanvasElement {
return this.copyImageToCanvas();
public override readImageToCanvas(options?: ReadImageToCanvasOptions): HTMLCanvasElement {
return this.copyImageToCanvas(options);
}
}

Expand Down
3 changes: 3 additions & 0 deletions test-apps/display-test-app/public/locales/en/SVTTools.json
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,9 @@
"SaveImage": {
"keyin": "dta save image"
},
"SaveImageWithCanvasDecorations": {
eringram marked this conversation as resolved.
Show resolved Hide resolved
"keyin": "dta save image with decorations"
},
"OutputShaders": {
"keyin": "dta output shaders"
},
Expand Down
3 changes: 2 additions & 1 deletion test-apps/display-test-app/src/frontend/App.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ import { MarkupTool, ModelClipTool, ZoomToSelectedElementsTool } from "./Viewer"
import { MacroTool } from "./MacroTools";
import { RecordTileSizesTool } from "./TileSizeRecorder";
import { TerrainDrapeTool } from "./TerrainDrapeTool";
import { SaveImageTool } from "./SaveImageTool";
import { SaveImageTool, SaveImageWithCanvasDecorationsTool } from "./SaveImageTool";
import { ToggleSecondaryIModelTool } from "./TiledGraphics";
import { BingTerrainMeshProvider } from "./BingTerrainProvider";
import { AttachCustomRealityDataTool, registerRealityDataSourceProvider } from "./RealityDataProvider";
Expand Down Expand Up @@ -358,6 +358,7 @@ export class DisplayTestApp {
ResizeWindowTool,
RestoreWindowTool,
SaveImageTool,
SaveImageWithCanvasDecorationsTool,
ShutDownTool,
SignInTool,
SignOutTool,
Expand Down
50 changes: 48 additions & 2 deletions test-apps/display-test-app/src/frontend/SaveImageTool.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@

import { ProcessDetector } from "@itwin/core-bentley";
import { Point2d } from "@itwin/core-geometry";
import { imageBufferToPngDataUrl, IModelApp, openImageDataUrlInNewWindow, Tool } from "@itwin/core-frontend";
import { parseArgs } from "@itwin/frontend-devtools";
import { imageBufferToPngDataUrl, IModelApp, openImageDataUrlInNewWindow, ReadImageToCanvasOptions, Tool } from "@itwin/core-frontend";
import { parseArgs, parseToggle } from "@itwin/frontend-devtools";

interface SaveImageOptions {
copyToClipboard?: boolean;
Expand Down Expand Up @@ -89,3 +89,49 @@ export class SaveImageTool extends Tool {
return this.run(opts);
}
}

export class SaveImageWithCanvasDecorationsTool extends Tool {
public static override toolId = "SaveImageWithCanvasDecorations";
public static override get minArgs() { return 0; }
public static override get maxArgs() { return 1; }

public override async run(showDecorations: boolean): Promise<boolean> {
const vp = IModelApp.viewManager.selectedView;
if (!vp)
return false;

const width = vp.viewRect.width;
const height = vp.viewRect.height;

await vp.waitForSceneCompletion();
const buffer = vp.readImageBuffer({ size: new Point2d(width, height) });
if (!buffer) {
alert("Failed to read image");
return true;
}

const options: ReadImageToCanvasOptions = {
includeCanvasDecorations: showDecorations,
}

const canvas = vp.readImageToCanvas(options);
const url = canvas.toDataURL();

if (!url) {
alert("Failed to produce PNG");
return true;
}

openImageDataUrlInNewWindow(url, "Saved View");
return true;
}

public override async parseAndRun(...args: string[]): Promise<boolean> {
let showDecorations;
if (!args[0])
showDecorations = true;
else
showDecorations = !!parseToggle(args[0]);
return this.run(showDecorations);
}
}
Loading