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

Add possibility to bind 3D textures and 2D textures array as color attachment to framebuffer #20111

Merged
merged 1 commit into from
Feb 6, 2021

Conversation

DavidPeicho
Copy link
Contributor

Hi,

This PR includes the changes:

  • WebGLLayeredRenderTarget ⟶ adds the possibility to render to 3d texture and 2d texture arrays.
  • Updates WebGLRenderer.setRenderTarget to choose the layer to which the rendering should occur
  • Adds a complete example of using WebGLLayeredRenderTarget. The example is simply based on the Texture 2D array example, and is mostly useful for debugging. In the future, I can replace it with something fancier.

Why?

Having the possibility to render to volume allows for instance to:

  • Generates volume on the GPU (think about Perlin noise, etc...)
  • Compute irrandiance cache
  • etc...

Improvements

There are a couple of things that aren't nice and look pretty dirty for now:

  • This is a complete hack. I think it would make more sense for a user to directly give the texture to the render target, but that would be an API break change. Alternatively, I guess we could do the texture in a method and the WebGLLayeredRenderTarget could call this function.
  • Same here, this feels like a hack. The issue is still hat WebGLRenderTarget is made to only accept Texture.
  • In the renderer, I used activeCubeFace as the layer selector. I believe it could makes sense maybe to have a separate function for cube / layered render target?

Let me know what you think. I know this is delicate because it touches to some of the internals.

@DavidPeicho
Copy link
Contributor Author

It would be better if I could fit the render target in the already existing WebGLRenderTarget instead of creating a new class though....

@mrdoob
Copy link
Owner

mrdoob commented Aug 24, 2020

It would be better if I could fit the render target in the already existing WebGLRenderTarget instead of creating a new class though....

Yes, I was thinking that.

I wonder if it would be possible to just have a single WebGLRenderTarget and delegate the logic to the texture type. So instead of WebGLCubeRenderTarget it would be a WebGLRenderTarget with a CubeTexture. Same with DataTexture3D, DataTexture2DArray, etc

Not sure if that would work for MultisampleTexture though.

@mrdoob mrdoob added this to the r121 milestone Aug 24, 2020
@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 24, 2020

As a first step in this process, how about removing this hack:

this.texture.isWebGLCubeRenderTargetTexture = true; // HACK Why is texture not a CubeTexture?

Make WebGLCubeRenderTarget using a CubeTexture seems like a change the engine definitely wants^^.

@mrdoob
Copy link
Owner

mrdoob commented Aug 24, 2020

@Mugen87 Yep! I guess if we give that a go we'll have a better picture of whether the idea is viable or not.

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 24, 2020

@DavidPeicho Do you want to investigate this or should I have a look? 😇

@DavidPeicho
Copy link
Contributor Author

I wonder if it would be possible to just have a single WebGLRenderTarget and delegate the logic to the texture type

100% what I was thinking. Doing that makes it much easier to have any texture attached as a render target.

Not sure if that would work for MultisampleTexture though.

Not sure yet, I have to check! I am not sure you can multisample a 3D texture.

@DavidPeicho Do you want to investigate this or should I have a look? 😇

I can give it a look. I will need your opinions on some stuff obviously especially regarding the API.

So let me try to use the original WebGLRenderTarget with any supported texture as an attachment, and without breaking the API 😄

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 25, 2020

I can give it a look. I will need your opinions on some stuff obviously especially regarding the API.

I think I would start with a fresh PR which only introduces CubeTexture in WebGLCubeRenderTarget, first. No API changes should be required here. It's be a pure internal change.

@DavidPeicho
Copy link
Contributor Author

Wouldn't it be better to try to also migrate WebGLCubeRenderTarget in WebGLRenderTarget?
That makes sense, and only the internal renderer cares about what it renders to.

I feel the concept of render target should be dissociated from the texture type it has. It's obviously hard now to make that without a breaking change, but we can keep the current behavior and maybe add console.warn as usual.

WebGLRenderTarget does nothing except exposing the fromEquirectangularTexture that could also be somewhere else?

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 25, 2020

Wouldn't it be better to try to also migrate WebGLCubeRenderTarget in WebGLRenderTarget?

Not sure if we can do this change for backwards compatibility reasons. Keep in mind that users create instances of WebGLCubeRenderTarget, e.g. when using fromEquirectangularTexture() or when creating CubeCameras. This topic needs more investigation.

I would focus on integrating CubeTexture in WebGLCubeRenderTarget first.

@DavidPeicho
Copy link
Contributor Author

DavidPeicho commented Aug 25, 2020

Yes, that's why I was thinking about maybe letting the user set its own texture for now in WebGLRenderTarget. That will fix most issues and allow to render to any texture.
We can make the change in parallel to WebGLCubeRenderTarget so that it's still backward compatible.
Doing that basically doesn't break anything, and this PR can go on.

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 25, 2020

First try: #20182

@DavidPeicho
Copy link
Contributor Author

DavidPeicho commented Aug 26, 2020

Okay great, thanks!

I tried to re-work the more general case, allowing WebGLRenderTarget to accept 3D texture and 2D texture array.

Basically, now you can set your texture via:

const renderTargetTexture = new THREE.DataTexture2DArray();
renderTargetTexture.format = ...;
renderTargetTexture.type = ...;

const rt = new THREE.WebGLRenderTarget();
rt.setTexture( renderTargetTexture );
rt.setSize( 100, 100, 50 );

This sounds pretty reasonable to me. It doesn't introduce any API break change. The few things I am not 100% fan are:

  • It's not really possible to pass the depth as an argument to THREE.WebGLRenderTarget, because it's suppose to have an options object. To be fair it could be enough to check whether options is a number.
function WebGLRenderTarget( width, height, options ) {
        
        if ( !isNaN( options ) ) {
        
                this.depth = options;

        }
        ...
}
  • A Texture may be created for nothing by default in the render target. I don't think that's too bad because anyway, creating render target isn't supposed to be a hot code path

Let me know what you think @mrdoob and @Mugen87

@takahirox takahirox mentioned this pull request Aug 27, 2020
@DavidPeicho
Copy link
Contributor Author

I actually found today that using ANGLE, rendering to 3D textures will fail on a layer != 0. I tried on a Windows machine and it failed.

Links:

@mrdoob mrdoob modified the milestones: r121, r122 Sep 29, 2020
@mrdoob mrdoob modified the milestones: r122, r123 Oct 28, 2020
@mrdoob mrdoob modified the milestones: r123, r124 Nov 25, 2020
@mrdoob mrdoob modified the milestones: r124, r125 Dec 24, 2020
@DavidPeicho
Copy link
Contributor Author

Any news on how you want me to proceed API-wise?
I have to look whether this is still an issue in Angle

@mrdoob mrdoob modified the milestones: r125, r126 Jan 27, 2021

if ( renderTarget ) {

isRenderTarget3D = renderTarget.is3D();
Copy link
Owner

@mrdoob mrdoob Feb 3, 2021

Choose a reason for hiding this comment

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

Instead of adding is3D to WebGLRenderTarget I think I would rather add the code here:

isRenderTarget3D = renderTarget.texture.isDataTexture3D || renderTarget.texture.isDataTexture2DArray;

Copy link
Owner

@mrdoob mrdoob Feb 6, 2021

Choose a reason for hiding this comment

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

I'll see if I can resolve the change and fix this on my end.

@mrdoob mrdoob merged commit e1e5ce7 into mrdoob:dev Feb 6, 2021
@mrdoob
Copy link
Owner

mrdoob commented Feb 6, 2021

Aaaand merged. Thanks!

@mrdoob
Copy link
Owner

mrdoob commented Feb 6, 2021

@DavidPeicho
Copy link
Contributor Author

Hi! I will have a look in the next fews days. I let that on the side because of the bug in ANGLE. I will fix the black screen no worries

@mrdoob
Copy link
Owner

mrdoob commented Feb 6, 2021

Thanks!

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