More SAO related changes (revised)

[sfan5]

Various bug & documentation fixes related to entities.

DO NOT SQUASH

To do

This PR is a Ready for Review.

[Wuzzy2]

Can you please elaborate what this PR is good for?
This this just improved logging or is there more?

[Wuzzy2]

* The object is removed after returning from Lua, however the `ObjectRef`
 instantly becomes invalid and unusable.

What does this mean, “unusable” and “invalid”? What happens when a rogue mod tries to use the object anyway? Does the ObjectRef magically become nil? Will Minetest go “boom” when I try to use it anyway? Will a warning be written? Etc. And what can the modder do to defend against possible bugs?

Whatever the answer is, this text needs improvement as I don't fully understand it.

[sfan5]

This just improved logging or is there more?

Yes, the rest is random bugfixes and other things that were necessary.

What does this mean, “unusable” and “invalid”?

It means all methods return nil without doing anything.

Does the ObjectRef magically become nil?

This would be cool (or perhaps not), but no, that is not the case.

Will a warning be written?

I briefly looked into that but that wasn't easy to do in the code. Calling object methods after remove() is an easy to spot bug anywayn.

And what can the modder do to defend against possible bugs?

Possible bugs where?

[Wuzzy2]

It means all methods return nil without doing anything.

Ah. This definitely needs to go in the documention because if ALL functions can suddenly return nil (because a different mod removed the object to which I have a reference to), this will catch many mods by surprise, and is a bug factory. Please add a sentence somewhere that says something like: “Note: All these functions return nil and do nothing if the ObjecRef is invalid” or something like this.

There's another question about remove: Is it guaranteed that none of the object's callbacks, especially on_step is called after remove? I remember in Ye Olden Times, this sometimes happened after removing an object, on_step was still called a couple of times. But I didn't check lately.

I propose to add an is_valid function (maybe in a different PR) that returns false for invalid ObjectRefs, because it is useful for more reliable Lua code. This can allow you to skip some processor-heavy functions, or to avoid dozens of nil checks for every objectref function.

Possible bugs where?

Uh, forget it. My question has been answered.

[Wuzzy2]

To clarify: I propose to add the “functions return nil note in a more prominent place, like at the top of the “ObjectRef” method section, since that's a huge gotcha. Putting this in the remove section is correct, but not enough IMO.
Thanks.

[sfan5]

(because a different mod removed the object to which I have a reference to)

But here's the thing. If that happens either you or the other mod has already made the fundamental mistake of storing ObjectRefs.

Is it guaranteed that none of the object's callbacks, especially on_step is called after remove?

Yes.

I propose to add an is_valid function (maybe in a different PR) that returns false for invalid ObjectRefs, because it is useful for more reliable Lua code. [..]

I consider such defensive programming unnecessary and adding such a method could in fact cause the opposite. Unless you violate the scoping rules for ObjectRefs, there is absolutely no reason to test if an ObjectRef is valid because you can rely on it being valid.
If you're afraid of other mods messing up, set deprecated_lua_api_handling = log and tell the author of that other mod that he sucks (a warning will be printed).

[Wuzzy2]

But here's the thing. If that happens either you or the other mod has already made the fundamental mistake of storing ObjectRefs.

The documentation doesn't really say that this isn't allowed, right? :P

the scoping rules for ObjectRefs

Today I heard for the first time that there are scoping rules. :D I always suspected something like that, but I can't remember it being written out explicitly anywhere, and I can't find it with a quick Ctrl-F in lua_api.txt.

