Add multisampled color and depth textures

[bwanders]

Hi there!

First off, three-d is a joy to work with, even for someone with little experience in Rust (though admittedly, I do have some experience working with OpenGL).

While experimenting with custom shaders (to implement a bloom effect, in this case), I ran into the issue that the default framebuffer uses some form of multisampling. Created textures do not come with multisampling by default, so any attempt to render the scene to a texture will lead to 'straight line staircase' aliasing:

Above screenshot is from the example included in this PR. Aliasing can also be observed live in the fog example, when looking closely at the edges of the model.

The proposed changes add Texture2DMultisample and DepthTexture2DMultisample to allow rendering to a multisampled texture. A new example called multisample is also included to clearly show the difference. This allows render-to-texture to also enjoy the benefits of anti-aliasing:

Unfortunately, these multisampled textures come with some limitations:

• The current sampling shader snippet for the *Multisample textures effectively performs nearest filtering, due to having to use a sampler2DMS texture sampler in the fragment shader, which only supports texelFetch. This works fine when using copy_from to copy the color values (or depth values) from a texture to the default framebuffer, but is less desirable when using the multisampled texture to texture a mesh.
• The "correct" way to resolve the multisampled texture into a non-multisampled texture is to perform a blit. As far as I was able to discern, three-d does not expose a blit operation between two RenderTargets. Therefore, at the moment the example uses copy_from, which performs fine for this case both time-wise and visual-quality-wise.

Despite the limitations, I think that the benefits of having multisampled color and depth textures outweighs the complexity of their pecularities at shader level, especially since the default case of 'render to texture screen-sized , copy to screen' is covered.

[asny]

Hi!

Thanks a lot for the kind words 🙏 I'm happy that you find it easy to use 🙂

Super nice contribution! If you haven't seen it, multisample render targets has actually been on the to do list for a rather long time (#73), so it's nice to finally get it started. And, looking really quickly, it seems to be almost done 💪 I will take a closer look real soon and get back to you.

The current sampling shader snippet for the *Multisample textures effectively performs nearest filtering, due to having to use a sampler2DMS texture sampler in the fragment shader, which only supports texelFetch. This works fine when using copy_from to copy the color values (or depth values) from a texture to the default framebuffer, but is less desirable when using the multisampled texture to texture a mesh.

I think it would be very weird to use a MSAA texture to texture a mesh, but maybe it should be documented, that if you want to use it to texture a mesh, you need to copy it to a regular texture first 🙂

The "correct" way to resolve the multisampled texture into a non-multisampled texture is to perform a blit. As far as I was able to discern, three-d does not expose a blit operation between two RenderTargets. Therefore, at the moment the example uses copy_from, which performs fine for this case both time-wise and visual-quality-wise.

You are right, I had blit support at some point, but it was not working on all platforms with all texture settings, so it was easier to have one solution that worked everywhere. Also, I think it's not necessarily faster to use blit because it's not well supported and therefore not optimised on all platforms, whereas render calls are very optimised 🙂

[bwanders]

I think it would be very weird to use a MSAA texture to texture a mesh, but maybe it should be documented, that if you want to use it to texture a mesh, you need to copy it to a regular texture first 🙂

That's probably a good idea. I can see this happen if someone wants to implement an security camera type thing where they render the scene as seen from the security camera to a texture, and follow up by using that texture on a monitor model somewhere else in the scene. A note in the documentation might be a good idea there.

You are right, I had blit support at some point, but it was not working on all platforms with all texture settings, so it was easier to have one solution that worked everywhere. Also, I think it's not necessarily faster to use blit because it's not well supported and therefore not optimised on all platforms, whereas render calls are very optimised 🙂

Ah. That makes sense!

[asny]

I've looked through it and it looks good except for one issue and unfortunately that is a major issue 😬 tex storage 2D multisample is not supported in webgl2 (which is probably the reason why I didn't add it at an earlier stage) 😞

However, renderbuffer storage multisample is supported, so could we use that and then not support sampling of the texture (revert the changes in program.rs) and use blit to copy (check if the color and depth textures given as input to RenderTarget::copy_partially_from are multi sample and then do a blit and, if they are not, then just do exactly as currently)?

If that's a way forward, I would also add a filter parameter (of type Interpolation) when constructing the multi sample texture and then use that parameter when blitting.

[bwanders]

tex storage 2D multisample is not supported in webgl2

Oh. That's disappointing.

