DumpData: Add Native override #17365

alexchuber · 2025-10-29T17:40:53Z

This PR adds the glue code to enable DumpData usage in BN.

Changes:

Factor out core image encoding to a separate function matching NativeEncoding's own exposed function name. This follows the current nativeOverride pattern for module-level functions (see the MathHelper class.)
Remove the bitmaprenderer path. I initially planned for Native to polyfill this, but that turned out to be not a good idea.

…ot assignable to type '(pixelData: Uint8Array<ArrayBufferLike>, width: number, height: number, mimeType: string, invertY: boolean, quality?: number | undefined) => Promise<...>'. Type 'unknown' is not assignable to type 'Promise<ArrayBuffer>'.

bjsplat · 2025-10-29T17:42:25Z

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

alexchuber · 2025-10-29T17:43:49Z

packages/dev/core/src/Misc/dumpTools.ts

+                        reject(new Error("DumpData: Failed to convert canvas to blob."));
+                    } else if (blob.type !== mimeType) {
+                        // The MIME type of a canvas.toBlob() output can be different from the one requested if the browser does not support the requested format
+                        reject(new Error(`DumpData: Failed to convert canvas to blob as ${mimeType}. Got ${blob.type} instead.`));


This is a breaking change, but I think it makes sense to do, considering the API contract is to just return the binary data-- with the expectation that it matches the requested MIME type.

This change would also enable the changes proposed in #17366.

But there's one case that I'm not sure how to handle: when DumpData is used just to download an image, and the user doesn't care about the return type. I think ScreenshotTools is an example of this.
How should this be handled?

This is a breaking change, but I think it makes sense to do, considering the API contract is to just return the binary data-- with the expectation that it matches the requested MIME type.

I would say this is a bug fix so it's okay.

But there's one case that I'm not sure how to handle: when DumpData is used just to download an image, and the user doesn't care about the return type. I think ScreenshotTools is an example of this.
How should this be handled?

Maybe we can have an optional mimeType and we select the best one in the implementation?

I would say this is a bug fix so it's okay.

OK, I'll leave this be. Thanks.

Maybe we can have an optional mimeType and we select the best one in the implementation?

By implementation, do you mean the implementation of ScreenshotTools? If so, it already does something like this, defaulting to PNG if no mime type is supplied.

I was thinking in this function since that was my interpretation of your question. Maybe we can sync offline quickly.

bjsplat · 2025-10-29T17:57:12Z

Snapshot stored with reference name:
refs/pull/17365/merge

Test environment:
https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/17365/merge/index.html

To test a playground add it to the URL, for example:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/17365/merge/index.html#WGZLGJ#4600

Links to test babylon tools with this snapshot:

https://playground.babylonjs.com/?snapshot=refs/pull/17365/merge
https://sandbox.babylonjs.com/?snapshot=refs/pull/17365/merge
https://gui.babylonjs.com/?snapshot=refs/pull/17365/merge
https://nme.babylonjs.com/?snapshot=refs/pull/17365/merge

To test the snapshot in the playground with a playground ID add it after the snapshot query string:

https://playground.babylonjs.com/?snapshot=refs/pull/17365/merge#BCU1XR#0

bjsplat · 2025-10-29T18:02:41Z

WebGL2 visualization test reporter:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/17365/merge/testResults/webgl2playwright/index.html

bjsplat · 2025-10-29T18:04:30Z

Visualization tests for WebGPU

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/17365/merge/testResults/webgpuplaywright/index.html

bjsplat · 2025-10-29T18:12:10Z

Interaction tests

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/17365/merge/testResults/interactionplaywright/index.html

alexchuber · 2025-10-29T21:07:50Z

packages/dev/core/src/Misc/dumpTools.ts

+     * @internal
+     */
+    @nativeOverride
+    public static async EncodeImageAsync(pixelData: Uint8Array, width: number, height: number, mimeType: string, invertY: boolean, quality?: number): Promise<ArrayBuffer> {


As of now, it seems the two approaches to using nativeOverride for module-level function are:

what's here now: a helper class with static-only functions

export const EncodeImageAsync = nativeOverride(async (....) => { ... })

Is this correct? @ryantrem

I don't think the second bullet is a thing as far as I recall and from a quick glance at the code. I think we could make something like that work though, and avoid introducing a new class.

Gotcha. Mainly wanted to check if any existing approaches could work. I’ll stick with the class approach for now to keep the PR focused :)

bghgary · 2025-10-29T21:09:33Z

packages/dev/core/src/Misc/dumpTools.ts

+                        reject(new Error("DumpData: Failed to convert canvas to blob."));
+                    } else if (blob.type !== mimeType) {
+                        // The MIME type of a canvas.toBlob() output can be different from the one requested if the browser does not support the requested format
+                        reject(new Error(`DumpData: Failed to convert canvas to blob as ${mimeType}. Got ${blob.type} instead.`));


This is a breaking change, but I think it makes sense to do, considering the API contract is to just return the binary data-- with the expectation that it matches the requested MIME type.

I would say this is a bug fix so it's okay.

But there's one case that I'm not sure how to handle: when DumpData is used just to download an image, and the user doesn't care about the return type. I think ScreenshotTools is an example of this.
How should this be handled?

Maybe we can have an optional mimeType and we select the best one in the implementation?

bghgary · 2025-10-29T21:11:33Z

packages/dev/core/src/Misc/dumpTools.ts

    }