Lua API docs are suspiciously sparse about what you can (and can't) do with ObjectRefs.

[sorcerykid]

I have to agree with Wuzzy. To suggest that someone made the "fundamental mistake of xyz" when xyz isn't officially documented anywhere doesn't seem fair. I remember the first time I learned about this was on a random forum reply from Rubenwardy. People can't reasonably be expected to know every caveat of a complex system, only what they are told :P

[rubenwardy]

I was actually wrong, and just repeating what I was told. Storing objectrefs isn't that bad as they are gracefully cleared and won't have segfaults. However, you need to account for invalid objectrefs and they won't become valid again if the object is emerged

[sfan5]

won't have segfaults

Maybe not inside the ObjectRef code itself, but breaking implicit assumptions could certainly still segfault the engine until now (#9387) or 2016 (b8aed9d) or 2015 (ce8a9ed).

This is also not the case for anonymous ObjectRefs, though I couldn't find where these are used or needed.
But if such a ref is kept by Lua beyond the lifetime of the SAO, the engine will crash because it's working on memory that has been freed.

However, you need to account for invalid objectrefs

There is no API function for this (though several fulfill equivalent purpose) and not having one is preferable IMO.
It is much less error-prone to work with ObjectRefs inside the "intended" scope, you will never need to wonder if the object you are working on still exists.

Unfortunately some mods (such as advtrains) are forced to touch internals like core.luaentities to get their functionality working. Maybe the guarantees the engine makes about ObjectRefs could use some revising.

[Wuzzy2]

So what is a valid scope for ObjectRefs? I'm not sure what you even mean. Do you mean “scope” as in Lua?
What's an anonymous ObjectRef?
And what are the guarantees that the engine makes about ObjectRefs?
Are there other important facts about ObjectRefs, that no modders knows yet?

Please answer the questions in lua_api.txt. :-)

Unfortunately some mods (such as advtrains) are forced to touch internals like core.luaentities to get their functionality working.

But minetest.luaentities is officially documented, however, and core==minetest AFAIK. So it's not really internal. Documentation also does not warn about it being for “internal use only”. If this table is meant to be internal, documentation needs fixing.

[sorcerykid]

It is much less error-prone to work with ObjectRefs inside the "intended" scope, you will never need to wonder if the object you are working on still exists.

^ This statement right here. Please add that to the documentation. It would be extremely beneficial for modders to know. Full disclosure and all.

[sfan5]

But minetest.luaentities is officially documented [...]. Documentation also does not warn about it being for “internal use only”.

Hmm, this is kind of a problem. core.luaentities and core.object_refs are there, documented and there are some valid usescases for touching it.

Maybe this stance on ObjectRef reuse is not viable (and not necessary anyway since #9390).

[sfan5]

---

Removed the ObjectRef "misuse" tracking and instead added some advice to lua_api.txt

[sorcerykid]

Very thorough explanation! Thanks so much for adding!

[Wuzzy2]

Instead, ObjectRefs should be "let go" of as soon as control is returned from
Lua back to the engine.

Just to make sure I understand this right: As long I use an ObjectRef inside the same callback (minetest.register_on_whatever), it's safe to use (until it's removed).
As soon I'm outside of this callback, I should no longer trust this ObjectRef.
Is this correct?

[sfan5]

Assuming "inside" and "outside" mean the same thing I am thinking of, then yes.

[Wuzzy2]

Great!

[SmallJoker]

Trivial change needed, works.

[TheTermos]

However, doing this is NOT recommended as there is (intentionally) no method to test if a previously acquired ObjectRef is still valid.

@sfan5 Why is that? Why intentionally?
The only other way for one object to keep track of another I can think of is to save some data on that object and call get_objects_inside_radius repeatedly, which is as ugly as it gets.

A function to check if an object is still active would be extremely useful, but because it doesn't exist currently my hack is calling get_yaw().
Another useful thing would be get_object_by_id, probably difficult to implement because saos don't do IDs.

But there's nothing, so ObjectRefs will keep being saved, recommended or not.

[sfan5]

(This answer focuses on LuaEntities, it should be obvious why there is zero need to store player ObjectRefs.)

Why is that?

The reason is that objects can unloaded and loaded again at any point, so your ObjectRef will be invalid despite the "same" object still existing. This may or may not be what you want and will come to a surprise on unsuspecting modders who store ObjectRefs.

Why intentionally?

My opinion is that until the engine provides a way to properly keep track of another object, working around this by storing ObjectRefs should be discouraged.

The only other way for one object to keep track of another I can think of is [...]

There is another way actually.

Another useful thing would be get_object_by_id, probably difficult to implement because saos don't do IDs.

You can index core.object_refs by object ID just fine, not sure where you'd get the ID or why you would want to do this though.

[TheTermos]

Why intentionally?

My opinion is that until the engine provides a way to properly keep track of another object, working around this by storing ObjectRefs should be discouraged.

You wrote that it is intentional that there's no method to test if an ObjectRef is valid.

I don't mind if storing ObjectRefs is discouraged, but taking effort not to provide a method to test the validity is another story.
Such a method is badly needed, considering there's nothing else to get the job done efficiently and the proper way of keeping track of other objects may never happen.

[sfan5]

but taking effort not to provide a method to test the validity is another story.

Providing such a method would encourage storing ObjectRefs and also be a commitment to support that API in the future.
There are 100% reliable workarounds by checking the return value of getters that normally never return nil (get_yaw, get_armor_groups, get_velocity, get_luaentity, ...).

[TheTermos]

There are 100% reliable workarounds by checking the return value of getters that normally never return nil (get_yaw, get_armor_groups, get_velocity, get_luaentity, ...).

Yes, that what I've been using, but checking object validity isn't their purpose and that behavior can change anytime on other merits.

Anyway I don't see why there seems to be such a problem, checking validity is the first thing one learns when working with stored refs, that's no rocket science.

[sorcerykid]

but checking object validity isn't their purpose

Hmm, I'd argue that the get_luaentity() method would be a perfectly suitable and even coherent way to check whether an object is valid or not. If you can't get the LuaEntity, then the object must not exist.