Skip to content

CopyTexture Native rendering command #16497

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

Merged
merged 1 commit into from
Apr 16, 2025

Conversation

CedricGuillemet
Copy link
Contributor

@CedricGuillemet CedricGuillemet commented Apr 16, 2025

Copy texture as a rendering command. More explanations in BN PR.

@CedricGuillemet CedricGuillemet requested a review from bghgary April 16, 2025 08:48
@bjsplat
Copy link
Collaborator

bjsplat commented Apr 16, 2025

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.

@bjsplat
Copy link
Collaborator

bjsplat commented Apr 16, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Apr 16, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Apr 16, 2025

@CedricGuillemet CedricGuillemet changed the title CopyTexture Native rendering Command CopyTexture Native rendering command Apr 16, 2025
@sebavan sebavan merged commit 81d5eda into BabylonJS:master Apr 16, 2025
17 checks passed
@@ -1635,9 +1635,14 @@ export class NativeEngine extends Engine {
}

if (!!texture && !!texture._hardwareTexture) {
const context = canvas.getContext();
context.flush();
Copy link
Member

Choose a reason for hiding this comment

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

Is it really ok to do this synchronously right here? Couldn't queued commands still alter the state of the canvas/context? Should the flush itself be queued, even if the flush has to be a blocking operation on the native side?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As long as the flush (and nanovg rendering) happens before the copy, it will be fine. The queue is managed by NativeEngine so, the context/canvas won't be aware of the flush command in the queue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Couldn't queued commands still alter the state of the canvas/context?

I don't think so. Canvas and nanovg have their own queue that do not share objects with the engine. Main contact point is texture copy.

bghgary pushed a commit that referenced this pull request Apr 16, 2025
revert protocol increment introduced with
#16497
use with BabylonJS/BabylonNative#1503
ryantrem added a commit to BabylonJS/BabylonNative that referenced this pull request Apr 18, 2025
BJS PR ( ~~BabylonJS/Babylon.js#16497
BabylonJS/Babylon.js#16502 )needs to be merged
before and a new NPM needs to be generated.

Copy texture is now a queued command (see BJS PR), protocol changed.

Before: 

- canvas rendering was done with a task, in sync with frame rendering. 

Now:

- canvas rendering is done when when copying texture. Browser HTML
Canvas rendering happens when rendering commands are issued. With
Native, nanovg keeps its own queue that needs to be actually done before
the copy. This is done with the `flush` command.
- Because of this 'flush when needed', no need to keep a dirty flag in
the context.
- In order to always get the same context with `Canvas.getContext` some
code from `CanvasTest` branch has been integrated in this PR

### Test

add this to experience.js. Quad gets black when texture is disposed.

```

    var ground = BABYLON.MeshBuilder.CreateGround("ground1", { width: 0.5, height: 0.5, subdivisions: 2 }, scene);
    ground.rotation.x = -Math.PI * 0.5;
    ground.rotation.y = Math.PI;

    var texSize = 512;
    var dynamicTexture = new BABYLON.DynamicTexture("dynamic texture", texSize, scene);
    dynamicTexture.clear();
    var context = dynamicTexture.getContext();


    var materialGround = new BABYLON.StandardMaterial("Mat", scene);
    materialGround.diffuseTexture = dynamicTexture;
    ground.material = materialGround;
    materialGround.backFaceCulling = false;

    context.save();
    context.fillStyle = "DarkRed";
    context.fillRect(0, 0, texSize, texSize);
    context.restore();

    dynamicTexture.update();
    setTimeout(() => {
        console.log("update-dispose");
        dynamicTexture.update();
        dynamicTexture.dispose();
    }, 5000);
```
### Testing Issues 

- [x] `Dynamic Texture context clip` : race condition. shape is rendered
when using a breakpoint
~~- [ ] `GLTF Extension KHR_materials_volume with attenuation` :
rendering is a bit different but it doesn't seem to be related to
Canvas/copytexture. To confirm~~ wrong material.js
- [x] `GLTF Material Alpha Mask`: inconsistent result. Might be related
to race condition mentioned above.

---------

Co-authored-by: Ryan Tremblay <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants