Help modders deal with object invalidation

[appgurueu]

Implements part of what I have outlined in #14687 (related: #13297):

• "Fixes" raycasts: Invalid objects should be skipped.
• Adds a method is_valid to idiomatically check for object validity.
• Adds iterator variants of get_objects_(inside_radius|in_area), which skip invalid objects (and are also more convenient to use).

[sfan5]

Adding a method does not qualify as bugfix, but the first two commits would be useful for 5.9 so how about splitting it?

• Adds iterator variants of get_objects_(inside_radius|in_area), which skip invalid objects

But the function already skips invalid objects, so this is only convenience, right?

[rubenwardy]

But the function already skips invalid objects, so this is only convenience, right?

The current function returns a table of objects. Some of these objects may be invalidated between building the table and reaching the object in the for loop

[sfan5]

The current function returns a table of objects. Some of these objects may be invalidated between building the table and reaching the object in the for loop

Ah, that's easy to miss. The docs should explain the benefit of the new functions more clearly.

[appgurueu]

I agree that it may be debatable whether the new iterator variants (and :is_valid) are a "bug fix", but they are certainly helpful in eliminating bugs in mod code. (I also think that the implementation is simple enough that there is little risk in it.)

I can split if you want me to, though.

[sfan5]

After finding out that it's not just a simple helper but rather fixes an easy to miss edge case I'm okay with keeping it as a bug fix.

[Zughy]

Why not simply overwrite the current get functions? Like, I can't think of any useful case where mods want to iterate invalid objects as well. On the contrary, I always have to put checks in my for loops in order to avoid crashes

[appgurueu]

Why not sinply overwrite the current get functions?

Their API unfortunately makes this infeasible: They return a "list" of object refs.

Now the edge case happens when you do something like this, for example:

for _, obj in ipairs(minetest.get_objects_inside_radius(...)) do
    obj:punch(...)
end

Because punching an object can invalidate other objects in the list (such as children) via side effects. The problem is that these objects are already in the table, so there's no (proper) way to get rid of them after the fact. They will be visited in the traversal.

With an iterator, this becomes cleaner, and the iterator can now check for and skip invalid objects. But this obviously requires modders to use the iterator rather than the table variant, hence is a "breaking" change and requires the addition of a new function which does the right thing.

[sfan5]

Fine by me.

[Zughy]

Wouldn't this fix the ghost entities issue completely?

[appgurueu]

Wouldn't this fix the ghost entities issue completely?

Ultimately, I don't know. It fixes ghost entity issues related to this mistake, if the API is used properly.

I would say it would probably make the issue a "possible close" under suspicion that this is the (main?) root cause for the issue.

I can't rule out the possibility that there are other places where "ghost entities" may appear, though, i.e. that there is an underlying bug not fixed by this.