Fix rendering of translucent items in inventory & when wielded

[appgurueu]

Closes #5461. Ready for review.

How to test

Look at the three marbles, both in the inventory and as wield items:

• The opaque marble should be opaque.
• The clipped marble should be clipped.
• The blended marble should be blended.

Also look at the alpha test node with alpha = 64. It should be visible, both in your inventory and when wielded. It should look like this:

Cycle through the devtest items and verify that they look right; also look at some simple wield items, like pickaxes, to make sure that there are no regressions.

Furthermore, enable inventory_items_animations and check that the spinny items look as expected.

Relevant documentation: https://irrlicht.sourceforge.io/docu/namespaceirr_1_1video.html#ac8e9b6c66f7cebabd1a6d30cbc5430f1ac08aa3715ad41281472202107a81f736

[sfan5]

lgtm

[grorp]

I've been trying to get through the wieldmesh.cpp code and I can't say that I've got everything. However, from what I understand:

In both cases (wield & inventory), the mesh comes from wieldmesh.cpp. Wield mesh is from WieldMeshSceneNode::setItem, inventory mesh from getItemMesh (which share a lot of duplicated code btw). Their child functions already try to set the correct transparency material using TileLayer::applyMaterialOptions. On the master branch, this has no chance to work because the transparency material is always overwritten by the user of the mesh.

Now you've made the overwriting work correctly, but the transparency material is still being set somewhere in the mesh generation code too. If possible, I think it would be preferable to make the mesh generation code set the transparency material correctly, getting rid of the overwriting.

[Wuzzy2]

Awesome! These broken translucent inventory images for the transparency test in DevTest have bothered me for a long time. Thanks for fixing this.

I found a regression: When you wield air, the transparent pixels turn black. If spinning items are enabled, the pixels also turn black when you hover this item in the inventory.

Water sources turn semi-transparent, but the flowing water doesn't. Seems to be only the default flowing water tho. Maybe this is something in the node definition (so not your fault)?

The rest seems fine. I tested wielding all the items (tip: /hotbar 24 helps a lot), and hovering all the items with spinning items enabled.
I tested wielding the translucent items in front of translucent blocks to find bugs with depth sorting. All looks fine.
Moving the translucent items around in inventory also works fine.

I noticed there is no DevTest test item for testing the case of a inventory_image or wield_image with translucent pixels. I don't know if Minetest allows this. So I think this test case needs to be added.

[appgurueu]

I found a regression: When you wield air, the transparent pixels turn black. If spinning items are enabled, the pixels also turn black when you hover this item in the inventory.

Thanks for finding this. Fixed in 8c11c12:

Seems to be only the default flowing water tho. Maybe this is something in the node definition (so not your fault)?

Indeed this is a devtest bug; the flowing water definition is missing a ..ALPHA. Fixed in f06364e:

I noticed there is no DevTest test item for testing the case of a inventory_image or wield_image with translucent pixels. I don't know if Minetest allows this. So I think this test case needs to be added.

Added in a509aad, works as expected:

[appgurueu]

Alright @grorp, I've tried refactoring this (ed11b33). According to my testing, everything still works. I also tested some additional things:

• Shaders on/off - should be irrelevant
• Translucent liquids on/off - should be consistent
• Dropping items - should look as expected

[Wuzzy2]

Now every item glow in the wieldhand and as dropped item. Test the wield image at night/darkness and you'll see.

[appgurueu]

Now every item glow in the wieldhand and as dropped item. Test the wield image at night/darkness and you'll see.

I'm not sure I can reproduce. This is after /time 0 in devtest:

vs. /time 12000:

Tested with ed11b33. I also reviewed the code - I shouldn't haven't touched any Lighting = true/false assignments for materials.

[grorp]

Can reproduce with enable_shaders = true, cannot reproduce with enable_shaders = false. See also #9525 and related PRs.

The Lighting material flag is only for the fixed pipeline IIRC.

[grorp]

Posting this thing here too:

Summary

The mighty wieldmesh.cpp

#14701 as of commit ed11b33

WieldMeshSceneNode::setItem

determines material at the very beginning via ContentFeatures::getMaterialType and MaterialType_to_irr, saves it as m_material_type/m_material_param

cases:

• WieldMeshSceneNode::setExtruded
  • respects m_material_type/m_material_param
• WieldMeshSceneNode::setCube
  • uses TileLayer::applyMaterialOptions and respects m_material_type/m_material_param (???)
• createSpecialNodeMesh
  • uses TileLayer::applyMaterialOptions

=> diagnosis: inconsistent

getItemMesh

cases:

• getExtrudedMesh
  • always uses EMT_TRANSPARENT_ALPHA_CHANNEL/0.0f, material is overwritten later
  • mostly duplicate of WieldMeshSceneNode::setExtruded