I can see the render buffer storage method working. If we go down this route, we will be handling the render buffer storage as if it were a texture, by handling it via the ColorTexture machinery... Hopefully we don't run into to many situations where the fact that it's not actually a texture---but a render buffer pretending to be one---collides with expectations elsewhere in the codebase. (I see that cubemaps also have unimplemented!() fragment shader sources, so there's some precedent for that; but at least a cubemap is still a real texture in the sense that you can use it as a uniform in a shader.)

Personally, I would say that render buffer storage is not enough like a texture that we can shoehorn them in and pretend that everything is fine. I understand why you didn't add multisample rendering at an earlier stage... If you are comfortable with going down the route of using render buffer storage, I'm willing to give implementing that a try somewhere in the coming week.

If we do decide to go down the render buffer route: the filter parameter is a good idea, but we will need to decide what to do when blitting both the color and depth buffer. Blitting via glBlitFramebuffer constrains the filtering based on whether you are blitting the depth buffer.

Having the filter set on the color buffer might create a situation where you have to set up your color buffer differently based on whether you'll be blitting with or without depth buffer later on... To me, that looks like a situation that could create some confusion (by having implementation details leaking through the abstraction) if you just want to render anti-aliased images 🤔

That being said, I'd also be perfectly fine with closing this PR and postponing multisample rendering for now, my original use case has since been handled in another way. In that case, my work on the boilerplate code will hopefully help ease the way towards multisampled render targets in the future!

[asny]

I've postponed it several times already, so wanted to do it now that I still remember what the problems are 🙂 I've experimented a bit and got to a place that I'm quite happy with. What do you think? It's not done, but the idea is that you can only create a multisample render target (I've reverted the changes to ColorTexture), consisting of either a color buffer, depth buffer or both. You can then render into it and afterwards resolve it to regular textures.

The reason for not creating a multisample version of a ColorTarget and DepthTarget is that you cannot combine a multisample color buffer with a non-multisample depth buffer. This way, it is ensured that both are multisample or both are not.

Having the filter set on the color buffer might create a situation where you have to set up your color buffer differently based on whether you'll be blitting with or without depth buffer later on... To me, that looks like a situation that could create some confusion (by having implementation details leaking through the abstraction) if you just want to render anti-aliased images 🤔

You are right, and it's not necessary when you resolve to a texture that support linear sampling.

[bwanders]

This looks good. Not offering multisample versions of the ColorTarget and DepthTarget makes sense. The resolve methods are a nice way to not need a public blit operation, these methods really help to clarify the intended use!

The only remark I have is that, purely from an ergonomic point of view, it might be better if we can somehow drop the call to as_render_target. The expected usage of the multisample target will usually look something like this "create RenderTargetMultisample, call as_render_target, render into the target." I can see an argument that converting something to a render target when it is literally called RenderTargetMultisample is not the most understandable required step. But in all honesty, this is a very minor nitpick and I have no idea if it is even possible.

To me it seems almost done (or at least, all required functionality is almost done)?

[asny]

This looks good. Not offering multisample versions of the ColorTarget and DepthTarget makes sense. The resolve methods are a nice way to not need a public blit operation, these methods really help to clarify the intended use!

Nice, I'm glad to hear it makes sense 👍

The only remark I have is that, purely from an ergonomic point of view, it might be better if we can somehow drop the call to as_render_target. The expected usage of the multisample target will usually look something like this "create RenderTargetMultisample, call as_render_target, render into the target." I can see an argument that converting something to a render target when it is literally called RenderTargetMultisample is not the most understandable required step. But in all honesty, this is a very minor nitpick and I have no idea if it is even possible.

Again, I agree with you 100% 🙂 I wasn't happy with the API of RenderTargetMultisample either, just the overall structure. So I've managed to find a bit of time to improve it, now there is no call to as_render_target, instead RenderTargetMultisample implements all of the methods that are relevant (for example copy_from and read is not relevant for a multisample render target). Also, there is now a ColorTargetMultisample and DepthTargetMultisample to avoid optional arguments to RenderTargetMultisample. What do you think, is this better?

To me it seems almost done (or at least, all required functionality is almost done)?

Yeah, it's getting close 🙂 This is what I can think of that still needs a bit of improvement:

• Ensure a valid resolve texture (equal width, height, depth format and color format as the multisample texture) - I'm not quite sure how to do this!? If it is not possible or feasible, it could just be documented. Maybe the resolve texture could be created at the same time as the multisample texture, but then we need a lot of new methods to cover all of the different cases of textures (texture2d, cubemap, texture array) and combinations of multisample/non-multisample targets (for example a color and depth multisample target that resolves to a color target).
• Check valid number of samples
• Documentation

[bwanders]

What do you think, is this better?

Absolutely! 👍

• Ensure a valid resolve texture (equal width, height, depth format and color format as the multisample texture) - I'm not quite sure how to do this!? If it is not possible or feasible, it could just be documented.

One way to partially enjoy such sanity checks is to rename the current resolve_* family of methods to resolve_*_to (that since they resolve into an external texture), and re-introduce resolve (and friends) as resolving into an internally created "default" texture.

The idea being that if you just want to render something multisampled and resolve it, you can get a resolved texture via resolve; and if you want full control, you can get that via de resolve_to. But having full control will also leave you with a scaled blit if you give it a different width/height, or failed blit if you give it the wrong formats. Unfortunately, this would necessitate putting the generic type arguments for C and D back on the multisample targets (as was the solution before a2898ffb0294d649ba4e441d5885cb8617d4ed03).

If this approach works for you, I can work on implementing it. 😃

• Check valid number of samples

I don't think this is feasible to add.

This is an implementation-dependent number. In native OpenGL situations, I would suggest using glGetInternalformativ to get the actual supported sample counts for the texture data type. And in WebGL contexts roughly the same functionality is captured by getInternalformatParameter. But I am unable to find either of these functions in glow, and they would also both suffer from the same issue: it would not be a compile-time check.

Alternatively we could try to check against the maximum number of samples (via glGet* with GL_MAX_SAMPLES), but that would give a false sense of security, since not every internal format necessarily supports the maximum number of samples. (And also, I don't think every number below the maximum is valid, they'll probably follow powers of two.)

At least a sample count of 4 is nearly always valid (due to being required by the spec), so I think we document that and leave it there. (Unless you have some deeper insight into this, in which case let's do that!)

[asny]

Absolutely! 👍

Nice 🥳

• Ensure a valid resolve texture (equal width, height, depth format and color format as the multisample texture) - I'm not quite sure how to do this!? If it is not possible or feasible, it could just be documented.

One way to partially enjoy such sanity checks is to rename the current resolve_* family of methods to resolve_*_to (that since they resolve into an external texture), and re-introduce resolve (and friends) as resolving into an internally created "default" texture.

The idea being that if you just want to render something multisampled and resolve it, you can get a resolved texture via resolve; and if you want full control, you can get that via de resolve_to. But having full control will also leave you with a scaled blit if you give it a different width/height, or failed blit if you give it the wrong formats. Unfortunately, this would necessitate putting the generic type arguments for C and D back on the multisample targets (as was the solution before a2898ff).

If this approach works for you, I can work on implementing it. 😃

Yes, by all means, go ahead and try it out, I don't think I have time anyway the next week or so 👍 Just FYI, the reason I removed the generic C and D was it made the macro_rules! much more difficult, but I'm also not at all a macro expert 🙂

• Check valid number of samples

I don't think this is feasible to add.

This is an implementation-dependent number. In native OpenGL situations, I would suggest using glGetInternalformativ to get the actual supported sample counts for the texture data type. And in WebGL contexts roughly the same functionality is captured by getInternalformatParameter. But I am unable to find either of these functions in glow, and they would also both suffer from the same issue: it would not be a compile-time check.

Alternatively we could try to check against the maximum number of samples (via glGet* with GL_MAX_SAMPLES), but that would give a false sense of security, since not every internal format necessarily supports the maximum number of samples. (And also, I don't think every number below the maximum is valid, they'll probably follow powers of two.)

At least a sample count of 4 is nearly always valid (due to being required by the spec), so I think we document that and leave it there. (Unless you have some deeper insight into this, in which case let's do that!)

Yeah, I wasn't too precise here, I was thinking of a runtime check to avoid people getting a generic 'invalid operation' error that is difficult to interpret. I didn't know the supported sample count varied by format, but that makes sense. But anyway, I think it's enough to check that it's power of 2 and that it's less or equal to GL_MAX_SAMPLES. If it's not, then panic with a useful error message. It might still happen that people will get a 'invalid operation' error but at least most people will get a useful error 🙂

[bwanders]

(Ignore the force push, that was me discovering that some interface elements of github do not do the thing I expect them to do, and me consequently correcting that 😞)

Yes, by all means, go ahead and try it out, I don't think I have time anyway the next week or so 👍 Just FYI, the reason I removed the generic C and D was it made the macro_rules! much more difficult, but I'm also not at all a macro expert 🙂

Done and done! I've kept the macros deliberately non-generic to make them easier to understand and maintain (f533fe10f42e537ac9213bf8319b5f8d8391a971). If we want a more generic version, we can use the method described in Rust macro accepting type with generic parameters. I've tried that, and it worked, but the macro becomes symbol soup.

Yeah, I wasn't too precise here, I was thinking of a runtime check to avoid people getting a generic 'invalid operation' error that is difficult to interpret.

That makes way more sense than what I was thinking of. I've implemented a multisample_sanity_check function that encompasses these checks. I'm note sure about what we should say in the panic messages yet, so I kept them simple (895bf205cacb2c9103fb6986529095609cf93e83).

Also included is a first stab at the necessary documentation in 41f437a32cf827fbab3742611ce17d429da338d2. This is just a first draft, so feel free to rewrite those completely to fit with the rest of the documentation 🤔

Let me know what you think!

[asny]

I think it looks awesome, really nice work 💪 and I'm glad you made the macros readable 😄

I will add a bit more documentation, but I think that's it. I hope to get to it real soon 🙂

[asny]

I had a bit of a crazy week, so didn't get to it "real soon", but better late than never 😆 I only changed minor things:

• Only check for correct number of multisamples if in debug mode since it has a GL call.
• Fixed blit_to in the case where it blits to the screen render target (also added a comment about that in RenderTargetMultisample::resolve_to)
• Use linear sampling for default resolve to textures
• Documentation

I'll merge as soon as the tests are green. Thank you so much for your contribution 💪

[bwanders]

It was nice to work on this for a bit, thanks for all the feedback 🎉

[asny]

I'm glad to hear and no problem 🙂 Feel free to pick another task at some point if you feel like it 💪