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

Stop checking isFramebufferTexture in copyFramebufferToTexture. #23964

Merged
merged 1 commit into from
May 6, 2022

Conversation

chubei-oppen
Copy link
Contributor

@chubei-oppen chubei-oppen commented Apr 29, 2022

Description

FramebufferTexture was first added in #22916 because copyTexImage2D doesn't work with texStorage2D. However, copyTexImage2D was later replaced by copyTexSubImage2D in #22985, making checking FramebufferTexture unnecessary.

However, FramebufferTexture is still useful in that we don't have to set an image to allocate texture memory.

This contribution is funded by OppenFuture Technologies

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 29, 2022

I wonder if it is a good idea to allow other texture types again. FramebufferTexture was introduced to have a dedicated class for extracting framebuffer data into a texture.

What would be the use case for using other texture types? Just because something is technically possible does not mean it has a value. So why would users require this change?

However, FramebufferTexture is still useful in that we don't have to set an image to allocate texture memory.

Forcing users into FramebufferTexture means they do not need an unnecessary image to allocate texture memory. I don't fell well with enabling workflow which does not have this advantage.

@chubei-oppen
Copy link
Contributor Author

chubei-oppen commented Apr 29, 2022

Serveral versions ago, there was a RoughnessMipmapper used for modifying roughness mipmaps to account for the normal variation when objects are rendered in distance. Before the introduction of FramebufferTexture, it does the modification in place. After that, it has to allocate a new texture for every material.

We're keeping this class in our project and I found that removing the check brings back the old more performant behaviour.

This is the only use case I know, though I can imagine other people wishing to copy framebuffer to exsiting textures which were not framebuffer textures when created (runtime modified environment maps for example) .

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 29, 2022

runtime modified environment maps for example

Sounds reasonable. Okay, let's remove the check. It shouldn't hurt anybody^^.

@Mugen87 Mugen87 added this to the r140 milestone Apr 29, 2022
@mrdoob mrdoob modified the milestones: r140, r141 Apr 30, 2022
@mrdoob mrdoob merged commit c298012 into mrdoob:dev May 6, 2022
@mrdoob
Copy link
Owner

mrdoob commented May 6, 2022

Thanks!

abernier pushed a commit to abernier/three.js that referenced this pull request Sep 16, 2022
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.

3 participants