Tool specific pointing and blocking pointable type

[cx384]

Goal of the PR

• Fixes Make some nodes pointable using only specific tools #13183

How does the PR work?

• This PR makes it possible to specify pointabilities in the item definition, to accomplish tool specific pointable properties different to what is specified in the Item or Object definition.
• As suggested in the issue it also adds a blocking pointable type which is not selectable and not point through.
• The API for the new pointabilities field in the item definition looks like this:

pointabilities = {
	nodes = {
		["default:stone"] = "blocking", 
		["group:leaves"] = false,
	},
	objects = {
		["modname:entityname"] = true,
		["group:ghosty"] = true,
	}
 },

(The API has been discuses and should be fine.)

• It contains lists to override the 'pointable' property of pointed nodes and objects. The index can be a node/entity name or a group with the prefix "group:". (For objects armor_groups are used.) If multiple fields fit, a priority order is applied.

• The liquids_pointable is kept as is. It could be marked as deprecated and refer to using groups instead, but usually nodes with the liquids drawtype are not assigned to a liquids group. So I guess it can stay as a semi-useful feature.

Does this relate to a goal in the roadmap?

• 2.1 Rendering/Graphics improvements (Because of the blocking pointable type you don't have to show selection boxes anymore.)
• 2.4 Object and entity improvements

Some concrete Usecases

• MTG firefliyes that are only pointable with the net.
• An axe that can point through leaves.
• In the CTF game the barrier at the end of the map should be point blocking such that you can't dig nodes behind the barrier and can't use it to build a staircase into the sky.
• Make lava point blocking, since you shouldn't be able to point through such a hot liquid.
• Let buckets only point source nodes and no flowing liquids.

To do

Ready for review.

• Get an approval how the pointabilities property should look like
• Change the code and documentation accordingly

It is waiting for feature freeze of version 5.8.0 to be done. 5.8.0 got released
(Maybe some followup PRs, to generally refactor code and move properties of the item definition (like range) into tool_capabilities to allow dynamic change, should be done.)

How to test

• Start devtest
• Place some "Pointable Node", "Not Pointable Node" and "Blocking Pointable Node"
• Spawn some "testentities:pointable" entities
• Get the items "Blocked Pointing Staff" and "Ultimate Pointing Staff"
• Start pointing

[Desour]

Thanks! Should be mostly fine otherwise.

• Please add a feature flag in core.features.
• Would it make sense to trigger on_rightclick and on_punch callbacks for blocking nodes, with pointability in pointed thing? This would be useful to e.g. implement lava that hurts when you try touching it.
• Pointabilities are not checked server-side, right? (I.e. a modified client can trigger an on_punch of a non pointable node.) It might be worth writing this down in the doc.
• (Haven't tested thoroughly yet (e.g. with multiple players, and performance impact).)

[cx384]

Please add a feature flag in core.features.

Done.

Would it make sense to trigger on_rightclick and on_punch callbacks for blocking nodes, with pointability in pointed thing? This would be useful to e.g. implement lava that hurts when you try touching it.

I don't think this would make much sense. The whole purpose of blocking-pointable is that it can't be interacted with, like non-pointable. (For example you can't use it as surface to place nodes on it.)
Also, I don't think it would be good to have interactable nodes that don't show a selection box.
For your use case you could just make lava pointable, or we may need a new feature to define a function which takes into account which nodes are in the pointing raycast.
(You could for example also use it to make a splash sound when you point through water and air at the same time.)

Pointabilities are not checked server-side, right? (I.e. a modified client can trigger an on_punch of a non pointable node.) It might be worth writing this down in the doc.

Done.

(Haven't tested thoroughly yet (e.g. with multiple players, and performance impact).)

I have tested it a little bit but haven't made benchmarks, the performance impact should be negligible.

Thanks for the review, I hope I solved everything appropriately and didn't miss anything.

[cx384]

It's rebased now.

[Desour]

Sorry for letting you wait!

Also, I don't think it would be good to have interactable nodes that don't show a selection box.

👍

[Desour]

Tested thoroughly, found no issues. 🎉

Just one thing: Please modify the testtools (remover, branding iron, ...) in devtest to make them able to point the test entities.

One limitation that I've noticed is that lua raycasts don't support pointabilities yet. But I would put that in another PR, because this limitation is not new, i.e. lua raycasts can not iterate through air or other non-pointable nodes in master either.

Tested:

• Nodes.
• Luaentities.
• Other players.
• Lua raycast, with the help of luacmd:
  /lua local pos1 = me:get_pos():offset(0, me:get_properties().eye_height, 0) local raycast = core.raycast(pos1, pos1 + me:get_look_dir()*4, true, true) for pt in raycast do if pt.type == "node" then print(core.get_node(pt.under).name) elseif pt.type == "object" then if pt.ref:is_player() then print("player") else print("luaent") end else print("unknown") end end
  Formatted version:

local pos1 = me:get_pos():offset(0, me:get_properties().eye_height, 0)
local raycast = core.raycast(pos1, pos1 + me:get_look_dir()*4, true, true)
for pt in raycast do
 if pt.type == "node" then
  print(core.get_node(pt.under).name)
 elseif pt.type == "object" then
  if pt.ref:is_player() then print("player") else print("luaent") end
 else
  print("unknown")
 end
end

(This would be nice to have as a testtool, for testing lua raycasts in general, btw..)

[cx384]

Just one thing: Please modify the testtools (remover, branding iron, ...) in devtest to make them able to point the test entities.

OK, now all testtools that work on objects can point Pointable Test Objects.
Additionally, testtools that work on nodes can now also point Pointable Test Nodes, e.g. you can use the remover on the Blocking Pointable Node.

[Desour]

Thanks!

[appgurueu]

Code-only review so far. I still have to test this properly, but it looks good so far; my comments are all rather minor, nothing critical.

Throughout the code, I see duplications for object / node pointabilities which effectively use the same structure (groups + names). Why not abstract that with a class?

A metadata API (like for tool capabilities) would be nice for this, but that can come later.

[cx384]

Thanks for the review.
I also removed your merge commit and rebased properly, since this is what the CONTRIBUTING.md says.

I hope I addressed everything appropriately.

Throughout the code, I see duplications for object / node pointabilities which effectively use the same structure (groups + names). Why not abstract that with a class?

If I understudy you correctly you want to have a class consisting of two unordered maps, one for groups and one for names, with then provides some functions.
For now I don't think it is worth adding another abstraction layer, since it is only used for pointabilities and armor groups are somewhat controversial and very different from node groups. Also in the future other things may be added solely for nodes or objects (like depending on the drawtype).
Moreover, I use a matchGroups function so the duplication is already very minimal.

Imo it would be more important to add more abstractions to the object class code, since there is a lot of duplication in server active objects and client active objects.

[appgurueu]

Thanks for the PR! Tested everything ({hand, blocking pointable staff, ultimate pointable staff, test tools ...} x pointable nodes x pointable entities), works as expected.

Nitpick I found while testing: The light testtool and node meta privatizer can't be used on the blocking and non pointable test node. The latter is probably completely irrelevant; you might want to change the former, but I don't have any strong feelings on this as I don't think the light levels of pointable test nodes are particularly interesting.