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 reallocation of a depth render target in the SceneGrab class #5650

Merged
merged 8 commits into from
Sep 20, 2023

Conversation

querielo
Copy link
Contributor

There's a scenario where a camera has a custom render target without a depth buffer texture, and the SceneGrab class incorrectly assumes that the grabbed depth texture has to be the size of the device. This can cause issues when the color buffer of the render target and a canvas have different sizes.

To fix this issue, I suggest that the code should check the size of the color buffer in the absence of a depth buffer, since they should have the same size. This will ensure that the reallocation of SceneGrab.depthRenderTarget works as expected.

I hope this helps! Let me know if you have any questions.

I confirm I have read the contributing guidelines and signed the Contributor License Agreement.

@querielo querielo changed the title Fix reallocation of render targets in SceneGrab class Fix reallocation of a depth render target in the SceneGrab class Sep 20, 2023
@mvaligursky
Copy link
Contributor

If the render target does not have depth, we should probably just log error and early out / fail as what is the reason to execute depth grab if there is no depth buffer?

@querielo
Copy link
Contributor Author

querielo commented Sep 20, 2023

Hm. I like your suggestion.

But it just looks like there are two independent ways to grab depth buffers. "Depth grabpass" simplify usage "getLinearScreenDepth" or something like this. And a user can instantiate RenderTarget the next way:

const renderTarget = new pc.RenderTarget({
    colorBuffer: texture
    depth: true
});

In this case, renderTarget._depthBuffer === undefined

@mvaligursky
Copy link
Contributor

In this case, renderTarget._depthBuffer === undefined

Yep, that's a valid point!

Copy link
Contributor

@mvaligursky mvaligursky left a comment

Choose a reason for hiding this comment

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

Happy with this, thanks!

@mvaligursky mvaligursky merged commit 9eff302 into playcanvas:main Sep 20, 2023
7 checks passed
@querielo
Copy link
Contributor Author

Thanks!

I have met another case. A camera with posteffects + Depth grabpass + Render target

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants