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

MRT Support #16390

Merged
merged 28 commits into from
May 6, 2021
Merged

MRT Support #16390

merged 28 commits into from
May 6, 2021

Conversation

takahirox
Copy link
Collaborator

@takahirox takahirox commented May 5, 2019

Fixes #5179.

Kudos to @mattdesl and @edankwan

MRT PR was opened #9358 #8439. It needed to be cleaned up to get merged but that PR hasn't been updated for years. I still want to see MRT in Three.js, so I cleaned up and created a new PR.

API

In the current implementation, users need to use RawShaderMaterial (or onBeforeCompile() for built-in material) to render to WebGLMultiRenderTarget.

const attachmentNums = 2;
const renderTarget = new THREE.WebGLMultiRenderTarget( width, height, attachmentNums );

// rendering to MultiRenderTarget
renderer.setRenderTarget( renderTarget );
renderer.render( scene, camera );

// rendering to screen with MultiRenderTarget as input
material1.map = renderTarget.textures[ 0 ];
material2.map = renderTarget.textures[ 1 ];
renderer.setRenderTarget( null );
renderer.render( scene2, camera2 );

// WebGL1 fragment shader

#extension GL_EXT_draw_buffers : require
...
void main() {
	gl_FragData[ 0 ] = ...
	gl_FragData[ 1 ] = ...
}

// WebGL2 fragment shader

#version 300 es
layout(location = 0) out vec4 gColor;
layout(location = 1) out vec4 gNormal;
...
void main() {
	gColor = ...
	gNormal = ...
}

@WestLangley
Copy link
Collaborator

Nice! I did not review the renderer code, though. The hard part. :-)

@fernandojsg
Copy link
Collaborator

Looking great! It could be nice to add also a WebGL1 MRT example to see the differences and test between both implementations

@takahirox
Copy link
Collaborator Author

takahirox commented May 6, 2019

Thanks for the review. I updated.

One concern with WebGL 1. No proper way to inject #extension GL_EXT_draw_buffers : require to the top of fragment shader code with ShaderMaterial because it adds common code to the top. So users need to use RawShaderMaterial instead of ShaderMaterial with WebGL 1. I'd like to think of the solution and WebGL 1 example later, after this PR is merged.

@fernandojsg
Copy link
Collaborator

@takahirox thanks, sounds good, we could get webgl2 support landed and iterate on the webgl1 example in another PR.

@nayyden
Copy link

nayyden commented Jun 5, 2019

Hi, any plans to release this in r106?

@valenn
Copy link

valenn commented Jun 7, 2019

Thanks for the review. I updated.

One concern with WebGL 1. No proper way to inject #extension GL_EXT_draw_buffers : require to the top of fragment shader code with ShaderMaterial because it adds common code to the top. So users need to use RawShaderMaterial instead of ShaderMaterial with WebGL 1. I'd like to think of the solution and WebGL 1 example later, after this PR is merged.

There is already ShaderMaterial.extensions.drawBuffers that does this, or does that cause any problems? (also if there is this extension already supported, is it possible to use it already?)

@nayyden
Copy link

nayyden commented Jun 7, 2019

There is already ShaderMaterial.extensions.drawBuffers that does this, or does that cause any problems? (also if there is this extension already supported, is it possible to use it already?)

For MRT we need a way to bind multiple color attachments to the framebuffer and I don't think this is possible with the current API.

@oskardahlberg
Copy link

This is great. Hope this can land soon. Two notes,

Firstly, built in materials currently write out highp vec4 pc_fragColor; into their frag shaders and this prohibits them from being used with this target. Changing this (in WebGLProgram) to layout(location = 0) out highp vec4 pc_fragColor; solves this and I can't see any potential issues with doing so.

Secondly, in the example, the texture options is set like this

renderTarget = new THREE.WebGLMultiRenderTarget( window.innerWidth * window.devicePixelRatio, window.innerHeight * window.devicePixelRatio, 2 );
renderTarget.texture.format = THREE.RGBAFormat;
renderTarget.texture.minFilter = THREE.NearestFilter;
renderTarget.texture.magFilter = THREE.NearestFilter;
renderTarget.texture.type = THREE.FloatType;
renderTarget.texture.generateMipmaps = false;

but to my understanding, this wont actually do anything. There is already a way to set it that does seem to work though, so changing this might be good before releasing.

renderTarget = new THREE.WebGLMultiRenderTarget(
  window.innerWidth * window.devicePixelRatio,
  window.innerHeight * window.devicePixelRatio,
  2,
  {
    format: THREE.RGBAFormat,
    minFilter: THREE.NearestFilter,
    magFilter: THREE.NearestFilter,
    type: THREE.FloatType,
    generateMipmaps: false
  }
);

Do verify this before changing it, but it seems highly odd the way it is.