-    const resources = await _GetDumpResourcesAsync();
+    const buffer = await EncodingHelper.EncodeImageAsync(data as Uint8Array, width, height, mimeType, invertY, quality);


Is it valid to convert ArrayBufferView to Uint8Array? I'm not sure this will work in all cases.

Definitely not valid :) I meant to think about this more.

On the one hand, DumpData's usage of engine.createRawTexture implies that the data is expected to be UNSIGNED_BYTE.
On the other hand, I have no clue why this hasn't been a problem yet. My guess is that DumpData is usually used in conjunction with readPixels, which returns either float array or uint8 array

packages/dev/core/src/Misc/dumpTools.ts

ryantrem · 2025-10-30T15:49:32Z

packages/dev/core/src/Misc/decorators.ts

    target: any,
    propertyKey: string,
-    descriptor: TypedPropertyDescriptor<(...params: Parameters<T>) => unknown>,
+    descriptor: TypedPropertyDescriptor<(...params: Parameters<T>) => any>,


Just curious why this change was needed?

To address this guy:

without touching this function too much.

Is there a more elegant way?

ryantrem · 2025-10-30T15:56:41Z

packages/dev/core/src/Misc/dumpTools.ts

+     * @internal
+     */
+    @nativeOverride
+    public static async EncodeImageAsync(pixelData: Uint8Array, width: number, height: number, mimeType: string, invertY: boolean, quality?: number): Promise<ArrayBuffer> {


I don't think the second bullet is a thing as far as I recall and from a quick glance at the code. I think we could make something like that work though, and avoid introducing a new class.

ryantrem · 2025-10-30T15:58:07Z

packages/dev/core/src/Misc/dumpTools.ts

+
+            texture.dispose();
+
+            Tools.ToBlob(


Should we add a ToBlobAsync that is promise returning?

We could. It wouldn't be as simple as replacing with ToBlob with ToBlobAsync, though, as the canvas draw + toBlob must be atomic. Guess we could use an AsyncLock?

alexchuber added 5 commits October 29, 2025 01:53

Remove bitmaprenderer

ffa3e0f

Experimentation with native implementation

6a82e50

Decided on Promise<ArrayBuffer>

7292799

Just use nativeOverride method decorator

0774434

alexchuber commented Oct 29, 2025

View reviewed changes

alexchuber mentioned this pull request Oct 29, 2025

glTF Exporter: Handle unsupported encode formats at platform level #17366

Draft

alexchuber requested a review from bghgary October 29, 2025 20:53

bghgary requested a review from ryantrem October 29, 2025 20:55

alexchuber commented Oct 29, 2025

View reviewed changes

bghgary reviewed Oct 29, 2025

View reviewed changes

ryantrem reviewed Oct 30, 2025

View reviewed changes

DumpData: Add Native override #17365

Are you sure you want to change the base?

DumpData: Add Native override #17365

Conversation

alexchuber commented Oct 29, 2025

Uh oh!

bjsplat commented Oct 29, 2025

Uh oh!

alexchuber Oct 29, 2025 • 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!

alexchuber Oct 29, 2025 • 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!

bjsplat commented Oct 29, 2025

Uh oh!

bjsplat commented Oct 29, 2025

Uh oh!

bjsplat commented Oct 29, 2025

Uh oh!

bjsplat commented Oct 29, 2025

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alexchuber Oct 29, 2025 •

edited

Loading

alexchuber Oct 29, 2025 •

edited

Loading