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

Introduce FramebufferTexture. #22916

Merged
merged 1 commit into from Nov 29, 2021
Merged

Introduce FramebufferTexture. #22916

merged 1 commit into from Nov 29, 2021

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Nov 29, 2021

Related issue: -

Description

The current usage of WebGLRenderer.copyFramebufferToTexture() is incorrect and also incompatible with gl.texStorage2D().

When copying data from the current bound framebuffer to a texture, the texture itself does not need any data definitions. So no call of gl.texImage2D() is required. It's sufficient to bind the texture and then call gl.copyTexImage2D().

gl.copyTexImage2D() is incompatible with immutable textures so calling gl.texStorage2D() is not required and wouldn't work anyway (since gl.texStorage2D() defines immutable texture storage).

Since it is not possible right now to properly process a texture in WebGLTextures which was updated via WebGLRenderer.copyFramebufferToTexture(), a new type of texture (FramebufferTexture) is introduced.

@mrdoob
Copy link
Owner

mrdoob commented Nov 29, 2021

Very nice!

@mrdoob mrdoob added this to the r136 milestone Nov 29, 2021
@mrdoob mrdoob merged commit c748762 into mrdoob:dev Nov 29, 2021
@mrdoob
Copy link
Owner

mrdoob commented Nov 29, 2021

Thanks!

material.roughnessMap.repeat.copy( roughnessMap.repeat );
material.roughnessMap.center.copy( roughnessMap.center );
material.roughnessMap.rotation = roughnessMap.rotation;
material.roughnessMap.image = roughnessMap.image;
Copy link
Contributor

Choose a reason for hiding this comment

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

@Mugen87 I just noticed that this line got removed (I think accidentally?). It's from #22741 and I believe it's still necessary.

Copy link
Collaborator Author

@Mugen87 Mugen87 Dec 9, 2021

Choose a reason for hiding this comment

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

I removed it since the image property from the original roughness texture should not overwrite the image property of a framebuffer texture. Both are actually different.

Sorry, I wasn't aware that my original solution did still need it. I've thought the instance of FramebufferTexture is just the target that holds the data from the framebuffer but is never used as a source texture in RoughnessMipmapper.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, if you look at the linked PR, the issue is when you go to export the scene. USDZExporter uses the original image, and even though we're tweaking the roughness here to account for the normal map, you want the original roughness map to go through. Also, since this only has width and height, it just fails to export since it can't actually find an image.

Copy link
Owner

Choose a reason for hiding this comment

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

We should probably add a comment in that line so we don't remove it again.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, let me file a PR that adds the line back.

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

3 participants