Z-writing wrongly enabled for transparent shader materials

[hendu]

Background: http://irrlicht.sourceforge.net/forum/v ... 08#p286808

Patch:
https://github.com/clbr/seirr/commit/ea ... 2eb535a6e3

SMaterial had a hardcoded check for isTransparent. That won't do in a shader world. This patch fixes the GL driver to use the MaterialRenderer's isTransparent function, and for good measure nukes the ugly SMaterial one, so that every place that needs the isTransparent check uses the Renderer's one.

[CuteAlien]

Patch looks good to me on first view. Same changes in DX9 and DX8 I guess (both seem to have that check). I hope Hybrid finds time to give this a quick view in case there was a reason why this wasn't changed yet for zwrite which we are missing (as all other places already seem to use the renderer isTransparent... so why was that one place left out?).

edit: Also function should be deprecated for now - not removed.

[CuteAlien]

Just did dig around a little - and this would change it for one more material besides shaders: ONETEXTURE_BLEND. Which has this comment: /** Is not always transparent, but mostly. */ - so probably it shouldn't always return true in isTransparent. This is the only material where SMaterial.isTransparent() and IMaterialRenderer::isTransparent return different results.

Unfortunately I've not yet worked with that material and don't know when it needs to return true or false. Or if maybe isTransparent needs another parameter.

[hendu]

Blending individual nodes is rather error-prone, but even for them it's better to have it happen in the transparent pass. Usually you would use that for screen quads with manual control.

Ie, even if it is not transparent, it needs the underlying picture to produce useful results, so isTransparent true is correct.

[Nadro]

Thanks for this patch, it looks good for me too, however I think that we should add further changes to current blending solution:
1. MaterialRenderer may force blending and assign node to transparent pass (built-in EMT_TRANSPARENT_ADD_COLOR, EMT_TRANSPARENT_ALPHA_CHANNEL etc).
2. If SMaterial has blending operation set to other value than EBO_NONE this object should be also assigned to transparent pass.

After these modifications we'll have more flexible blending system which will be compatible with existing materials (bug with transparent MaterialRenderer + EBO_NONE will be gone) and even allow us to remove BaseMaterial param from shader based materials, because at now we'll be able to enable/disable blending per each object without problems and render them in properly pass. No more separate materials for both solid and transparent objects like EMT_SOLID and EMT_TRANSPARENT_ALPHA_CHANNEL.

After this modification Hendu's patch will looks like this:

Code: Select all

if (material.ZWriteEnable && (AllowZWriteOnTransparent ||
(!MaterialRenderers[material.MaterialType].Renderer->isTransparent() &&
material.BlendOperation == EBO_NONE)))

What do you think about it?

[hendu]

Sounds good.

No more separate materials for both solid and transparent objects like EMT_SOLID and EMT_TRANSPARENT_ALPHA_CHANNEL.

Though I don't like this part. If I want simple transparency, after this change I would need to write two complex lines instead of one simple.

before:

Code: Select all

materialtype = EMT_TRANSPARENT_ALPHA_CHANNEL

after:

Code: Select all

materialtype = EMT_ONETEXTURE_BLEND
materialtypeparam = pack_texturefunc(bla, bla)

In other words, usability regression.

[Nadro]

Yes, you're right. It looks like we should erase point related to:

remove BaseMaterial param from shader based materials

In this case our custom materials will be able to force blending when it'll be necessary like in built-in materials: EMT_TRANSPARENT_ADD_COLOR, EMT_TRANSPARENT_ALPHA_CHANNEL etc, so we'll avoid usability regression from your example Thanks to it, we'll also avoid API break, what is also important.

I'll try to prepare this patch tomorrow. If all will be fine we'll merge both Hendu's zwrite fix and extended blending system patches to trunk.

[Nadro]

This is preview of this patch (it include Hendu's fix):
http://www.sendspace.com/file/4a0zuq

[CuteAlien]

Maybe there is some way to download it without getting an .exe ... but I can't see it. You can post patches to http://ideone.com/ usually.

[Nadro]

You don't need any software for download it. Just click button called "Click here to start download from sendspace"

If you will have problems with sendspace I'll upload it to ideone.com after back to home.

[CuteAlien]

I click that and it offers me a new link - which leads again to downloading an exe.

[Nadro]

OK, I'll upload it to http://ideone.com/ after back to home.

[CuteAlien]

Or just apply it, it's probably better than the current solution ;-) (and if not we can still modify it).

[Nadro]

I applied it to trunk. Just think about move this part of code:

Code: Select all

// force blending if necessary
if (Material.BlendOperation == EBO_NONE && MaterialRenderers[Material.MaterialType].Renderer->isTransparent())
    Material.BlendOperation = EBO_ADD;

to some method, but I'm not sure if CNullDriver is good place for it, however I don't have a better idea where we could put this method.

UPDATE:
We should add the same trick with MaterialTypeParam to force blending type by material renderer and call setBlendFunc in setBasicRenderStates method. After these modifications we'll have fully flexible blending solution. I'll prepare commit tomorrow.

[CuteAlien]

OK, I'm a little out of my usual zone when it comes to that code, but that's my feedback:

- Why force that blending? I admit I was never too happy with setting the default to EBO_NONE, but in the end that's what we have now and I could live with it (or work around it). I know people constantly run into this - but changing SMaterial values might be even worse. I think we should see SMaterial more or less as a read-only structure for rendering - it's something users control and not the engine. I've seen for example a few times already that users started abusing SMaterial variables which were unused by the Material for passing around other values (not nice... but well... it's still done). So it's not something we should change unless absolutely necessary or we start breaking user applications. So my guess would be that the only code that could be allowed to ignore EBO_NONE are either video-renderers or material-renderers on setup. And in case render-code can't ignore wrong flags then the user just has to care about setting that value correct (meaning we won't have to do anything about it and can revert the enforcing part).

- Shouldn't the check for EBO_NONE be in the material-renderers? Or even just in the one-texture-blend material-renderer parts? If this decides if the material is transparent - then with the current solution isTransparent continues to return the wrong value sometimes. Which is probably the reason why you added the additional EBO_NONE checks everywhere where isTransparent is used. Seems to make more sense to fix the isTransparent functions directly so no more workarounds are needed.