@@ -753,14 +775,14 @@ function WebGLTextures( _gl, extensions, state, properties, capabilities, utils,
// Render targets

// Setup storage for target texture and bind it to correct framebuffer
function setupFrameBufferTexture( framebuffer, renderTarget, attachment, textureTarget ) {
function setupFrameBufferTexture( framebuffer, renderTarget, texture, attachment, textureTarget ) {

var glFormat = utils.convert( renderTarget.texture.format );
var glType = utils.convert( renderTarget.texture.type );
var glInternalFormat = getInternalFormat( glFormat, glType );
state.texImage2D( textureTarget, 0, glInternalFormat, renderTarget.width, renderTarget.height, 0, glFormat, glType, null );
Copy link
Collaborator

@gkjohnson gkjohnson Jun 27, 2019

Choose a reason for hiding this comment

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

This can probably wait but is it possible for the different color attachments to have different sizes?

Copy link

@nayyden nayyden Jul 23, 2019

Choose a reason for hiding this comment

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

I am not sure if different image sizes have any real-world usage (if supported at all). Although the fragment stage outputs several colors, the rasterization stage has been executed only once, so separate target resolutions are not possible. To write to the smaller/bigger images the driver would need to down/up sample which might be possible but the gain of having some of them smaller is probably none. The gain of MRT is to exactly execute everything prior the fragment shader once only.

According to this post it might work but will be rendered with the size of the smallest image https://community.khronos.org/t/fbo-with-different-size-attachments/62925/2

P.S. Different formats and types for the images are possible and have practical usage. I read that some combination could possibly fail depending on the implementation. Not sure if this applies to WebGL or OpenGL only.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd like to go as is so far, and consider different size if users actually demand it.

@takahirox
Copy link
Collaborator Author

takahirox commented Jul 24, 2019

@oskardahlberg

Thanks for the comments.

Firstly, built in materials currently write out highp vec4 pc_fragColor; into their frag shaders and this prohibits them from being used with this target. Changing this (in WebGLProgram) to layout(location = 0) out highp vec4 pc_fragColor; solves this and I can't see any potential issues with doing so.

Built-in material as is seems working fine with WebGLMultiRenderTarget where numAttachments = 1. I guess if there is only one shader buffer output without location specification it seems being recognized as location = 0 (I couldn't find that definition in the specs tho).

Secondly, in the example, the texture options is set like this

The last argument of RenderTarget constructor is for its texture parameters so I think renderTarget.texture.xxx = yyy works but yeah defining via the constructor looks properer way. Otherwise, texture parameters won't set to .textures. Updated Example.

@takahirox
Copy link
Collaborator Author

Thanks for the comments. Updated. And using module in example now. I think ready to go.

@mrdoob
Copy link
Owner

mrdoob commented Apr 21, 2021

I think this is almost there! We should be able to land this at the beginning of the next dev cycle 🤞

A few more changes...

I don't think we should add setTexture(), setSize() or even copy(). For example, WebGLCubeRenderTarget doesn't have them and no one has requested them yet.

As per the class name, I think WebGLMultipleRenderTargets would be more clear. Otherwise it can be confused with WebGLMultisampleRenderTarget. And making the class name plural helps too.

I'm not sure it's a good idea to extend from WebGLRenderTarget either...

@mrdoob mrdoob modified the milestones: r128, r129 Apr 21, 2021
@takahirox
Copy link
Collaborator Author

We should be able to land this at the beginning of the next dev cycle 🤞

That sounds good to me.

I don't think we should add setTexture(), setSize() or even copy(). For example, WebGLCubeRenderTarget doesn't have them and no one has requested them yet.

I have removed setTexture(). But I think setSize() is necessary for example it's called on window resize.

And copy() would be necessary, too. Because it's used by clone() defined in the parent WebGLRenderTarget.

As per the class name, I think WebGLMultipleRenderTargets would be more clear. Otherwise it can be confused with WebGLMultisampleRenderTarget. And making the class name plural helps too.

Sounds good to me. I renamed it.

I'm not sure it's a good idea to extend from WebGLRenderTarget either...

Personally I prefer extending WebGLRenderTarget for the consistency with other render targets.

@takahirox
Copy link
Collaborator Author

So, r128 has been released. It's time to go now?

@marcofugaro
Copy link
Contributor

Happy 2nd birthday to this PR!! 🥳🥳🥳🥳

@mrdoob
Copy link
Owner

mrdoob commented May 6, 2021

What better way to celebrate than...

@mrdoob mrdoob merged commit 2510609 into mrdoob:dev May 6, 2021
@mrdoob
Copy link
Owner

mrdoob commented May 6, 2021

Thanks!

@takahirox takahirox deleted the MRTSupport branch May 6, 2021 04:08
@takahirox
Copy link
Collaborator Author

takahirox commented May 6, 2021

Finally the time has come, thanks!

The PR was originally made two years ago. There are a lot of updates which may affect the PR in the core during that time. I think I carefully kept updating the PR but let me know if you folks find anything which doesn't match the newest core API/design/behavior/etc.

@Mugen87 As I wrote in the PR comment, I took over @mattdesl and @edankwan works. Would you please add their names in the change log next to me?

@mrdoob mrdoob mentioned this pull request May 6, 2021
@takahirox
Copy link
Collaborator Author

takahirox commented May 6, 2021

And I think there is a big room for API improvement. For example currently WebGLRenderMutlipleRendertargets is expected to be used with RawShaderMaterial. But we may enable built-in materials MRT support.

When I made this PR, there were a lot of difficulties and limitations to improve the API. But during the two years we have had tons of new features and simplifications in Three.js (mainly done by @Mugen87 thanks). Such difficulties and limitations might have been removed and we might be able to improve the API now. Let's cooperate and work on it all together.

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

Successfully merging this pull request may close these issues.

When THREE.js will support "WEBGL_draw_buffers"?