[pls do not squash :3] Allow limiting object observers

[appgurueu]

Closes #8045 straightforwardly: By managing an opt-in set of observers per-entity; in the process I also made players use a proper std::string for their name rather than a char*.

This feature should work with newer servers and older clients as well.

To do

• API design: I am considering incremental methods add_observers / remove_observers in addition to the current set_observers / get_observers. Opinions?
  • Considered, not really necessary for a minimal feature yet. Unless someone asks for them, I won't add them; note that you can always easily implement them yourselves.
• What about static_save? The easiest solution would probably be to disallow static_save for entities with managed observers.
• What else did I fail to take into account?
  • Well well, let's find out! This PR is suspiciously simple, isn't it?

Feedback welcome!

How to test

Join a server with two clients. Spawn a testentities:observable. Punch the observable on one client: It should disappear for you and appear for the other client.

[MisterE123]

RE: static_save: Why not clear the table of players allowed to view the entity when the entity is saved, and let the entity set its viewable players when it comes back from being saved?

[appgurueu]

RE: static_save: Why not clear the table of players allowed to view the entity when the entity is saved, and let the entity set its viewable players when it comes back from being saved?

I considered this, but I believe it's problematic as it could lead to some kind of "polling" / "spurious" wake ups: Suppose a player who's not supposed to be an observer comes near an entity. Since we don't know whether this player will be set as an observer, we have to wake the entity up. The entity then sets its observers, excluding the player. We should put the entity back to sleep, since there is no nearby observer. But if we do that, this cycle repeats ad infinitum while the player is in proximity - the entity gets woken up, put back to sleep, ...

An alternative is to always load entities if any player is in proximity - be that an observer or not - and only unload them if no player is in proximity - again regardless of the observer status of the players. I'm not sure whether this is cleaner, though. (Given that I have not touched this code, I believe this should be the current state of affairs.)

Yet another alternative would be letting the engine persist observers. That way it could check the observers before waking an object up.

[sfan5]

gave the code a quick look, not exhaustive

[MisterE123]

RE: static_save: Why not clear the table of players allowed to view the entity when the entity is saved, and let the entity set its viewable players when it comes back from being saved?

I considered this, but I believe it's problematic as it could lead to some kind of "polling" / "spurious" wake ups: Suppose a player who's not supposed to be an observer comes near an entity. Since we don't know whether this player will be set as an observer, we have to wake the entity up. The entity then sets its observers, excluding the player. We should put the entity back to sleep, since there is no nearby observer. But if we do that, this cycle repeats ad infinitum while the player is in proximity - the entity gets woken up, put back to sleep, ...

An alternative is to always load entities if any player is in proximity - be that an observer or not - and only unload them if no player is in proximity - again regardless of the observer status of the players. I'm not sure whether this is cleaner, though. (Given that I have not touched this code, I believe this should be the current state of affairs.)

Yet another alternative would be letting the engine persist observers. That way it could check the observers before waking an object up.

I thought statically saved entites get woken up when the mapblock they are in loads, and put to sleep by the engine when the mapblock they are in unloads. Given that, the entity should be woken up regardless of who comes near. Are you telling me you will make custom wake/save logic that takes players who can view it into account? Does that mean that you cannot have an entity that is un-viewable by any player exist in an active state for a time?

I disagree that you should put the entity back to sleep if it is not viewable by players; that would be a hardcoded engine "feature" that would take control away from mods. Mods cnn easily use entity logic to delete or save entities if they really need to, but mods would not be able to easily work around an engine feature that automatically statically saved an unviewable entity if it was unviewable.

Usecase: an entity that is invisible to all players but which runs game logic at a particular location.

Usecase: an entity which is invisible to all or some players for a time, and at a specific time determined in on_step, shows itself to some players

Neither of these would be possible if you automatically statically saved entites when they did not have any viewers.

Your second alternative is the way entities currently work, and should be kept that way.

[appgurueu]

I thought statically saved entites get woken up when the mapblock they are in loads, and put to sleep by the engine when the mapblock they are in unloads. Given that, the entity should be woken up regardless of who comes near.

Yes, that should be the current state of this PR. I'm not sure whether that's clean though, hence why I'm discussing it.

Are you telling me you will make custom wake/save logic that takes players who can view it into account? Does that mean that you cannot have an entity that is un-viewable by any player exist in an active state for a time?

This would be the third option, yes. I don't know if I will implement this though, it's still up for discussion.

I disagree that you should put the entity back to sleep if it is not viewable by players; that would be a hardcoded engine "feature" that would take control away from mods. Mods cnn easily use entity logic to delete or save entities if they really need to [...]

