New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[pls do not squash :3] Allow limiting object observers #13987
base: master
Are you sure you want to change the base?
Conversation
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gave the code a quick look, not exhaustive
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. |
f3839b0
to
e3cdd7d
Compare
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.
This would be the third option, yes. I don't know if I will implement this though, it's still up for discussion.
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).
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.)
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? |
9fc8c9c
to
4c3be89
Compare
Oh, wow, this allows actual player invisibility |
I'm curious why you have managed players as: 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. |
I couldn't resist: I made a quick-and-dirty per-player hallucination mod with the Seems to work great!
Wouldn't that be |
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.
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 Other engine functions (e.g. set privs) represent sets this way too.
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?).
Currently the case (using player names as keys, though)
Yeah, that's currently the case: Doing
Same as in your proposed list format:
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 ( |
4c3be89
to
408a632
Compare
408a632
to
798f321
Compare
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 |
:3 |
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
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). |
Thanks for your answer! I think you are totally right. I was indeed thinking about cloak, but didn't consider the other use case... |
doc/lua_api.md
Outdated
* If players are managed, they always need to have themselves as observers. | ||
* Attachments: | ||
* If an object's observers are managed, the observers of all children need to be managed too. | ||
* The observers of children need to be a subset of the observers of parents. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unanswered question: object (un)loading is not touched by observers, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ don't see this answered yet
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 |
fc4c828
to
e3a4bda
Compare
I've implemented intersecting observers along the parent chain (and rebased). Glad to know that the segfault doesn't seem to be coming from this PR. |
c447d47
to
7b58a0a
Compare
@appgurueu rebase needed (can't wait for this PR!) |
3dd4606
to
30716d8
Compare
30716d8
to
05fd763
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review only
e9ddf57
to
f2e1aa8
Compare
f2e1aa8
to
c8aabe9
Compare
Co-authored-by: sfan5 <sfan5@live.de>
c8aabe9
to
e6f0a39
Compare
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 achar*
.This feature should work with newer servers and older clients as well.
To do
add_observers
/remove_observers
in addition to the currentset_observers
/get_observers
. Opinions?static_save
? The easiest solution would probably be to disallowstatic_save
for entities with managed observers.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.