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 all 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 @@ -8235,7 +8235,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 @@ -8319,7 +8319,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 @@ -9071,6 +9071,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 @@ -10356,7 +10361,7 @@ export abstract class RenderTarget implements Disposable, 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 @@ -11619,7 +11624,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 @@ -14759,7 +14764,7 @@ export abstract class Viewport implements Disposable, 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"
}
12 changes: 10 additions & 2 deletions core/frontend/src/Viewport.ts
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,14 @@ export interface ReadPixelsArgs {
excludedElements?: Iterable<Id64String>;
}

/** Arguments supplied to [[Viewport.readImageToCanvas]].
* @public
*/
export interface ReadImageToCanvasOptions {
/** If true, canvas decorations will be included in the saved image. */
omitCanvasDecorations: 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 @@ -2708,8 +2716,8 @@ export abstract class Viewport implements Disposable, 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 Disposable, 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
82 changes: 64 additions & 18 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, ScreenViewport, 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 @@ -157,6 +158,8 @@ export abstract class Target extends RenderTarget implements RenderTargetDebugCo
public displayNormalMaps = true;

public freezeRealityTiles = false;

protected _omitCanvasDecorations? = false;
public get shadowFrustum(): Frustum | undefined {
const map = this.solarShadowMap;
return map.isEnabled && map.isReady ? map.frustum : undefined;
Expand Down Expand Up @@ -1177,15 +1180,56 @@ export abstract class Target extends RenderTarget implements RenderTargetDebugCo
return image;
}

public copyImageToCanvas(): HTMLCanvasElement {
private getCanvasFromImageBuffer(): 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);
return retCanvas;
}

private combineCanvasWithViewportOverlayCanvas(baseCanvas: HTMLCanvasElement, vp: ScreenViewport): HTMLCanvasElement {
const overlayCanvas = vp.canvas;
const ctx = baseCanvas.getContext("2d")!;
ctx.drawImage(overlayCanvas, 0, 0);
return baseCanvas;
}

private toggleOmitCanvasDecorations(hideDecorations: boolean, vp: ScreenViewport) {
this._omitCanvasDecorations = hideDecorations;
vp.invalidateScene();
}

public copyImageToCanvas(options?: ReadImageToCanvasOptions): HTMLCanvasElement {

/** There are four cases to consider:
* 1. Single viewport, with decorations: Must combine the webgl canvas with the 2d canvas, as canvas decorations are overlayed on top of the webgl canvas using the 2d canvas.
* 2. Single viewport, no decorations: Basic Copy image from image buffer. Decorations will be automaticaly excluded as they are overlayed on the separate 2d canvas.
* 3. Multiple viewports, with decorations: Basic copy image from image buffer. In this case, decorations will be included as they are not overlayed on a separate canvas
* when multiple viewports are present.
* 4. Multiple viewports, no decorations: Turn off decorations, copy image, then turn decorations back on.
*/

const vp = IModelApp.viewManager.selectedView;
if (!vp)
return document.createElement("canvas");

// case 1
if (vp.rendersToScreen && !options?.omitCanvasDecorations) {
return this.combineCanvasWithViewportOverlayCanvas(this.getCanvasFromImageBuffer(), vp);
}

// case 4
if (!vp.rendersToScreen && options?.omitCanvasDecorations) {
this.toggleOmitCanvasDecorations(true, vp);
const retCanvas = this.getCanvasFromImageBuffer();
this.toggleOmitCanvasDecorations(false, vp);
return retCanvas;
}

// cases 2 and 3
return this.getCanvasFromImageBuffer();
}