Good point on the deletion, though I'm not sure on the saving (unless you mean saving entities in something like a mod-managed file rather than the map database).

[...] but mods would not be able to easily work around an engine feature that automatically statically saved an unviewable entity if it was unviewable.

When is it useful to keep an entity that can't be observed around? I feel this is wasteful. Consider something like a statically saved ghost which is supposed to haunt a specific player: You'll wake this ghost up and keep it in memory if any player comes nearby, loading the mapblock (and thus the ghost).

That said, since entity static save is tied to mapblocks, I'm not sure how the ghost could efficiently be left asleep, and I don't want this PR to get out of scope, so leaving it "as is" is indeed probably the easier option here. I suppose this should be documented, though.

(There could be a workaround for this, actually: Have an invisible parent entity with unmanaged observers; make the child have static save = false, and let the parent (re)spawn the child if necessary. But I agree that it's dirty.)

Your second alternative is the way entities currently work, and should be kept that way.

Yes, it's also what I believe this PR currently implements (I haven't tested this yet). Keeping it this way and documenting it properly is probably be the cleanest & simplest solution. What do core devs think?

[MisterE123]

Oh, wow, this allows actual player invisibility

[MisterE123]

I'm curious why you have managed players as:
{pl_name1=true, pl_name2=true}
Instead of:
{pl_name1,pl_name2}
Where only the players in the table can view the object, and an empty table makes no one able to view it, and setting observers to nil (not even an empty table) makes them unmanaged.

Given your system, I am a bit confused as to how you would set an entity to have no observers without constantly using the on_step to add player names to the table and set them to false.

[grorp]

I couldn't resist: I made a quick-and-dirty per-player hallucination mod with the set_observers API added by this PR.
https://github.com/grorp/hallucinations

Seems to work great!

Given your system, I am a bit confused as to how you would set an entity to have no observers without constantly using the on_step to add player names to the table and set them to false.

Wouldn't that be set_observers({})?

[appgurueu]

Oh, wow, this allows actual player invisibility

Yeah, from the perspective of the client players are pretty much just CAOs, so I saw no reason to disallow player invisibility - with the exception that you can't hide a player from themselves this way, since that would break various assumptions the engine makes.

I'm curious why you have managed players as: {pl_name1=true, pl_name2=true}

Because this is a set of player names: Player names are either in the set, or they aren't; they may not appear twice. A table from player names to true captures this well. It also makes tests for inclusion convenient (on the Lua side of things): You can simply do if observers[name] then ... end. I think {[element] = true, ...} is simply the most natural way to model a set in Lua most of the time.

Other engine functions (e.g. set privs) represent sets this way too.

Instead of: {pl_name1,pl_name2}

Lists are another way to represent sets (which the engine also uses), but it doesn't make much of a difference, and I don't see any significant advantages of lists (except maybe for being slightly less typing work); duplicates would also need to be dealt with (error or silently ignore?).

Where only the players in the table can view the object and an empty table makes no one able to view it

Currently the case (using player names as keys, though)

and setting observers to nil (not even an empty table) makes them unmanaged.

Yeah, that's currently the case: Doing obj:set_observers() or obj:set_observers(nil) makes them unmanaged. I suppose I should make this clearer in the docs?

Given your system, I am a bit confused as to how you would set an entity to have no observers

Same as in your proposed list format: obj:set_observers({}), as grorp said.

without constantly using the on_step to add player names to the table and set them to false.

Oh no, don't do that! It's a set, not a map from player name to whether they are an observer 😄

This is interestingly the same confusion as for setting privs ({interact = false} will set the player privileges to exactly interact, rather than revoking interact or similar). I had hoped for my docs to be less ambiguous, but oh well... I should make the code error if false is used as a value, and expand the docs.

[LoneWolfHT]

I'm running this PR on the CaptureTheFlag server, as you can see if you look at teammate nametags. I'll let OP know if there are any issues

[jordan4ibanez]

:3

[hlqkj]

Is the TAB autocomplete in the game chat also benefiting from, or affected by this?

[appgurueu]

Is the TAB autocomplete in the game chat also benefiting from, or affected by this?