• createCube + postProcessNodeMesh
  • doesn't set any material, material is overwritten later
  • maybe duplicate of WieldMeshSceneNode::setCube? (but doesn't use TileLayer::applyMaterialOptions)
• createSpecialNodeMesh
  • uses TileLayer::applyMaterialOptions and material is overwritten later (???)

sets material at the very end via ContentFeatures::getMaterialType and MaterialType_to_irr, by overwriting all materials of the already created mesh

=> diagnosis: inconsistent

Open questions

• What about TileLayer::applyMaterialOptions? only two usages, inconsistent
• What about TileLayer::applyMaterialOptionsWithShaders?
• There was code for rendering wieldmeshes using shader materials before, why is it gone?

[appgurueu]

WieldMeshSceneNode::setCube: Got rid of the m_material_type / m_material_type_param usage there, thanks. That should make setItem consistent: applyMaterialOptions is used for nodes, m_material_type[_param] for extrusion meshes (items, nodes that use inventory image / wield image).

getItemMesh is a mess. I'll try to clean it up. Note that for getExtrudedMesh, the material is not overwritten later on all paths.

Edit: I decided to leave the overwriting for plantlike drawtypes in for alpha blending consistency (if a plantlike node has opaque or clipped alpha, the extrusion mesh should be consistent with that). The alternative would be to pass the appropriate material type & param to getExtrudedMesh. I've gotten rid of the unnecessary assignment in createSpecialNodeMesh.

What about TileLayer::applyMaterialOptions? only two usages, inconsistent

You're right, I should try to use this consistently for nodes.

What about TileLayer::applyMaterialOptionsWithShaders?

Pretty much does a subset of the things TileLayer::applyMaterialOptions does, except it also sets normal map wrapping, but that is to my knowledge irrelevant: Wield meshes & items rendered in the inventory do not use normal maps. (IIRC the broken bumpmapping, and with it normal maps, were even (supposedly) removed entirely; I assume these are remnants?)

There was code for rendering wieldmeshes using shader materials before, why is it gone?

I wrongly assumed that this was obsolete after my refactoring, which introduced the regression. I still find it weird that we first give our materials to our shader to then get an Irrlicht material type (?) back, rather than having our shader deal properly with the material type it is given.

[grorp]

I still find it weird that we first give our materials to our shader to then get an Irrlicht material type (?) back, rather than having our shader deal properly with the material type it is given.

I believe it returns a "shader material" with an index past the builtin materials and that's how the shader is applied.

[appgurueu]

I believe it returns a "shader material" with an index past the builtin materials and that's how the shader is applied.

Yes. This is quite a dirty hack IMO, but I won't fix it here. Looking at the enum, this is unexpected (though EMT_FORCE_32BIT should probably have raised my eyebrow).

Something like std::variant<E_MATERIAL_TYPE, ShaderMaterialId> would probably make more sense.

---

Anyways, I think as far as this fix goes, this PR should be okay now? Please check whether I've missed anything.

[appgurueu]

(I just moved this around because I made it static.)

[appgurueu]

(Same here)

[sfan5]

I believe it returns a "shader material" with an index past the builtin materials and that's how the shader is applied.

Indeed.
Irrlicht has some built-in materials that are implemented either using shaders or by the fixed pipeline.
When you register a pair of vertex / fragment shaders that itself becomes a new "material" to use and it shares the enumeration space with the built-in ones.

Something like std::variant<E_MATERIAL_TYPE, ShaderMaterialId> would probably make more sense.

This would have unneeded overhead and make few sense given internal handling is often identical.

[grorp]

New summary

WieldMeshSceneNode::setItem

at the very beginning, m_material_type/m_material_type_param are set via ContentFeatures::getMaterialType/MaterialType_to_irr/getShader

cases:

• WieldMeshSceneNode::setExtruded (!wield_image.empty() && check_wield_image branch, fallback else branch)
  • material set from m_material_type/m_material_type_param
• WieldMeshSceneNode::setExtruded (def.type == ITEM_NODE branch)
  • material set from m_material_type/m_material_type_param
  • material overriden with m_material_type/m_material_type_param later
• WieldMeshSceneNode::setCube (def.type == ITEM_NODE branch)
  • material set via TileLayer::applyMaterialOptions (probably doesn't take shaders into account?)
  • material overriden with m_material_type/m_material_type_param later
• createSpecialNodeMesh (def.type == ITEM_NODE branch)
  • material not set
  • material overriden with m_material_type/m_material_type_param later

getItemMesh

cases:

• getExtrudedMesh (!inventory_image.empty() branch, def.type == ITEM_NODE && f.drawtype == NDT_AIRLIKE branch)
  • material always set to EMT_TRANSPARENT_ALPHA_CHANNEL/0.0f
• getExtrudedMesh (def.type == ITEM_NODE branch)
  • material always set to EMT_TRANSPARENT_ALPHA_CHANNEL/0.0f
  • material overriden with TileLayer::applyMaterialOptions later
• ExtrusionMeshCache::createCube (def.type == ITEM_NODE branch)
  • material not set
  • material overriden with TileLayer::applyMaterialOptions later
• createSpecialNodeMesh (def.type == ITEM_NODE branch)
  • material not set
  • material overriden with TileLayer::applyMaterialOptions later

Anyway, this PR causes a regression with the texture overlay test nodes:

master branch

this PR