public drawPlanarClassifiers() {
if (this._planarClassifiers) {
this._planarClassifiers.forEach((classifier) => {
Expand Down Expand Up @@ -1468,16 +1512,18 @@ export class OnScreenTarget extends Target {
this._2dCanvas.needsClear = false;
}

const canvasDecs = this.graphics.canvasDecorations;
if (canvasDecs) {
for (const overlay of canvasDecs) {
ctx.save();
if (overlay.position)
ctx.translate(overlay.position.x, overlay.position.y);

overlay.drawDecoration(ctx);
this._2dCanvas.needsClear = true;
ctx.restore();
if (!this._omitCanvasDecorations) {
const canvasDecs = this.graphics.canvasDecorations;
if (canvasDecs) {
for (const overlay of canvasDecs) {
ctx.save();
if (overlay.position)
ctx.translate(overlay.position.x, overlay.position.y);

overlay.drawDecoration(ctx);
this._2dCanvas.needsClear = true;
ctx.restore();
}
}
}
}
Expand Down Expand Up @@ -1512,8 +1558,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 +1602,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
103 changes: 102 additions & 1 deletion core/frontend/src/test/Viewport.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {
AnalysisStyle, ColorDef, EmptyLocalization, Feature, ImageBuffer, ImageBufferFormat, ImageMapLayerSettings,
} from "@itwin/core-common";
import { ViewRect } from "../common/ViewRect";
import { OffScreenViewport, ScreenViewport, Viewport } from "../Viewport";
import { OffScreenViewport, ReadImageToCanvasOptions, ScreenViewport, Viewport } from "../Viewport";
import { DisplayStyle3dState } from "../DisplayStyleState";
import { SpatialViewState } from "../SpatialViewState";
import { IModelApp } from "../IModelApp";
Expand All @@ -21,6 +21,7 @@ import { Pixel } from "../render/Pixel";
import { GraphicType } from "../common/render/GraphicType";
import { RenderGraphic } from "../render/RenderGraphic";
import { Decorator } from "../ViewManager";
import { CanvasDecoration } from "../core-frontend";

describe("Viewport", () => {
beforeAll(async () => IModelApp.startup({ localization: new EmptyLocalization() }));
Expand Down Expand Up @@ -716,4 +717,104 @@ describe("Viewport", () => {
});
});
});

describe("Read Image To Canvas", () => {

class PixelCanvasDecoration implements CanvasDecoration {
public drawDecoration(ctx: CanvasRenderingContext2D) {
ctx.fillStyle = "red";
ctx.fillRect(0,0,1,1);
}
}

class PixelCanvasDecorator implements Decorator {
public decorate(context: DecorateContext) {

const builder = context.createGraphicBuilder(GraphicType.WorldDecoration, undefined);
builder.addLineString2d([new Point2d(0,0), new Point2d(1,1)],0);
context.addDecorationFromBuilder(builder);
context.addCanvasDecoration(new PixelCanvasDecoration());
}
}

function createViewport(): ScreenViewport {
const state = SpatialViewState.createBlank(createBlankConnection(), { x: 0, y: 0, z: 0 }, { x: 1, y: 1, z: 1 })
const parentDiv = document.createElement("div");
parentDiv.setAttribute("height", "100px");
parentDiv.setAttribute("width", "100px");
parentDiv.style.height = parentDiv.style.width = "100px";
document.body.appendChild(parentDiv);
return ScreenViewport.create(parentDiv, state);
}

const activeDecorators: Decorator[] = [];
function addDecorator(dec: Decorator) {
IModelApp.viewManager.dropDecorator(dec);
IModelApp.viewManager.addDecorator(dec);
activeDecorators.push(dec);
}

function getPixelRgb(pixel: Uint8ClampedArray): [number, number, number] {
return [pixel[3], pixel[2], pixel[1]];
}

afterEach(() => {
for (const dec of activeDecorators) {
IModelApp.viewManager.dropDecorator(dec);
}

activeDecorators.length = 0;
});

it("should not include canvas decorations if omitCanvasDecorations is true", () => {
const vp = createViewport();
Copy link
Contributor Author

@andremig-bentley andremig-bentley Jan 24, 2025

Choose a reason for hiding this comment

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

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

IModelApp.viewManager.addViewport(vp);

addDecorator(new PixelCanvasDecorator());
vp.renderFrame();

const readImageOptions: ReadImageToCanvasOptions = {
omitCanvasDecorations: true,
};

const canvas = vp.readImageToCanvas(readImageOptions);
const ctx = canvas.getContext("2d");
const pixel = ctx!.getImageData(0, 0, 1, 1).data;
const rgb = getPixelRgb(pixel);
expect(rgb).toEqual([0,0,0]);



IModelApp.viewManager.dropViewport(vp);
});

it("should include canvas decorations if omitCanvasDecorations is false", () => {
const vp = createViewport();
IModelApp.viewManager.addViewport(vp);

addDecorator(new PixelCanvasDecorator());
vp.renderFrame();

const readImageOptions: ReadImageToCanvasOptions = {
omitCanvasDecorations: false,
};

const canvas = vp.readImageToCanvas(readImageOptions);
const ctx = canvas.getContext("2d");
const pixel = ctx!.getImageData(0, 0, 1, 1).data;
const rgb = getPixelRgb(pixel);
expect(rgb).toEqual([255,0,0]);

IModelApp.viewManager.dropViewport(vp);
});

/**
* Other changes to the above tests which produced the same bug-free behavior (not what we want):
* 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
*/
});
});
8 changes: 8 additions & 0 deletions docs/changehistory/NextVersion.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ Table of contents:
- [Change to pullMerge](#change-to-pullmerge)
- [No pending/local changes](#no-pendinglocal-changes)
- [With pending/local changes](#with-pendinglocal-changes)
- [Graphics](#graphics)
- [Read Image To Canvas](#read-image-to-canvas)

## Selection set

Expand Down Expand Up @@ -286,3 +288,9 @@ This method offers several advantages:
4. In the future, this method will be essential for lock-less editing as it enables applications to merge changes with domain intelligence.

For more information read [Pull merge & conflict resolution](../learning/backend/PullMerge.md)

## Graphics

### Read Image To Canvas

Previously, when using [Viewport.readImageToCanvas]($core-frontend) with a single open viewport, canvas decorations were not included in the saved image. Sometimes this behavior was useful, so the [ReadImageToCanvasOptions]($core-frontend) interface was created to allow the option to choose whether or not canvas decorations are included in the saved image. Now, if [ReadImageToCanvasOptions.includeCanvasDecorations]($core-frontend) is true, canvas decorations will be included in the saved image. If undefined or false, previous behavior will persist and canvas decorations will not be included. All existing calls to [Viewport.readImageToCanvas]($core-frontend) will be unaffected by this change as the inclusion of [ReadImageToCanvasOptions]($core-frontend) is optional, and when they are undefined, previous behavior will persist.
Loading
Loading