Fix texture coordinates of cuboid drawtypes

[cx384]

• A more general solution than Fix texture coords off by one error #16055
• Fixes Combining animated & world-aligned textures has alignment issues. #12266 and more
• Changes texture coordinates of cuboid drawtypes to stay in the [0,1] range, instead of carrying the mapblock alignment and becoming negative after transformations

To do

Ready for review

How to test

See that texture coordinates are correct,
align style and other texture coordinates features like rotation still work.
and check nodebox drawtype nodes.

[cx384]

Subtracting from 1 here is always correct, since even for texture coordinates from oversized nodeboxes, it stays aligned to the [0,1] range, and always has scale 1.
(visual_scale changes the mesh scale and not the texture coords scale.)
So we are not subtracting from the corresponding entry in txc.

[sfan5]

to check UV coords:

diff --git a/src/client/meshgen/collector.cpp b/src/client/meshgen/collector.cpp
--- a/src/client/meshgen/collector.cpp
+++ b/src/client/meshgen/collector.cpp
@@ -25,8 +25,9 @@ void MeshCollector::append(const TileLayer &layer, const video::S3DVertex *verti
 
        u32 vertex_count = p.vertices.size();
        for (u32 i = 0; i < numVertices; i++) {
+               v2f uv{core::clamp(vertices[i].TCoords.X, 0.f, 1.f), core::clamp(vertices[i].TCoords.Y, 0.f, 1.f)};
                p.vertices.emplace_back(vertices[i].Pos + offset, vertices[i].Normal,
-                               vertices[i].Color, vertices[i].TCoords);
+                               vertices[i].Color, uv);
                m_bounding_radius_sq = std::max(m_bounding_radius_sq,
                                (vertices[i].Pos - m_center_pos).getLengthSQ());
        }

[sfan5]

world-aligned coords have shifted by one node, but the new one is actually aligned properly:

^ old | new v

[sfan5]

checked:

• world-aligned nodes
• facedir
• nodeboxes
• gromit test mod from #12266

[sfan5]

this could probably be generalized by performing maths on the normal vector of the tile (do we have that?) but whatever

[Desour]

glasslike_framed is broken. (Also uses drawAutoLightedCuboid and expects nodebox-like uv.)

Found no issues otherwise (also tested with tileable_vertical, it works now. 🎉 ). Thanks!

[Desour]

Btw. you could easily fix scale being limited to int fractions of 16 (currently it's mesh chunk size dependent then), by adding blockpos_nodes here. (Maybe follow up PR.)

[cx384]

Oh, I must have thought that glasslike_framed is just a normal node mesh, should be fixed now.

[grorp]

Some meshgen tests were added in #13616. Would testing the texture coordinates there be possible?

also tested with tileable_vertical, it works now. 🎉

Hmm, it doesn't seem to be working for me, Devtest "Dirt with Snow" still appears to use repeat instead of clamp. With texture filtering enabled, and texture_min_size disabled by source code change (also visible without, but this makes it much more obvious):

luanti/games/devtest/mods/basenodes/init.lua

Lines 37 to 45 in 486fb7c

	core.register_node("basenodes:dirt_with_snow", {
	description = "Dirt with Snow",
	tiles ={"basenodes_dirt_with_snow.png",
	-- a little dot on the bottom to distinguish it from dirt
	"default_dirt.png^basenodes_dirt_with_snow_bottom.png",
	{name = "default_dirt.png^default_snow_side.png",
	tileable_vertical = false}},
	groups = {crumbly=3, soil=1},
	})

(Feel free to consider things out of scope.)

[cx384]

Maybe, I'm not very familiar with our unittests.
I suppose it would be a bit much for this PR, it can always be added later, and if we add tests it should probably be done for all draw types, not only cuboids.

I haven't looked into tileable_vertical and tileable_horizontal maybe Desour knows.
With this PR, texture coordinates are inside the [0,1] range, so at least tiles that have not wrap mode GL_REPEAT should not be completely broken.

[grorp]

Maybe, I'm not very familiar with our unittests.
I suppose it would be a bit much for this PR, it can always be added later, and if we add tests it should probably be done for all draw types, not only cuboids.

Some unit tests are better than no unit tests :)
But yeah, it's fair if you want to consider that out of scope for this PR.

I haven't looked into tileable_vertical and tileable_horizontal maybe Desour knows.
With this PR, texture coordinates are inside the [0,1] range, so at least tiles that have not wrap mode GL_REPEAT should not be completely broken.

Yeah, this is definitely a step in the right direction to get that working.
I think the remaining brokenness might only apply to "solid" nodes because the code for them just overrides the tile material flags:

luanti/src/client/content_mapblock.cpp

Lines 429 to 434 in 486fb7c

	getTile(tile_dirs[face], &tiles[face]);
	for (auto &layer : tiles[face].layers) {
	if (backface_culling)
	layer.material_flags |= MATERIAL_FLAG_BACKFACE_CULLING;
	layer.material_flags |= MATERIAL_FLAG_TILEABLE_HORIZONTAL;
	layer.material_flags |= MATERIAL_FLAG_TILEABLE_VERTICAL;

Presumably this was done as a hacky fix for everything turning broken with tileable_vertical/horizontal=false, and is no longer needed after the fixes in this PR. (The flags are regularly set here:

luanti/src/nodedef.cpp

Lines 696 to 705 in 486fb7c

	// Material flags
	layer->material_flags = 0;
	if (backface_culling)
	layer->material_flags |= MATERIAL_FLAG_BACKFACE_CULLING;
	if (tiledef.animation.type != TAT_NONE)
	layer->material_flags |= MATERIAL_FLAG_ANIMATION;
	if (tiledef.tileable_horizontal)
	layer->material_flags |= MATERIAL_FLAG_TILEABLE_HORIZONTAL;
	if (tiledef.tileable_vertical)
	layer->material_flags |= MATERIAL_FLAG_TILEABLE_VERTICAL;

)

EDIT: looks like I was right :)
#6903 (comment)
d80d2c8
(later merged as #13216)

[sfan5]

While unit testing meshgen is good I think we would also benefit from having a static scene that can be rendered in CI and the screenshot compared to a known good snapshot.

[Desour]

also tested with tileable_vertical, it works now. 🎉

Hmm, it doesn't seem to be working for me

I meant that it doesn't completely break nodes anymore.
But yes it's force-disabled for solid nodes. (I should know this, but I completely forgot it. x))

Fixing this can also be done in another PR. 🤷

Unittests would of course always be nice. I approve anyways.

[Desour]

I found a small issue: glasslike used to just take the lower 1/scale portion of the texture, now it uses the whole texture.
In practice I think this only matters for glasslike_framed_optional when you want to use world aligned with scale frames (does anyone do this? x)), and want to have a proper fallback for normal glasslike.

It would be nice if we could also properly support glasslike and flowingliquid, and the shiny part of glasslike_framed.

[sfan5]

Adding to next release due to high-risk bug fix (IMO).

[cx384]

According to our documentation, align_style is only supported for solid nodes and nodeboxes, not sure if glasslike and glasslike_framed count as solid. And scale is documented to work for world-aligned textures only.

If I'm not wrong, according to our code, glasslike does not support align_style and it also did not support it before this PR. So I guess it is safe to assume that glasslike_framed_optional does not support it, even if glasslike_framed (maybe undocumented) does. If we want to it change it, better support align_style for glasslike nodes, instead of introducing a fallback. To be honest, glasslike_framed_optional gets rarely used and align_style even more rarely. Those two combined is very unlikely, so it's probably not worth supporting for now.