From d4e8da10416944e566168ab3114ba1abd34f0cef Mon Sep 17 00:00:00 2001 From: Lars Mueller Date: Fri, 21 Jun 2024 18:19:11 +0200 Subject: [PATCH 1/8] Add (failing) unit test for raycasts w.r.t. entities --- games/devtest/mods/unittests/entity.lua | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/games/devtest/mods/unittests/entity.lua b/games/devtest/mods/unittests/entity.lua index 565dd6adf..6cc207234 100644 --- a/games/devtest/mods/unittests/entity.lua +++ b/games/devtest/mods/unittests/entity.lua @@ -130,3 +130,27 @@ local function test_entity_attach(player, pos) obj:remove() end unittests.register("test_entity_attach", test_entity_attach, {player=true, map=true}) + +core.register_entity("unittests:raycastable", { + initial_properties = { + hp_max = 1, + visual = "upright_sprite", + textures = { "no_texture.png" }, + static_save = false, + }, +}) + +local function test_entity_raycast(_, pos) + local obj1 = core.add_entity(pos, "unittests:raycastable") + local obj2 = core.add_entity(pos:offset(1, 0, 0), "unittests:raycastable") + local raycast = core.raycast(pos:offset(-1, 0, 0), pos:offset(2, 0, 0), true, false) + for pt in raycast do + if pt.type == "object" then + assert(pt.ref == obj1) + obj2:remove() + obj1 = nil -- object should be hit exactly one + end + end + assert(obj1 == nil) +end +unittests.register("test_entity_raycast", test_entity_raycast, {map=true}) From 0ad415c18d8db796a2a4a0a71adb25b478f956f1 Mon Sep 17 00:00:00 2001 From: Lars Mueller Date: Fri, 21 Jun 2024 18:20:01 +0200 Subject: [PATCH 2/8] Skip invalid objects in raycasts --- src/script/lua_api/l_env.cpp | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/script/lua_api/l_env.cpp b/src/script/lua_api/l_env.cpp index 06dc27b2d..2b3d15bea 100644 --- a/src/script/lua_api/l_env.cpp +++ b/src/script/lua_api/l_env.cpp @@ -155,6 +155,7 @@ void LuaLBM::trigger(ServerEnvironment *env, v3s16 p, int LuaRaycast::l_next(lua_State *L) { GET_PLAIN_ENV_PTR; + ServerEnvironment *senv = dynamic_cast(env); bool csm = false; #ifndef SERVER @@ -163,7 +164,17 @@ int LuaRaycast::l_next(lua_State *L) LuaRaycast *o = checkObject(L, 1); PointedThing pointed; - env->continueRaycast(&o->state, &pointed); + for (;;) { + env->continueRaycast(&o->state, &pointed); + if (pointed.type != POINTEDTHING_OBJECT) + break; + if (!senv) + break; + const auto *obj = senv->getActiveObject(pointed.object_id); + if (obj && !obj->isGone()) + break; + // skip gone object + } if (pointed.type == POINTEDTHING_NOTHING) lua_pushnil(L); else From 08102756401e5c45d159fbdde0bcee084e13fe42 Mon Sep 17 00:00:00 2001 From: Lars Mueller Date: Fri, 21 Jun 2024 18:42:55 +0200 Subject: [PATCH 3/8] Add `ObjectRef:is_valid` method --- doc/lua_api.md | 4 ++++ games/devtest/mods/unittests/entity.lua | 4 ++-- src/script/lua_api/l_object.cpp | 9 +++++++++ src/script/lua_api/l_object.h | 3 +++ 4 files changed, 18 insertions(+), 2 deletions(-) diff --git a/doc/lua_api.md b/doc/lua_api.md index b1412ca85..37dfb8004 100644 --- a/doc/lua_api.md +++ b/doc/lua_api.md @@ -7815,6 +7815,10 @@ child will follow movement and rotation of that bone. ### Methods +* `is_valid()`: returns whether the object is valid. + Objects may be invalidated either through explicit removal, + or implicitly between server steps by the engine. + **The below methods should only be called for valid objects.** * `get_pos()`: returns position as vector `{x=num, y=num, z=num}` * `set_pos(pos)`: * Sets the position of the object. diff --git a/games/devtest/mods/unittests/entity.lua b/games/devtest/mods/unittests/entity.lua index 6cc207234..71f8ec32a 100644 --- a/games/devtest/mods/unittests/entity.lua +++ b/games/devtest/mods/unittests/entity.lua @@ -71,13 +71,13 @@ local function test_entity_lifecycle(_, pos) -- with binary in staticdata local obj = core.add_entity(pos, "unittests:callbacks", "abc\000def") + assert(obj:is_valid()) check_log({"on_activate(7)"}) obj:set_hp(0) check_log({"on_death(nil)", "on_deactivate(true)"}) - -- objectref must be invalid now - assert(obj:get_velocity() == nil) + assert(not obj:is_valid()) end unittests.register("test_entity_lifecycle", test_entity_lifecycle, {map=true}) diff --git a/src/script/lua_api/l_object.cpp b/src/script/lua_api/l_object.cpp index bc5ddba5c..6baf146e5 100644 --- a/src/script/lua_api/l_object.cpp +++ b/src/script/lua_api/l_object.cpp @@ -19,6 +19,7 @@ with this program; if not, write to the Free Software Foundation, Inc., #include "lua_api/l_object.h" #include +#include #include "lua_api/l_internal.h" #include "lua_api/l_inventory.h" #include "lua_api/l_item.h" @@ -106,6 +107,13 @@ int ObjectRef::l_remove(lua_State *L) return 0; } +// is_valid(self) +int ObjectRef::l_is_valid(lua_State *L) +{ + lua_pushboolean(L, getobject(checkObject(L, 1)) != nullptr); + return 1; +} + // get_pos(self) int ObjectRef::l_get_pos(lua_State *L) { @@ -2646,6 +2654,7 @@ const char ObjectRef::className[] = "ObjectRef"; luaL_Reg ObjectRef::methods[] = { // ServerActiveObject luamethod(ObjectRef, remove), + luamethod(ObjectRef, is_valid), luamethod_aliased(ObjectRef, get_pos, getpos), luamethod_aliased(ObjectRef, set_pos, setpos), luamethod(ObjectRef, add_pos), diff --git a/src/script/lua_api/l_object.h b/src/script/lua_api/l_object.h index 73264db10..3e6d01201 100644 --- a/src/script/lua_api/l_object.h +++ b/src/script/lua_api/l_object.h @@ -67,6 +67,9 @@ class ObjectRef : public ModApiBase { // remove(self) static int l_remove(lua_State *L); + // is_valid(self) + static int l_is_valid(lua_State *L); + // get_pos(self) static int l_get_pos(lua_State *L); From be14c86320ce475e4d59aa9a178d629580c1d869 Mon Sep 17 00:00:00 2001 From: Lars Mueller Date: Fri, 21 Jun 2024 19:08:13 +0200 Subject: [PATCH 4/8] Add object inside radius / area iterators --- builtin/game/misc.lua | 26 +++++++++++++++++++++++ doc/lua_api.md | 16 +++++++++----- games/devtest/mods/unittests/entity.lua | 28 ++++++++++++++++++++++--- 3 files changed, 62 insertions(+), 8 deletions(-) diff --git a/builtin/game/misc.lua b/builtin/game/misc.lua index dc394a00c..a8a6700f9 100644 --- a/builtin/game/misc.lua +++ b/builtin/game/misc.lua @@ -272,3 +272,29 @@ function core.get_globals_to_transfer() } return all end + +do + local function valid_object_iterator(objects) + local i = 0 + local function next_valid_object() + i = i + 1 + local obj = objects[i] + if obj == nil then + return + end + if obj:is_valid() then + return obj + end + return next_valid_object() + end + return next_valid_object + end + + function core.objects_inside_radius(center, radius) + return valid_object_iterator(core.get_objects_inside_radius(center, radius)) + end + + function core.objects_in_area(min_pos, max_pos) + return valid_object_iterator(core.get_objects_in_area(min_pos, max_pos)) + end +end diff --git a/doc/lua_api.md b/doc/lua_api.md index 37dfb8004..bad61832f 100644 --- a/doc/lua_api.md +++ b/doc/lua_api.md @@ -6123,12 +6123,18 @@ Environment access * Items can be added also to unloaded and non-generated blocks. * `minetest.get_player_by_name(name)`: Get an `ObjectRef` to a player * Returns nothing in case of error (player offline, doesn't exist, ...). -* `minetest.get_objects_inside_radius(pos, radius)` - * returns a list of ObjectRefs. +* `minetest.get_objects_inside_radius(center, radius)` + * returns a list of ObjectRefs * `radius`: using a Euclidean metric -* `minetest.get_objects_in_area(pos1, pos2)` - * returns a list of ObjectRefs. - * `pos1` and `pos2` are the min and max positions of the area to search. + * deprecated, use `objects_inside_radius` instead to get only valid objects +* `minetest.objects_inside_radius(center, radius)` + * returns an iterator of valid objects + * example: `for obj in minetest.objects_inside_radius(center, radius) do obj:punch(...) end` +* `minetest.get_objects_in_area(min_pos, max_pos)` + * returns a list of ObjectRefs + * `min_pos` and `max_pos` are the min and max positions of the area to search. +* `minetest.objects_inside_area(min_pos, max_pos)` + * returns an iterator of valid objects * `minetest.set_timeofday(val)`: set time of day * `val` is between `0` and `1`; `0` for midnight, `0.5` for midday * `minetest.get_timeofday()`: get time of day diff --git a/games/devtest/mods/unittests/entity.lua b/games/devtest/mods/unittests/entity.lua index 71f8ec32a..a6ce7c020 100644 --- a/games/devtest/mods/unittests/entity.lua +++ b/games/devtest/mods/unittests/entity.lua @@ -131,7 +131,7 @@ local function test_entity_attach(player, pos) end unittests.register("test_entity_attach", test_entity_attach, {player=true, map=true}) -core.register_entity("unittests:raycastable", { +core.register_entity("unittests:dummy", { initial_properties = { hp_max = 1, visual = "upright_sprite", @@ -141,8 +141,8 @@ core.register_entity("unittests:raycastable", { }) local function test_entity_raycast(_, pos) - local obj1 = core.add_entity(pos, "unittests:raycastable") - local obj2 = core.add_entity(pos:offset(1, 0, 0), "unittests:raycastable") + local obj1 = core.add_entity(pos, "unittests:dummy") + local obj2 = core.add_entity(pos:offset(1, 0, 0), "unittests:dummy") local raycast = core.raycast(pos:offset(-1, 0, 0), pos:offset(2, 0, 0), true, false) for pt in raycast do if pt.type == "object" then @@ -154,3 +154,25 @@ local function test_entity_raycast(_, pos) assert(obj1 == nil) end unittests.register("test_entity_raycast", test_entity_raycast, {map=true}) + +local function test_object_iterator(pos, iterator) + local obj1 = core.add_entity(pos, "unittests:dummy") + local obj2 = core.add_entity(pos, "unittests:dummy") + -- As soon as we find one of the objects, we invalidate the other. + for obj in iterator do + assert(obj:is_valid()) + if obj == obj1 then + obj2:remove() + elseif obj == obj2 then + obj1:remove() + end + end +end + +unittests.register("test_objects_inside_radius", function(_, pos) + test_object_iterator(pos, minetest.objects_inside_radius(pos, 1)) +end, {map=true}) + +unittests.register("test_objects_in_area", function(_, pos) + test_object_iterator(pos, minetest.objects_in_area(pos:offset(-1, -1, -1), pos:offset(1, 1, 1))) +end, {map=true}) From 89079a66194e3c83022285a6e4a5404b8ee4b124 Mon Sep 17 00:00:00 2001 From: Lars Mueller Date: Fri, 21 Jun 2024 19:56:25 +0200 Subject: [PATCH 5/8] Fix up documentation --- doc/lua_api.md | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/doc/lua_api.md b/doc/lua_api.md index bad61832f..ac22fe4ad 100644 --- a/doc/lua_api.md +++ b/doc/lua_api.md @@ -6126,14 +6126,19 @@ Environment access * `minetest.get_objects_inside_radius(center, radius)` * returns a list of ObjectRefs * `radius`: using a Euclidean metric - * deprecated, use `objects_inside_radius` instead to get only valid objects + * **Warning**: If you modify objects (f.e. punch them), + this may cause later objects in the list (f.e. children) to become invalid. + Use `minetest.objects_inside_radius` instead to iterate only valid objects * `minetest.objects_inside_radius(center, radius)` - * returns an iterator of valid objects - * example: `for obj in minetest.objects_inside_radius(center, radius) do obj:punch(...) end` + * returns an iterator of valid objects + * example: `for obj in minetest.objects_inside_radius(center, radius) do obj:punch(...) end` * `minetest.get_objects_in_area(min_pos, max_pos)` * returns a list of ObjectRefs - * `min_pos` and `max_pos` are the min and max positions of the area to search. -* `minetest.objects_inside_area(min_pos, max_pos)` + * `min_pos` and `max_pos` are the min and max positions of the area to search + * **Warning**: If you modify objects (f.e. punch them), + this may cause later objects in the list (f.e. children) to become invalid. + Use `minetest.objects_in_area` instead to iterate only valid objects +* `minetest.objects_in_area(min_pos, max_pos)` * returns an iterator of valid objects * `minetest.set_timeofday(val)`: set time of day * `val` is between `0` and `1`; `0` for midnight, `0.5` for midday @@ -7796,13 +7801,18 @@ When you receive an `ObjectRef` as a callback argument or from another API function, it is possible to store the reference somewhere and keep it around. It will keep functioning until the object is unloaded or removed. -However, doing this is **NOT** recommended as there is (intentionally) no method -to test if a previously acquired `ObjectRef` is still valid. -Instead, `ObjectRefs` should be "let go" of as soon as control is returned from -Lua back to the engine. +However, doing this is **NOT** recommended - `ObjectRefs` should be "let go" +of as soon as control is returned from Lua back to the engine. + Doing so is much less error-prone and you will never need to wonder if the object you are working with still exists. +If this is not feasible, you can test whether an `ObjectRef` is still valid +via `object:is_valid()`. + +Getters may be called for invalid objects and will return nothing then. +All other methods should not be called on invalid objects. + ### Attachments It is possible to attach objects to other objects (`set_attach` method). @@ -7822,9 +7832,7 @@ child will follow movement and rotation of that bone. ### Methods * `is_valid()`: returns whether the object is valid. - Objects may be invalidated either through explicit removal, - or implicitly between server steps by the engine. - **The below methods should only be called for valid objects.** + * See "Advice on handling `ObjectRefs`" above. * `get_pos()`: returns position as vector `{x=num, y=num, z=num}` * `set_pos(pos)`: * Sets the position of the object. From e00f1d07633a37aaaeac48b34f748f6025ea80ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lars=20M=C3=BCller?= <34514239+appgurueu@users.noreply.github.com> Date: Sat, 22 Jun 2024 14:21:14 +0200 Subject: [PATCH 6/8] Apply suggestions from code review Co-authored-by: sfan5 --- doc/lua_api.md | 8 +++++--- games/devtest/mods/unittests/entity.lua | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/doc/lua_api.md b/doc/lua_api.md index ac22fe4ad..3f97af304 100644 --- a/doc/lua_api.md +++ b/doc/lua_api.md @@ -6126,9 +6126,11 @@ Environment access * `minetest.get_objects_inside_radius(center, radius)` * returns a list of ObjectRefs * `radius`: using a Euclidean metric - * **Warning**: If you modify objects (f.e. punch them), - this may cause later objects in the list (f.e. children) to become invalid. - Use `minetest.objects_inside_radius` instead to iterate only valid objects + * **Warning**: Any kind of interaction with the environment or other APIs + can cause later objects in the list to become invalid while you're iterating it. + (e.g. punching an entity removes its children) + It is recommend to use `minetest.objects_inside_radius` instead, which + transparently takes care of this possibility. * `minetest.objects_inside_radius(center, radius)` * returns an iterator of valid objects * example: `for obj in minetest.objects_inside_radius(center, radius) do obj:punch(...) end` diff --git a/games/devtest/mods/unittests/entity.lua b/games/devtest/mods/unittests/entity.lua index a6ce7c020..521076592 100644 --- a/games/devtest/mods/unittests/entity.lua +++ b/games/devtest/mods/unittests/entity.lua @@ -71,7 +71,7 @@ local function test_entity_lifecycle(_, pos) -- with binary in staticdata local obj = core.add_entity(pos, "unittests:callbacks", "abc\000def") - assert(obj:is_valid()) + assert(obj and obj:is_valid()) check_log({"on_activate(7)"}) obj:set_hp(0) From dc116f0a448f5b7eb845bc37894feb8ba9673ece Mon Sep 17 00:00:00 2001 From: Lars Mueller Date: Sat, 22 Jun 2024 15:26:20 +0200 Subject: [PATCH 7/8] Fix tests --- games/devtest/mods/unittests/entity.lua | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/games/devtest/mods/unittests/entity.lua b/games/devtest/mods/unittests/entity.lua index 521076592..38d026663 100644 --- a/games/devtest/mods/unittests/entity.lua +++ b/games/devtest/mods/unittests/entity.lua @@ -147,6 +147,7 @@ local function test_entity_raycast(_, pos) for pt in raycast do if pt.type == "object" then assert(pt.ref == obj1) + obj1:remove() obj2:remove() obj1 = nil -- object should be hit exactly one end @@ -155,24 +156,31 @@ local function test_entity_raycast(_, pos) end unittests.register("test_entity_raycast", test_entity_raycast, {map=true}) -local function test_object_iterator(pos, iterator) +local function test_object_iterator(pos, make_iterator) local obj1 = core.add_entity(pos, "unittests:dummy") local obj2 = core.add_entity(pos, "unittests:dummy") - -- As soon as we find one of the objects, we invalidate the other. - for obj in iterator do + assert(obj1 and obj2) + local found = false + -- As soon as we find one of the objects, we remove both, invalidating the other. + for obj in make_iterator() do assert(obj:is_valid()) - if obj == obj1 then - obj2:remove() - elseif obj == obj2 then + if obj == obj1 or obj == obj2 then obj1:remove() + obj2:remove() + found = true end end + assert(found) end unittests.register("test_objects_inside_radius", function(_, pos) - test_object_iterator(pos, minetest.objects_inside_radius(pos, 1)) + test_object_iterator(pos, function() + return core.objects_inside_radius(pos, 1) + end) end, {map=true}) unittests.register("test_objects_in_area", function(_, pos) - test_object_iterator(pos, minetest.objects_in_area(pos:offset(-1, -1, -1), pos:offset(1, 1, 1))) + test_object_iterator(pos, function() + return core.objects_in_area(pos:offset(-1, -1, -1), pos:offset(1, 1, 1)) + end) end, {map=true}) From d74ec349c7c94b8cec144c364963c99f35b96581 Mon Sep 17 00:00:00 2001 From: Lars Mueller Date: Sat, 22 Jun 2024 17:00:15 +0200 Subject: [PATCH 8/8] Minor doc adjustments --- doc/lua_api.md | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/doc/lua_api.md b/doc/lua_api.md index 3f97af304..7d18f7e37 100644 --- a/doc/lua_api.md +++ b/doc/lua_api.md @@ -6129,7 +6129,7 @@ Environment access * **Warning**: Any kind of interaction with the environment or other APIs can cause later objects in the list to become invalid while you're iterating it. (e.g. punching an entity removes its children) - It is recommend to use `minetest.objects_inside_radius` instead, which + It is recommended to use `minetest.objects_inside_radius` instead, which transparently takes care of this possibility. * `minetest.objects_inside_radius(center, radius)` * returns an iterator of valid objects @@ -6137,9 +6137,8 @@ Environment access * `minetest.get_objects_in_area(min_pos, max_pos)` * returns a list of ObjectRefs * `min_pos` and `max_pos` are the min and max positions of the area to search - * **Warning**: If you modify objects (f.e. punch them), - this may cause later objects in the list (f.e. children) to become invalid. - Use `minetest.objects_in_area` instead to iterate only valid objects + * **Warning**: The same warning as for `minetest.get_objects_inside_radius` applies. + Use `minetest.objects_in_area` instead to iterate only valid objects. * `minetest.objects_in_area(min_pos, max_pos)` * returns an iterator of valid objects * `minetest.set_timeofday(val)`: set time of day