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

PMREMGenerator: Fix replacing render targets with different heights #25301

Merged
merged 1 commit into from
Jan 19, 2023

Conversation

JordyvanDortmont
Copy link
Contributor

Related PR: #23322

Description

When calling fromEquirectangular (or _fromTexture in general) without a render target (default null) for a second time with a texture with a different height than the texture for the first call, but the same width, the _pingPongRenderTarget is not replaced. After which the lighting from setting the Scene.environment to the render target texture fails, in two different ways, depending on the order (larger or smaller height first/second).

@WestLangley
Copy link
Collaborator

/ping @elalish ... just checking... is this correct?

const width = 3 * Math.max( this._cubeSize, 16 * 7 );
const height = 4 * this._cubeSize;

@elalish
Copy link
Contributor

elalish commented Jan 18, 2023

@WestLangley indeed it is; it's much like the internal storage of a mipmapped cubemap, but with the addition of 7 16x16 cubemaps in a horizontal stripe at the top of the image for the higher roughness values. The max ensures there's enough room for them if the input cube is small.

This change looks good to me.

@WestLangley WestLangley added this to the r149 milestone Jan 18, 2023
@mrdoob mrdoob merged commit 6c86066 into mrdoob:dev Jan 19, 2023
netpro2k pushed a commit to Hubs-Foundation/three.js that referenced this pull request May 26, 2023
@JordyvanDortmont JordyvanDortmont deleted the fix-pmrem-generator branch July 29, 2023 11:02
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.

4 participants