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

Don't alter the scene's active cameras in the middle of taking a scre… #13040

Merged
merged 3 commits into from
Sep 30, 2022
Merged

Don't alter the scene's active cameras in the middle of taking a scre… #13040

merged 3 commits into from
Sep 30, 2022

Conversation

carolhmj
Copy link
Contributor

…enshot with RenderTargetTexture, because we can just set the RTT active camera instead.

Related to: https://forum.babylonjs.com/t/how-to-freeze-the-camera/34284/10 so we don't need to manually set the scene's active camera after taking a screenshot.

…enshot with RenderTargetTexture, because we can just set the RTT active camera instead.
@carolhmj carolhmj added the bug label Sep 28, 2022
@azure-pipelines
Copy link

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.

@RaananW
Copy link
Member

RaananW commented Sep 28, 2022

Just need to clean up the file (unused imports)

@sebavan sebavan enabled auto-merge September 28, 2022 18:46
@Popov72
Copy link
Contributor

Popov72 commented Sep 28, 2022

Unfortunately this change won't work because the list of meshes as seen by the screenshot camera is not refreshed correctly.

Try this PG: https://playground.babylonjs.com/#85EGZM#22

There is a sphere on the left but not visible by the main scene camera. When taking a screenshot (hit Spacebar and look at the browser console), you should see:

image

because the screenshot camera is put on the right of the box and is looking down the -x axis, so the sphere is visible.

With the PR, you won't see the sphere because the list of meshes that is used to render into the RTT is the list of active meshes of the main scene camera (so only the box in the PG). It is this line of code in RenderTargetTexture._renderToTarget which is doing that:

            const defaultRenderList = this.renderList ? this.renderList : scene.getActiveMeshes().data;

That's why the current screenshot code is replacing the scene camera by the screenshot camera and is doing scene.render() => this call is triggering a recomputation of the list of active meshes as seen by the current scene camera.

@Popov72 Popov72 disabled auto-merge September 28, 2022 21:25
@sebavan sebavan self-requested a review September 28, 2022 21:55
Copy link
Member

@sebavan sebavan left a comment

Choose a reason for hiding this comment

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

flag as request changes to prevent merge following @Popov72 comments

@carolhmj
Copy link
Contributor Author

Unfortunately this change won't work because the list of meshes as seen by the screenshot camera is not refreshed correctly.

Try this PG: https://playground.babylonjs.com/#85EGZM#22

There is a sphere on the left but not visible by the main scene camera. When taking a screenshot (hit Spacebar and look at the browser console), you should see:

image

because the screenshot camera is put on the right of the box and is looking down the -x axis, so the sphere is visible.

With the PR, you won't see the sphere because the list of meshes that is used to render into the RTT is the list of active meshes of the main scene camera (so only the box in the PG). It is this line of code in RenderTargetTexture._renderToTarget which is doing that:

            const defaultRenderList = this.renderList ? this.renderList : scene.getActiveMeshes().data;

That's why the current screenshot code is replacing the scene camera by the screenshot camera and is doing scene.render() => this call is triggering a recomputation of the list of active meshes as seen by the current scene camera.

In this case, could we add the scene's meshes explicitly to the RTT? Like this:

for (const node of scene.getNodes()) {
        if (node instanceof AbstractMesh) {
            texture.renderList?.push(node);
        }
    }

@Popov72
Copy link
Contributor

Popov72 commented Sep 29, 2022

Yes, I think that should do it.

However, you can do it in a simpler way:

texture.renderList = scene.meshes.slice();

The only thing is that frustum culling is not applied on those meshes, so they will be sent to the GPU even if they are not visible. I guess it's not really a problem for a screenshot. If it is, you will have to perform a loop instead and call mesh.isInFrustum(frustumPlanes) for each mesh before pushing it in the list (note: to get the planes to pass to isInFrustum(), you can do frustumPlanes = Frustum.GetPlanes(camera.getTransformationMatrix());).

@carolhmj carolhmj requested a review from sebavan September 29, 2022 20:28
@azure-pipelines
Copy link

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.

@azure-pipelines
Copy link

@sebavan sebavan merged commit 6040274 into BabylonJS:master Sep 30, 2022
@carolhmj carolhmj deleted the cameraScreenshot branch October 20, 2022 12:49
RaananW pushed a commit that referenced this pull request Dec 9, 2022
Don't alter the scene's active cameras in the middle of taking a scre…

Former-commit-id: 0749c6ed850b52ee56366604e5c52ca717d90140
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants