Add support for static boxes in 'leveled' type; add node box types 'leveled_plantlike[_rooted]'

[Wuzzy2]

The problems

Currently, 'leveled' boxes will allways have their upper face change with param2. So this only allows you to do fairly simple things. If you need a fully static box as part of a leveled nodebox, you are out of luck.

Also, you can't get selection boxes to be in sync with leveled plantlike nodes. No matter what you do, they are always off.

The PR

This PR adds a new field 'leveled_fixed' for the nodebox type 'leveled'. This is an optional box (or boxes) for leveled-type nodeboxes, but it will be fully static.

This screenshot shows an example (it's always the same node, but with different param2): https://satoshiupload.com/images/q5VZ1MoO8I.png

Moreover, it adds two new nodebox types leveled_plantlike and leveled_plantlike_rooted. These work like leveled except they are in sync with the height of the leveled 'plant' for the drawtypes plantlike and plantlike_rooted, respectively. This allows you to make pixel-perfect selection boxes for such nodes.

The height, however, is capped at one point because leveled plantlike nodes can get very high and I'm not sure how reliable selection boxes are when they're really high.

Fixes #8202.

Use cases

• Cauldron with water of variable height (leveled_fixed is the cauldron (which is static), fixed is the water box (which varies in height))
• Trash can with something inside
• Some kind of simple ice spike
• Kelp plant with varying height with pixel-perfect selection box that respects height

Please note

Unfortunately, the naming is a little bit weird for leveled nodeboxes. For some reason, the fixed box is actually NOT fixed, since it varies in height. The new nodebox field leveled_fixed is the box (or boxes) that is ACTUALLY fixed.

It has been like that before, I cannot change that trivially w/o breaking compability. I think it is best to just keep the naming for the sake of compability, which is MUCH more important.

The nodebox types leveled_plantlike and leveled_plantlike_rooted are intended to be used as selection boxes so you can capture the plant pixel-perfectly. New nodebox types were neccessary because leveled has a step of 1/64 while leveled plantlike nodes have a step of 1/16. The new nodebox types use a step of 1/16 so it matches. leveled_plantlike_rooted is basically the same as leveled_plantlike but a height offset is applied (because of the base cube).
Also, unlike leveled, the new nodebox types do NOT restart at param2=128.

For collision boxes, they are only safe to use if you keep the upper end of the "plant" below a height of -0.5+127/64 (roughly 1.5), otherwise, collision bugs will start to occur. There is nothing in the code that enforces this limit so it's the responsibilty of the game author to make sure the "plant" doesn't become too tall. However, using these nodebox types as collision boxes is a rather esoteric use case anyway, so it should not be that big of a deal. I documented that anyway, just in case.

How to test

Use the following test nodes in DevTest. Try them together with the Param2 Tool.

• testnodes:nodebox_leveled_fixed (new node)
• testnodes:plantlike_leveled (old node, new dynamic selection box)
• testnodes:plantlike_rooted_leveled (old node, new dynamic selection box)

[Wuzzy2]

I've got a question: Would it be OK to allow selection boxes to be arbitrarily high? A leveled plantlike node (testnodes:plantlike_leveled in DevTest) can reach a maximum Y of 15.4375 (at param2=255). I have made a few tests with dropping the height limits I enforced (currently at max. Y of 3.5) and the selection box still worked flawlessly even at max. height. I can punch the node at any height just fine and if I place something at the selection box, even at max height, it works like you expect (i.e. the node is placed at the correct height, not just at the bottom).

So, can I drop the height limit safely and allow for selection boxes to go for the full plantlike height? Or is there still some edge case I might have overlooked?

Dropping the hight limit would be really neat because this means you could make e.g. algae, kelps, etc. pointable everywhere, and that pixel-perfectly.

[loffren174]

I like all of these ideas to extend the functionality of param, param2, param3(?),
especially this one, and those allowing more simultaneous rotations+color bits.
This one in particular would be especially neat for any sort of 3d-pillar-progress bar/energy bar/fluid fill status node, like making sci-fi pipes that represent a counter or energy threshold, or simply showing a player their score in a minigame with a neat pillar.
Sort of like this stock image, but rectangular:
https://images.stockfreeimages.com/592/sfixl/5925868.jpg
With this pr it would be trivial to make any voluminous fluid-holding tank represent a linear value by its color height, without sacrificing the ability to texture its borders.

[rubenwardy]

Unfortunately, this feature is not on the roadmap and hasn't received a concept approval. Under the new rules, non-roadmap non-bugfix PRs have 7 days to be concept approved, otherwise they should be closed. These PRs can also have preapproval if they fix an issue that is concept approved

If a core dev wants to support this PR, they may reopen it and apply the "Supported by core dev" label

[Wuzzy2]

I totally understand that PRs can take months or even years to be even recognized by the core devs. I understand that time is a limited resource. I also understand when bad PRs are rejected for being bad. Because eventually, with enough patience, most of my PRs came through eventually. And those that were ultimately rejected were always rejected for a reason. Even if I didn't always agree with it, at least there was a reason. The long waiting times weren't great, but at least patience pays off eventually.

But this new policy is just about murdering PRs without a single core dev having even looked at it. This is infuriating! There's no review, no consideration, no explanation, not even a single topic about the mere concept. Just a giant "Fuck you!", as if my work is not even taken serious anymore. Do you realize how condescending this looks? And no, I don't buy the excuse of "If a core dev wants to support this PR" ... It is closed now. Who is going to regularily look at the list of closed PRs?

And a week seems short. So if all or most core devs are on vacation or distracted, all PRs posted in this period are by definition already doomed. Frankly, this new policy sucks.

I am kinda pissed … But OK, I play along for now … According to the roadmap, I am supposed to post an issue before a PR. So I am going to do that … Among the >900 issues. Ooff. Fun times! I predict this will slow down development even further, because there are now two layers of approval (one for the issue, one for the PR), as if Minetest doesn't already have enough bureaucracy. ;-)

Also: How on earth is "improving drawtypes" not on the roadmap? I am utterly confused.

[v-rob]

Well, I myself feel unhappy to let this PR get closed because it seems like a really good idea, but if I add a "Supported by core dev" label, then it seems as though I'm supposed to take a high level of responsibility for it. It seems to me that it's more likely for PRs to get closed than for core devs to take more on to their plate.

But whatever. I'm not letting this get closed; I think Minetest's most important asset is nodes, so PRs to improve them are very good. I guess that basically makes me one forced reviewer.

[Wuzzy2]

I have just rebased this PR to make it testable again. I repeat my question from before:

Would it be OK to allow selection boxes to be arbitrarily high?

See #11391 (comment)

[v-rob]

I'd personally remove the limit. There's no limit to selection boxes (or collision boxes) for any other node, so it doesn't make much sense to enforce it for this type in specific. Of course, collision boxes break after a certain size, and even nodeboxes don't always display right at huge sizes, so if huge selection boxes break, it wouldn't be much of a surprise (or much of a problem either).

[Wuzzy2]

Thanks for that. I am waiting for more opinions on this.

In my tests so far, very large selectionboxes (if I disable the limit) still work fine. I'm mostly askin in case I overlook something.
Very large collisionboxes break (of course, that's not new and by design), but this is documented.

[kab0u]

I'd like to express my support for this feature, but with some caveats.

I am currently creating a "composter" node for MineClone2 and its derivatives and some aspects of this node box type might fit my purpose very well. However, it would not entirely fit my purpose, because I also need to have different textures depending on the node's level. AFAICS there is no support for having different textures mapped to varying levels in this PR.

The composter is a static box that can be filled with 7 different levels of compost. It uses three different top textures for the node, ie. a "bottom" texture for the empty composter, a "compost" texture for the various stages of filling, and a "compost with bone meal" texture when the composter is ready for harvest. The solution that I am using now is to have several different node boxes defined each with a node attribute "_compost_level" and reading that from minetest.registered_nodes[node.name][_compost_level] . Seven nodes have almost identical definitions, apart from the node box's height and the name and _compost_level attribute that have been parametrized in the node registration. The "empty" and "ready" composters have additionally different textures defined.

If I were to use the static leveled option that this PR implements, I could set the level of individual instances of composter nodes, but if I were to update the texture in the node definitions, this would affect the textures of all composter nodes placed on the map.

Hence my additional feature request for some form of level-indexed node textures. Possibly, the original leveled type could even be extended to have indexed textures, that would IMHO add even more value to this PR.

[Wuzzy2]

Please don't hijack PRs with major feature requests.

[v-rob]

Textures per nodebox (or per level) is something I've thought of myself before as there's no other way of doing that sort of thing without a model. However, it's not something for this PR, without a doubt.

(Now I really need to review this PR...)

[kab0u]

Apologies if my remarks were taken as an intent to hijack the PR. I just wanted to describe a use case beyond the trivial and suggest a possible extension/enhancement. If that is beyond the scope of this PR then so be it.

[v-rob]

I did some extreme selection box testing. I tested selection boxes with heights up to 220 nodes. If anything would break selection boxes, the camera offset would. At a height of 220, it will 100% cross mapblock boundaries and camera offset boundaries. Surprisingly, selection boxes still seem to work perfectly. The bad news: selection boxes that big obliterated my framerate. I was getting perhaps 0.5-1 FPS. It doesn't matter whether the node exists on the map or not: just having a node definition with such a selection box causes it.

Now, the highest selection box leveled plantlike_rooted could achieve (in this PR without the limit) is a height of just below 8. I didn't take any precise measurements, but there seemed to be very little difference in terms of framerate between Minetest with a selection box that big and Minetest without. In fact, I couldn't tell any difference up until after 14 or so. It might be that performance doesn't drop until selection boxes become bigger than mapblock sizes.

Bearing all these things in mind, I feel like removing the limit would be reasonable.

[Wuzzy2]

Thanks for testing. So I have increased the limit to 16.5, which is slightly above the maximum possible height of a plantlike_rooted node (measured from the center of the node pos).

It works fine for me, no issues or FPS drop. It also doesn't matter if the "plant" crosses mapblock boundaries, even if param2=255.

If you require any other changes or bugfixes, let me know!

[Zughy]

@Wuzzy2 please rebase

[Zughy]

@Wuzzy2 please rebase

[Wuzzy2]

Rebase done. The rebase was difficult again, I hope I didn't screw up. Surface-level testing looks OK.

Let's hope this PR gets merged before hitting the 2 year anniversary. XD

[Zughy]

@Wuzzy2 please rebase docs (see #13438)

[Wuzzy2]

Done.

[Zughy]

@Wuzzy2 aaaand rebase again..

[Wuzzy2]

The rebase has been completed. I also noticed the documentation was slightly wrong/outdated, so I fixed it.

[appgurueu]

Thanks for the PR.

Tested the three devtest test nodes, they work as expected - boxes match the visuals and keep matching when changing param2. Also tested with the box visualizer tool that the collision box is correct for the leveled fixed test node. The fixed boxes of e.g. stairs still work as expected.

I have some comments on the implementation, but nothing major.

(Also, rebased and fixed a compilation failure caused by a rename of a method you were using)

[appgurueu]

Shouldn't this be documented?

[appgurueu]

Could use box.MaxEdge.Y = std::min(box.MaxEdge.Y, SAFE_SELECTION_BOX_LIMIT * BS).

Also, is the safe selection box limit really that large (~16 nodes)? This would seem unlikely to me, wouldn't raycasts have to be much slower if this was the case?

[appgurueu]

Why? If this is to silence a compiler warning, why not enumerate the ignored types instead? This ensures that if someone adds a new type, they get a compiler warning if they forget to add it here.

[appgurueu]

What happens here if is is at EOF (newer client receiving nodeboxes from an older server)? leveled_fixed_count should be conveniently zero-initialized then, but there should be a comment explaining this, or even more robust, an explicit check for EOF after readU16 (in case readU16 is changed in the future to not zero-initialize anymore).

[appgurueu]

Why not switch on nodebox.type here, instead of an if-else if-else chain?

[appgurueu]

No need to have this expression as a comment, you can put it in the code as -0.5f + 127f/64f; the compiler should take care of the rest.

[appgurueu]

May I suggest: f32 max = std::max({fabsf(half_processed.MinEdge.X), ...}) (std::max with an initializer list). That should allow you to get rid of the loop.

[appgurueu]

Shouldn't this be an else if?

[appgurueu]

This seems slightly suspicious to me: What if you have a min point larger than 0, or a max point less than 0? I would initialize this box to {INFINITY, INFINITY, INFINITY, -INFINITY, -INFINITY, -INFINITY} instead.

(That said, depending on how this union is used, it might not matter if it is a bit too large and includes the origin despite not having to.)