In its current form, this doesn't affect TAB autocompletion or the list of names sent to the serverlist at all. Depending on the use case, this could be desirable: Imagine something like an "invisibility" feature in a CTF server. You don't want to pretend that invisible players have left to the serverlist, or disable TAB autocomplete for them. On the other hand, a "cloaking" mod intended to conceal the presence of staff will want to disable both TAB autocomplete and sending the name to the server list; not sending names to the serverlist is also a demanded feature: Servers might have bot players (like NodeCore's spectator which streams to Twitch) which they wish to hide, or players may be asking for slightly more privacy than having their online status on a Minetest server readily available in some public JSON.

In conclusion, I believe it's probably best to leave these features up to a separate PR, to keep this PR simple and focused. Should it be considered necessary to "complete" this feature however, I already have a commit prepared which adapts the player list sent to clients based on who it is sent to, but I'm not very fond of it since it conflates "can see player in-game" and "knows that player is in-game" (e.g. available for a chat), which aren't exactly the same (refer to the example use cases above). The server list is yet another issue, which deserves its own API.

TL;DR: Controlling

• Tab completion (player list sent to other clients) and
• Serverlist visibility

deserve to be their own features in follow-up PRs. They are out of scope for this PR (which is only concerned with limiting who the active objects are sent to).

[hlqkj]

Thanks for your answer! I think you are totally right. I was indeed thinking about cloak, but didn't consider the other use case...

[sfan5]

Thought: In the unmanaged state the engine should automatically "propagate" the observers to children objects.

Otherwise this feature is quite intrusive because if you want to attach something to the player (*) you suddenly have to change your code to care about observers (to not violate the conditions).

(*) or any other objects whose observer state you don't get to decide

[appgurueu]

Thanks for the review @SmallJoker. I think I have addressed it now, but some details might need further discussion.

Also relevant: #14342 would add some serverside sound tracking this somewhat depends on.

This PR currently can't play nice with attached sounds, since the engine doesn't keep (enough) track of them. It basically suffers from the same bug we've already been seeing when objects get out of range: The sound stays. Now we could fix this by making clients remove the sound, but then it would be gone if the object reappears.

For now I would add a note to the docs that this doesn't work. The main use case (visibility) won't be impacted by this. Thoughts?

[BluebirdGreycoat]

I don't know about others, but for me, a mention in the docs is just fine.

[SmallJoker]

@appgurueu I'd also say that documenting it with a one or two-liner is good enough.

[grorp]

minetest.register_entity(":dodo", {})
minetest.register_on_joinplayer(function(player)
	local obj = assert(minetest.add_entity(player:get_pos() + vector.new(0, 5, 0), "dodo"))
	obj:set_observers({})
	player:set_attach(obj)
end)

in 9/10 cases, this results in:

ERROR[Main]: In thread 7fa029eb9080:
ERROR[Main]: /mt/src/client/localplayer.cpp:321: bool LocalPlayer::isDead() const: A fatal error occurred: LocalPlayer's CAO isn't initialized

probably because the player can't observe themself

[appgurueu]

probably because the player can't observe themself

Yes, thanks for pointing this out. This is effectively an error case, players must effectively observe themselves. I have documented this and added an error (which shuts down the server) in fedc99e (due to effective observers being computed lazily, this can unfortunately not be a proper Lua error with a stack trace).

There would be some alternatives, but they all seem messier to me:

• Make players forcibly observe themselves, perhaps warn. Problem: Now they're attached to an object they don't know about. I don't think this inconsistency between client and server is a good idea.
• Make players observe themselves and all their parents. This would work, but I don't think this follows the principle of least surprise (additionally you'd get questions / inconsistencies about get_effective_observers on the Lua side of things). It's also a bit uglier to implement.
• Error on set_attach / set_observers if condition is violated: This would require reworking the code to eagerly compute effective observers, which has potential performance implications I'm not really a fan of.

[SmallJoker]

There's no pointer hackery going on, and the attachments loops cannot be infinite due to sanity checks in UnitSAO::setAttachment. Overall I think the potential risk towards the 5.9.0 release is rather low (when this feature is not used).

Added a few comments for judgement.

[SmallJoker]

Observation, no action needed. This can be triggered by attaching to an object that is not observed, or when removing the player from the parent's observer list.

minetest.register_chatcommand("p", {
	func = function(name, param)
		local player = minetest.get_player_by_name(name)
		local pos = player:get_pos()

		local obj = core.spawn_item(pos, "default:dirt")
		obj:set_observers({["foo"] = true, ["bar"] = true })

		player:set_attach(obj)
		return true, "OK!"
	end
})

It is similar to the situation in the previous review: #13987 (comment) but very difficult to make this less error-prone. The main issue is that this error cannot be captured by pcall (there is no active callback).

[SmallJoker]

Works.

[sfan5]

@appgurueu are you sure you don't want this squashed with commits like "more things" and "doc"?