minetest / minetest

Minetest is an open source voxel game-creation platform with easy modding and game creation
https://www.minetest.net/
Other
10.42k stars 1.98k forks source link

Placing nodes very rarely replaces non-buildable_to nodes. #1011

Closed Warr1024 closed 10 years ago

Warr1024 commented 10 years ago

On a couple of recent occasions, I've gone to place a node against a surface, only to have it replace the non-buildable_to node I was pointing at, instead of being placed on top.

For instance:

I have not yet found a way to reliably reproduce the problem.

PilzAdam commented 10 years ago

This reminds me to #529

0gb-us commented 10 years ago

Me too. I hope that dead bug isn't coming back to haunt us.

ShadowNinja commented 10 years ago

What game and mods were you using? A game called Realtest redefines minetest.item_place_node, which would cause this.

Warr1024 commented 10 years ago

My world.mt below. I'm running IRC (https://github.com/kaeza/minetest-irc), mesecons (http://mesecons.net), sztest (my own WIP modpack at https://gitorious.org/sztest), and a few small local hacks and tweaks that don't do much.

Nothing that I'm aware of modifies item_place_node, though I do have a wrapper that temporarily disables add_node and remove_node during one call in sz_bucket's init.lua.

gameid = minetest
backend = leveldb
load_mod_irc = true
load_mod_sz_rotary = true
load_mod_sz_util = true
load_mod_sz_heat = true
load_mod_sz_items = true
load_mod_sz_bucket = true
load_mod_sz_stairs = true
load_mod_sz_slime = true
load_mod_mesecons = true
load_mod_mesecons_alias = true
load_mod_mesecons_blinkyplant = true
load_mod_mesecons_button = true
load_mod_mesecons_commandblock = true
load_mod_mesecons_compatibility = true
load_mod_mesecons_delayer = true
load_mod_mesecons_detector = true
load_mod_mesecons_extrawires = true
load_mod_mesecons_gates = true
load_mod_mesecons_hydroturbine = true
load_mod_mesecons_insulated = true
load_mod_mesecons_lamp = true
load_mod_mesecons_lightstone = true
load_mod_mesecons_luacontroller = false
load_mod_mesecons_materials = true
load_mod_mesecons_microcontroller = true
load_mod_mesecons_movestones = true
load_mod_mesecons_mvps = true
load_mod_mesecons_noteblock = true
load_mod_mesecons_pistons = true
load_mod_mesecons_powerplant = true
load_mod_mesecons_pressureplates = true
load_mod_mesecons_random = true
load_mod_mesecons_receiver = true
load_mod_mesecons_solarpanel = true
load_mod_mesecons_switch = true
load_mod_mesecons_textures = true
load_mod_mesecons_torch = true
load_mod_mesecons_walllever = true
load_mod_fluidhack = true
load_mod_haxtool = true
ShadowNinja commented 10 years ago

You use on_place in sz_rotary and set a node. You use minetest.get_node(pos) in sz_util.node_get, which returns ignore(a buildable_to node) if the position is unloaded, instead of minetest.get_node_or_nil which returns nil if it is unloaded. You even have a ~= nil check, although it will never be nil with minetest.get_node. Were you placing this node or a similar node that uses on_place? If so then changing node_get to use minetest.get_node_or_nil should fix it.

davidgumberg commented 10 years ago

This should be fixed by https://github.com/minetest/minetest_game/pull/29 , no?

ShadowNinja commented 10 years ago

@davidgumberg: No, the bucket was again fixed by minetest/minetest_game@1a9362afed0ad5092327ce8c8d5eaa1ad9cc987c but this only fixes it when the player is holding a bucket. sz_rotary has to be fixed to fix this particular case.

Warr1024 commented 10 years ago

Makes sense, and I'll be sure to apply that fix to sz_rotary, but wouldn't that only affect the nodes whose on_place was being called? i.e. it wouldn't affect me pouring water from a bucket or placing a cactus, as per the original bug report, would it?

ShadowNinja commented 10 years ago

No, you will only get this bug if you are weilding a tool that overrides on_place and uses minetest.get_node. Nodes that use the default on_place (cactus) and tools that use minetest.get_node_or_nil (bucket) won't have this issue. Also, make sure to check for protection if it is relevant. I'll close this because it seems to be solved.

Warr1024 commented 10 years ago

What has been solved so far are a different issue in sz_rotary (which never actually manifested as buggy behavior), and only part of the original bug (affecting the bucket, but not cactus). This issue should probably be re-opened, unless you intended this as a !repro or won't-fix...

ShadowNinja commented 10 years ago

You are still experiencing this with placing things that don't override on_place or that override it properly?

Warr1024 commented 10 years ago

I only ever had this happen once with a cactus block, and since then I haven't had much opportunity to reproduce it. I had only ever seen this happen once with a cactus block (and maybe 1 or 2 times with the bucket) so I don't know how frequently to expect it. I haven't seen anything yet that would lead me to believe that the situation has changed since it happened...

ShadowNinja commented 10 years ago

The bucket is fixed now, was the cactus using mietest.rotate_and_place or a similar method?

Warr1024 commented 10 years ago

This might have been right after the cactus 6d facedir was implemented. Unfortunately, it's been so long that I don't know if I can confirm what version of minetest_game this happened with.

It sounds like there's a general problem with the way ignores are handled right now. The fact that they're buildable_to and that you have to look out for them explicitly is really non-intuitive, and it sounds like mod authors (including the devs for minetest_game) get easily tripped up by that. It may be worthwhile to find a general solution for the way ignores are handled right now.

For instance, it looks like ignores are buildable_to to prevent people from accidentally explicitly placing them, then being unable to remove them. Maybe ignores should just not be buildable_to, and have then be automatically replaced by air in blocks as they're loaded. How often are ignore nodes actually explicitly placed inside mapblocks, anyway?

Alternatively (or additionally), add a minetest.build_node API that includes the buildable_to check (and possibly other sensible common checks), prevents building to ignores, and returns success/fail as a boolean. Having an API like that could simplify a lot of mod code for custom node placement (which creates an further incentive to use it over low-level placement code) and also ensure correctness of the relevant checks.

It also makes me wonder why a node that's only about 4 meters away from a player could be unloaded...