Source-Python-Dev-Team / Source.Python

This plugin aims to use boost::python and create an easily accessible wrapper around the Source Engine API for scripter use.
http://forums.sourcepython.com
GNU General Public License v3.0
164 stars 32 forks source link

Improving performance of the SetTransmit hook. #332

Closed vinci6k closed 1 year ago

vinci6k commented 4 years ago

Would it be possible to create this hook on the C++ side of SP, similar to the way OnPlayerRunCommand was made?

I feel like SetTransmit is one of the most expensive functions to hook, but it can be such a useful tool. Maybe if the filtering was done on the C++ side, the performance would improve?

jordanbriere commented 4 years ago

Please elaborate on how you would see its usage?

ghost commented 4 years ago

If you need hide only certain entity for everyone, you can use just entity.effects |= EntityEffects.NODRAW

You can also probably avoid make_object (I don't know how expensive it is) using raw pointers to get values: if you need only player index: index_frompointer() Generally: (pointer + offset).get*() Yes, it is very sad that we should think about perfomance and not do the real thing.

vinci6k commented 4 years ago

Please elaborate on how you would see its usage?

If we were to stay true to the function as much as possible, something like this:

# Specify which entity to look for within the decorator.
@OnSetTransmit('prop_physics_override')
def on_set_transmit(entity, info):
    """Called when an entity is being transmitted to a client.

    Args:
        entity (Entity): Instance of Entity that's being transmitted.
        info (CheckTransmitInfo): Instance of CheckTransmitInfo.

    """

Maybe it can be optimized further if we ditch creating the Entity instance and just pass the index instead (since most plugins will use some sort of list/set/dict to check for entities). And seeing as CCheckTransmitInfo returns an edict when accessing the client attribute, perhaps it should be converted to the player's index or userid?

Edit: Now that I think about it, creating the Entity instance would be required (or at least BaseEntity) in order to filter out the entities by classname..

jordanbriere commented 4 years ago

Yes, it is very sad that we should think about perfomance and not do the real thing.

If the tools that are made freely available to you make you sad, then perhaps you should stop torturing yourself and write your owns. Everyone deserves to be happy!

Please elaborate on how you would see its usage?

If we were to stay true to the function as much as possible, something like this:

# Specify which entity to look for within the decorator.
@OnSetTransmit('prop_physics_override')
def on_set_transmit(entity, info):
    """Called when an entity is being transmitted to a client.

    Args:
        entity (Entity): Instance of Entity that's being transmitted.
        info (CheckTransmitInfo): Instance of CheckTransmitInfo.

    """

Maybe it can be optimized further if we ditch creating the Entity instance and just pass the index instead (since most plugins will use some sort of list/set/dict to check for entities). And seeing as CCheckTransmitInfo returns an edict when accessing the client attribute, perhaps it should be converted to the player's index or userid?

Edit: Now that I think about it, creating the Entity instance would be required (or at least BaseEntity) in order to filter out the entities by classname..

Hmm. The optimization would be minimal and would really just be beneficial when there are multiple callbacks registered (as the wrapped pointers would be re-used for all of them). An optimal implementation would be to let the c++ callback do the filtering rather than having to interrogate the Python interpreter every calls.

An idea could be to add the following methods:

BaseEntity.hide() # Globally hide this entity for all players.
BaseEntity.unhide() # Globally unhide this entity for all players.
BaseEntity.is_hidden() # Return whether this entity is globally hidden.
Player.hide_entity(BaseEntity) # Hide the given entity for this player.
Player.unhide_entity(BaseEntity) # Unhide the given entity for this player.
Player.is_entity_hidden(BaseEntity) # Return whether the given entity is hidden for this player.

Then have an internal system that filter the SetTransmit calls based on a global map of bitsets, etc. That way the c++ callback would only have to do something like this:

def set_transmit(entity, player):
    if entity is globally hidden:
        return False

    if entity is hidden for player:
        return False

Basically, you flag an entity as hidden once and the c++ callback take care of it from there, etc. Of course this is just theoretical design but this would be optimal in my opinion because the hook already know what's up removing the back and forth, etc. At least, definitely sounds like a more worthy optimization overall.

Ayuto commented 4 years ago

I like your suggestion!

This would work unless you want to conditionally hide or unhide entities. So, additionally to your suggestion we might want to still call Python callbacks if there are any.

jordanbriere commented 4 years ago

I like your suggestion!

This would work unless you want to conditionally hide or unhide entities. So, additionally to your suggestion we might want to still call Python callbacks if there are any.

Yes, if there are callbacks registered then it makes sense to call them from there and re-use the objects. I would however vote against doing any classname filtering and directly pass entity, player to them. For example:

def set_transmit(entity, player):
    if entity is globally hidden:
        return False

    if entity is hidden for player:
        return False

    for callback in callbacks:
        if not callback(entity, player):
            return False

I don't think any other info is relevant, is it?

vinci6k commented 4 years ago

Basically, you flag an entity as hidden once and the c++ callback take care of it from there, etc. Of course this is just theoretical design but this would be optimal in my opinion because the hook already know what's up removing the back and forth, etc. At least, definitely sounds like a more worthy optimization overall.

This is so much better than my initial idea! Another thing that we need to keep in mind is the FL_EDICT_ALWAYS flag. Entities that have it can't be hidden until it is stripped.

jordanbriere commented 4 years ago

This is so much better than my initial idea! Another thing that we need to keep in mind is the FL_EDICT_ALWAYS flag. Entities that have it can't be hidden until it is stripped.

Perhaps you could write a draft in Python that handle such scenarios and edge cases that would have to be kept in mind? This sure would help immensely to get a better view of the big picture if all the special cases are known ahead of time as they could certainly influence how the whole system would need to be implemented. :)

vinci6k commented 4 years ago

Sure thing! Sorry for taking so long to respond, I've been away from home for a while. I'll post the draft in an hour or so.

vinci6k commented 4 years ago
# ../set_transmit/set_transmit.py

# Source.Python
from entities import CheckTransmitInfo
from entities.entity import Entity
from entities.helpers import index_from_pointer, edict_from_index
from entities.hooks import EntityPreHook, EntityCondition
from players import PlayerGenerator
from players.entity import Player
from players.helpers import userid_from_edict

FL_EDICT_ALWAYS = 1<<3

is_prop_physics = EntityCondition.equals_entity_classname(
    'prop_physics_multiplayer')

hidden_entities = {}

class EntityST(Entity):
    caching = True

    def hide(self):
        # Is this entity supposed to skip the SetTransmit hook?
        if self.edict.state_flags & FL_EDICT_ALWAYS:
            # Strip the FL_EDICT_ALWAYS flag to make the entity go through the
            # hook at least once.
            self.edict.state_flags = self.edict.state_flags ^ FL_EDICT_ALWAYS

        hidden_entities[self.index] = set()

    def unhide(self):
        try:
            hidden_entities.pop(self.index)
        except KeyError:
            return

    @property
    def is_hidden(self):
        # Wasn't sure how to handle this. Should it return True only if the
        # entity is hidden globally? Or even if it's hidden from one or more
        # players?
        return True if self.index in hidden_entities else False

class PlayerST(Player):
    caching = True

    def hide_entity(self, index):
        try:
            userids = hidden_entities[index]
            # Is the entity hidden globally?
            if not userids:
                return

        except KeyError:
            hidden_entities[index] = set()

        edict = edict_from_index(index)
        # Is this entity supposed to skip the SetTransmit hook?
        if edict.state_flags & FL_EDICT_ALWAYS:
            # Strip the FL_EDICT_ALWAYS flag to make the entity go through the
            # hook at least once.
            edict.state_flags = edict.state_flags ^ FL_EDICT_ALWAYS

        hidden_entities[index].add(self.userid)

    def unhide_entity(self, index):
        try:
            userids = hidden_entities[index]
        except KeyError:
            return

        # Is the entity hidden globally?
        if not userids:
            for edict in PlayerGenerator():
                userids.add(userid_from_edict(edict))

        userids.remove(self.userid)

    def is_entity_hidden(self, index):
        try:
            userids = hidden_entities[index]
        except KeyError:
            return False

        # Is the entity hidden globally or hidden from the player?
        if not userids or self.userid in userids:
            return True

        return False

@EntityPreHook(is_prop_physics, 'set_transmit')
def set_transmit_pre(stack_data):
    index = index_from_pointer(stack_data[0])

    try:
        userids = hidden_entities[index]
    except KeyError:
        return

    edict = edict_from_index(index)
    # Check if the FL_EDICT_ALWAYS flag has been reapplied.
    if edict.state_flags & FL_EDICT_ALWAYS:
        # If so, strip it again.
        edict.state_flags = edict.state_flags ^ FL_EDICT_ALWAYS

    # No userids found - hide the entity globally.
    if not userids:
        return False

    info = CheckTransmitInfo._obj(stack_data[1])
    # Should the entity be hidden from this player?
    if userid_from_edict(info.client) in userids:
        return False

From my experience with the SetTransmit hook, FL_EDICT_ALWAYS is the only special case I've come across so far. Maybe entities with the FL_EDICT_DONTSEND flag could be another? I'm not sure though, I never really messed with that flag.

jordanbriere commented 4 years ago

Alright so, I had some time this afternoon to play around with the idea and I ended up with something. Keep in mind this is a first pass so it is likely to change if/when I revisit the concept: https://github.com/Source-Python-Dev-Team/Source.Python/commit/14a38d4c5f19cf5f31e4b7c6477a5f7ed8595c28

First of all, I discarded the idea of using SetTransmit altogether for many reasons. The first one being that it is virtual, and constantly having to check if we have a hook registered for every entities was getting redundant. Another reason is that it was very unreliable. Trying to interfere with the decision process of the engine was a pain and would have required hooks on UpdateTransmitState, ShouldTransmit, and probably some others as well as SetTransmit in order have an uniform implementation that covers all cases. So yeah, instead I opted for a post transmission filtering system that waits for the engine to be done processing the entities and then we start filtering.

For the sake of example, let's say we wanted to hide all prop physics from all players.

The main class we are going to use is TransmitFilter:

TransmitFilter(type=TransmitType.OUT, criteria=None, override=None)

The type parameter defines the type of filtering we want to do. Either IN where we want to force the transmission of an entity, or OUT where we want to block the transmission.

To hide props from all players, we would need to register an OUT filter like so:

from entities.transmit import TransmitFilter
from entities.transmit import TransmitType

@TransmitFilter(TransmitType.OUT)
def transmit_filter(entity, player):
    return entity.classname == 'prop_physics_multiplayer'

The question we have to ask ourselves when returning from a filter is: Do we want to filter this entity <type> of the transmission? In our example, the answer is; yes, we want to filter the prop out of transmission.

However, this is very inefficient to have to constantly compare classnames, etc. and this is where the criteria comes into play. It allows us to narrow down the calls to our filter to precisely what we are looking for. For example, we would use a criteria that matches prop's classname:

from entities.transmit import TransmitCriteria
from entities.transmit import TransmitFilter

is_prop_physics = TransmitCriteria.equals_entity_classname(
    'prop_physics_multiplayer')

@TransmitFilter(criteria=is_prop_physics)
def transmit_filter(entity, player):
    return True

The criteria are basically a set of states that is tested when the entities are created so there is no dynamic check of the class name as it was already done and stored. Now our filter is only called for prop physics so we can just return True. However, calling the callback just for it to return True in all cases is inefficient and we should rather set the override like so:

from entities.transmit import TransmitCriteria
from entities.transmit import TransmitFilter

is_prop_physics = TransmitCriteria.equals_entity_classname(
    'prop_physics_multiplayer')

@TransmitFilter(criteria=is_prop_physics, override=True)
def transmit_filter(entity, player):
    pass

Now our callback will never be called and all props will be filtered out of the transmission list. Having a pass function is a bit weird, so the following syntax would most likely be preferred:

TransmitFilter(criteria=is_prop_physics, override=True).initialize()

Now we don't have a callback but a default value that is used when the entity match the criteria. Now let's say we wanted to hide/unhide entities whenever we want we could do:

from entities.transmit import BaseTransmitCriteria
from entities.transmit import TransmitFilter

hiddens_entities = BaseTransmitCriteria()
TransmitFilter(criteria=hiddens_entities, override=True).initialize()

And now we could just set any entities to True whenever we feel like it. For example:

from events import Event
from players.entity import Player

@Event('player_say')
def player_say(game_event):
    player = Player.from_userid(game_event.get_int('userid'))
    entity = player.view_entity
    if game_event['text'] == 'hide':
        hiddens_entities[entity.index] = True
    else:
        hiddens_entities[entity.index] = False

Filters are processed in the order they are registered and in an "exit early" philosophy, none after are called if any decide to override the transmission state of an entity. Although we are looping through all entities every time, it is very fast because all it does in most cases is comparing a few bits and move on. At least, depending of the precision of your filters. In any cases, it is much more efficient than any other hook since we are only processing once per frame per player as opposed to once per frame, per player, as well as per entity, etc.

There are two rules; the world is always transmitted and the player are always transmitted to themselves. If we don't, the clients are crashing randomly.

Anyways, here are some Windows builds if anyone wants to test: https://drive.google.com/drive/folders/1Tls07h9H_UTjflwV9hP3iEcN-4OFCDei?usp=sharing

vinci6k commented 4 years ago

Oh my.. This is amazing! I'm currently out of town so I'm unable to play around with it. Can't wait to get my hands on this.

CookStar commented 4 years ago

After seeing vinci6k's comment and jordanbriere's commit, I came up with an implementation that merges both of them. This commit is for the core-part (C++). https://github.com/CookStar/Source.Python/commit/d739f6e1b906c078a78b1b106df9224f9baefb96

Sure, this is still in the draft stage, but I think it will be easy to use and fast. And, if we implement And and Or in SIMD, we can furthermore speed up the process.

Example:

from events import Event
from players.entity import Player

from entities.transmit import transmit_manager

@Event("player_say")
def on_player_say(game_event):
    player = Player.from_userid(game_event["userid"])
    entity = player.view_entity

    if game_event["text"] == "hide":
        transmit_manager.hide(entity.index)
    if game_event["text"] == "hide_from":
        transmit_manager.hide_from(entity.index, player.index)

    if game_event["text"] == "unhide":
        transmit_manager.unhide(entity.index)
    if game_event["text"] == "unhide_from":
        transmit_manager.unhide_from(entity.index, player.index)

    if game_event["text"] == "is_hidden":
        #Print if the entity is hidden globally.
        print(transmit_manager.is_hidden(entity.index))
    if game_event["text"] == "is_hidden_from":
        #Print if the entity is hidden to a specific player
        print(transmit_manager.is_hidden_from(entity.index, player.index))
    if game_event["text"] == "get_hidden_states":
        #Get a list of player indexes where the entity is hidden.
        print(transmit_manager.get_hidden_states(entity.index))

    if game_event["text"] == "reset":
        transmit_manager.reset(entity.index)
    if game_event["text"] == "reset_from":
        transmit_manager.reset_from(entity.index, player.index)
    if game_event["text"] == "reset_all":
        transmit_manager.reset_all()

I completely discarded the Python callbacks in this implementation. I don't think we need a conditional lookup in callbacks to hide an entity.

Right now the filters are consuming a lot of memory, so I wonder if it would be a good idea to increase or decrease the filters on player active and disconnect to reduce memory consumption.

jordanbriere commented 4 years ago

And, if we implement And and Or in SIMD, we can furthermore speed up the process.

It could also be optimized by looping yourself instead of relying on And and Or. This would not only cut the amount of iterations by half, but would also remove the need for patching the SDK (which should really be a last resort).

Right now the filters are consuming a lot of memory, so I wonder if it would be a good idea to increase or decrease the filters on player active and disconnect to reduce memory consumption.

You are using a lot of memory because you are using the absolute max that is in bits resulting in your vector being 4 to 8 times bigger than what you really need depending of the game. Basically, your vector has entries for 255 players, while the max is 32 on most games, 64 on CS games, 4 on L4D, etc.

That said, your implementation doesn't seem to take into consideration that indexes can be re-used, or that players can join after an entity was globally hidden so you definitely should listen for (at least) player connection and entity deletion regardless.

I completely discarded the Python callbacks in this implementation. I don't think we need a conditional lookup in callbacks to hide an entity.

I tend to disagree here. I originally pitched that hide/unhide design but I grew against it because it simply can't compare with the flexibility a dynamic filtering system has to offers. For example, let's say I want to hide teammates from seeing each other. Using a filter, I could just do the following:

from entities.transmit import TransmitCriteria
from entities.transmit import TransmitFilter

@TransmitFilter(criteria=TransmitCriteria.is_player)
def transmit_filter(entity, player):
    return entity.team_index == player.team_index

While with your implementation, I would have to listen for team changes, player connection, disconnection, etc. in order to loop through all players every time and ensure their state are constantly up-to-date. Yet, this is a very basic example and could get even more trickier quickly if I wanted for example, not only hide entities from a specific team, but also based on their location on the map, etc. I honestly don't believe the rare few cases I can think of where simply having global/per-player switches would be viable as opposed to register my own hook.

Anyways, I have a clear vision of where I want to take my implementation but just haven't gotten to it yet and probably won't have any time to allocate for a second pass for at least another two weeks minimum. There are quite many flaws in my first pass I want to address, as well as minimizing the syntax to both optimize and improve flexibility. Let's discuss the pros and cons then I guess.

CookStar commented 4 years ago

It could also be optimized by looping yourself instead of relying on And and Or. This would not only cut the amount of iterations by half, but would also remove the need for patching the SDK (which should really be a last resort).

You are using a lot of memory because you are using the absolute max that is in bits resulting in your vector being 4 to 8 times bigger than what you really need depending of the game. Basically, your vector has entries for 255 players, while the max is 32 on most games, 64 on CS games, 4 on L4D, etc.

Yeah, I know. I just made it as a test, so detailed implementation is not yet done.

That said, your implementation doesn't seem to take into consideration that indexes can be re-used, or that players can join after an entity was globally hidden so you definitely should listen for (at least) player connection and entity deletion regardless.

That's right too, if you unhide the entity and then remove the entity, it will crash. This is a proof of concept.

I tend to disagree here. I originally pitched that hide/unhide design but I grew against it because it simply can't compare with the flexibility a dynamic filtering system has to offers.

The biggest issue with the filtering system is when you have a duplicate filter, or when you want to override a filter.

For example, let's say I want to hide teammates from seeing each other.

Using this example, if we wanted to further show specific players again, it would be difficult to do so. If this is my SP plugin, I can fix filter, but if it's someone else's plugin, I'll have to fix each of them. I think such a global filtering system has too much power. Depending on the purpose of the plugin, you may want to nest the hide state and show state, but I think that would be a difficult to achieve or would make an interface more complex with a global filtering system like this one.

I honestly don't believe the rare few cases I can think of where simply having global/per-player switches would be viable as opposed to register my own hook.

To use an example of what I would use, when certain enemy BOT get flash bang, I'll make them invisible for 10 seconds, but with such a little hide, a global filtering system would have too much overhead and would be difficult to implement hide.

CookStar commented 4 years ago

After seeing vinci6k's comment and jordanbriere's commit, I came up with an implementation that merges both of them. This commit is for the core-part (C++). CookStar@d739f6e

I have updated the entities_transmit on my version(https://github.com/CookStar/Source.Python/commit/27a70423d971649081de4a186851ebf584d5a940). I think all the features work with this update(Including Windows). Also, with this update, you can now hide and show directly from Entity and Player.

Entity class https://github.com/CookStar/Source.Python/blob/27a70423d971649081de4a186851ebf584d5a940/addons/source-python/packages/source-python/entities/_base.py#L783-L846

Player class https://github.com/CookStar/Source.Python/blob/27a70423d971649081de4a186851ebf584d5a940/addons/source-python/packages/source-python/players/_base.py#L1005-L1053

Other Function https://github.com/CookStar/Source.Python/blob/27a70423d971649081de4a186851ebf584d5a940/addons/source-python/packages/source-python/entities/transmit.py#L42-L47

The @jordanbriere example can be done this way.

@Event("player_spawn")
def on_player_spawn(game_event):
    player = Player.from_userid(game_event["userid"])
    for other in PlayerIter():
        if player.team_index == other.team_index:
            player.hide_entity(other.index)
            other.hide_entity(player.index)

#An optional reset, for spectating the game.
@Event("player_death")
def on_player_death(game_event):
    player = Player.from_userid(game_event["userid"])
    for other in PlayerIter():
        player.reset_entity(other.index)
CookStar commented 3 years ago

Any suggestions or opinions?

jordanbriere commented 3 years ago

Any suggestions or opinions?

Well, my opinion on this didn't really change since last time. I think the proposed alternative lacks dynamic.

The biggest issue with the filtering system is when you have a duplicate filter, or when you want to override a filter.

For example, let's say I want to hide teammates from seeing each other.

Using this example, if we wanted to further show specific players again, it would be difficult to do so. If this is my SP plugin, I can fix filter, but if it's someone else's plugin, I'll have to fix each of them. I think such a global filtering system has too much power. Depending on the purpose of the plugin, you may want to nest the hide state and show state, but I think that would be a difficult to achieve or would make an interface more complex with a global filtering system like this one.

The thing is that you shouldn't have to worry about other plugins. The idea with isolated filtering is that plugins do not clash with each others. As long as there is at least one plugin that want an entity hidden, it will remains hidden regardless if any of them determined it no longer want to override its transmission state. This ensure an healthy relationship between plugins because they do not override each other.

Just to give an analogy; this prevent issues such as DynamicHooks, CFunction and Python deleting a convention because they are done with it consequently breaking the other parties involved. The same applies for global hidden switches because if plugin A determine an entity should no longer be hidden, plugin B will be broken and vice-versa. While each of them having their own switches, they won't affect the others regardless of what they decides. This basically creates a "ref counting" system that makes its best to make everyone happy.

As for being too powerful, I wouldn't say that. It really just offers the same amount of power a hook on SetTransmit would offers while greatly decreasing the back and forth between C++ and Python (which is the real overhead here).

To use an example of what I would use, when certain enemy BOT get flash bang, I'll make them invisible for 10 seconds, but with such a little hide, a global filtering system would have too much overhead and would be difficult to implement hide.

Well, it wouldn't be difficult to achieve at all. You just set their state into your criteria, then delay its reversal.

The @jordanbriere example can be done this way.

@Event("player_spawn")
def on_player_spawn(game_event):
    player = Player.from_userid(game_event["userid"])
    for other in PlayerIter():
        if player.team_index == other.team_index:
            player.hide_entity(other.index)
            other.hide_entity(player.index)

#An optional reset, for spectating the game.
@Event("player_death")
def on_player_death(game_event):
    player = Player.from_userid(game_event["userid"])
    for other in PlayerIter():
        player.reset_entity(other.index)

Which is pretty much the point I was making before; if what used to be doable in a few lines through a hook becomes overly complicated, I'd say it fails to optimize what this issue is all about. Also, admin swap a player from T to CT; your plugin is broken until next round. As I previously said, if I have to listen to 10 events, 12 listeners, and ensure I cover many edges cases that could conflict with the dynamic aspect of the game I would personally register a hook for simplicity and I'm sure this would be what most scripters would do as well. To me it comes to; flexibility. If the system isn't offering as much flexibility as a SetTransmit hook then this isn't addressing the core of the problem.

I don't doubt your implementation would be slightly faster each frame (if we disregard the fact it does all its work by default as opposed to be idling until instructed to do something by a plugin), but this is like saying a stock car is less expensive than one that have advanced options. If you have to remove 90% of the features then this isn't a good option in my opinion.

Here is a challenge for you:

if I wanted for example, not only hide entities from a specific team, but also based on their location on the map, etc.

Write an example solely using global switches that hide team mates unless they are into a 2000 units range from each other.

Anyways, I did commit some code locally but haven't went back and fully tests everything. I've been either busy, lazy or easily distractible every time I try to get to it but this is still on my list.

CookStar commented 3 years ago

The thing is that you shouldn't have to worry about other plugins. The idea with isolated filtering is that plugins do not clash with each others. As long as there is at least one plugin that want an entity hidden, it will remains hidden regardless if any of them determined it no longer want to override its transmission state. This ensure an healthy relationship between plugins because they do not override each other.

I think you are missing my point.

As for being too powerful, I wouldn't say that. It really just offers the same amount of power a hook on SetTransmit would offers while greatly decreasing the back and forth between C++ and Python (which is the real overhead here).

Source.Python should not provide the equivalent of SetTransmit as a default feature.

Which is pretty much the point I was making before; if what used to be doable in a few lines through a hook becomes overly complicated, I'd say it fails to optimize what this issue is all about.

This is an inevitable problem in the real-time aspect of a game. Even if you have a larger amount of code, it is preferable to have the code executed only when it is necessary, rather than SetTransmit which is executed every tick.

Also, admin swap a player from T to CT; your plugin is broken until next round. As I previously said, if I have to listen to 10 events, 12 listeners, and ensure I cover many edges cases that could conflict with the dynamic aspect of the game I would personally register a hook for simplicity and I'm sure this would be what most scripters would do as well. To me it comes to; flexibility. If the system isn't offering as much flexibility as a SetTransmit hook then this isn't addressing the core of the problem.

That's losing the flexibility of the Source.Python as a whole instead of gaining the flexibility of a single script. As you wrote, admin swap a player from T to CT breaks the plugin, but that's exactly what this entities_transmit needs. The reason you can change a player's team freely is because there is no filter to force a team change and the Source.Python and Source Engine accept the changes at any time. The hidden/shown state of an entity should be as well. The hidden/shown state should be treated just like any other player state, such as team, noclip, god mode, freeze, voice mute, etc... for real flexibility.

If we are going to implement a filter, we need a method to block the following filter from outside the filter.

from entities.transmit import TransmitCriteria
from entities.transmit import TransmitFilter

@TransmitFilter(TransmitType.OUT)
def transmit_filter(entity, player):
    return True

What if the Admin wants to change the hidden/shown state of a particular entity or player? It's impossible.

Here is a challenge for you:

if I wanted for example, not only hide entities from a specific team, but also based on their location on the map, etc.

Write an example solely using global switches that hide team mates unless they are into a 2000 units range from each other.

If you want to lock in the hidden state "for real", this implementation is all you need. This should work about as fast as using the filter feature.(The code should be no different than when you implement it with filters.)

# Python Imports
#   Itertools
import itertools

# Source.Python Imports
#   Filters
from filters.players import PlayerIter
#   Listeners
from listeners import OnTick

@OnTick
def on_tick():
    for player, other in itertools.combinations(PlayerIter(), 2):
        if (not player.dead and not other.dead and
            player.team_index == other.team_index and
            player.origin.get_distance(other.origin) < 2000):
            player.hide_entity(other.index)
            other.hide_entity(player.index)
        else:
            player.reset_entity(other.index)
            other.reset_entity(player.index)

However, plugin authors should be fully aware of the dangers of using @OnTick and its impact on other plugins. And I don't think we should provide a filter, which is virtually equivalent to @OnTick, as a default feature of Source.Python.

jordanbriere commented 3 years ago

The thing is that you shouldn't have to worry about other plugins. The idea with isolated filtering is that plugins do not clash with each others. As long as there is at least one plugin that want an entity hidden, it will remains hidden regardless if any of them determined it no longer want to override its transmission state. This ensure an healthy relationship between plugins because they do not override each other.

I think you are missing my point.

I don't think I am missing your point. You want your plugin to be able to override every others whenever you say so. Which as I said; promote clashes between plugins. You need to look at it from an API standpoint, not a single plugin. Just like you were a mother bird that have to feed all its children and have them all happy and fed. And to do so, you have to listen to all of them and ensure one do not eat the food of the others, etc.

As for being too powerful, I wouldn't say that. It really just offers the same amount of power a hook on SetTransmit would offers while greatly decreasing the back and forth between C++ and Python (which is the real overhead here).

Source.Python should not provide the equivalent of SetTransmit as a default feature.

The ultimate goal here is, as mentioned in the title, to provide an optimized way to achieve what can be done through a SetTransmit hook without the burden of a SetTransmit hook. If the solution isn't as flexible as said hook, then this isn't addressing the issue as stated above.

Which is pretty much the point I was making before; if what used to be doable in a few lines through a hook becomes overly complicated, I'd say it fails to optimize what this issue is all about.

This is an inevitable problem in the real-time aspect of a game. Even if you have a larger amount of code, it is preferable to have the code executed only when it is necessary, rather than SetTransmit which is executed every tick.

I totally agree. Which is the very reason I originally pitched that hide/unhide design because it was eliminating the need to call Python entirely. However, I quickly realized that it wouldn't be a good replacement because there are too many moving parts within the game and dynamic lookup is a must more often than not (just search around and you will see how it is used by plugins).

Hence why there is a need for dynamic filtering, and the optimization comes by allowing scripters to have predefined static states so they can limit and reduce the calls to their filters for stuff that do not change. This attempt to offer the best of both worlds; the flexibility of the original hook, with reduced calls to their callback.

To be honest, I find it rather ironic for you to play the it is preferable to have the code executed only when it is necessary card, when your proposal relies on being active by default whenever SP is loaded as opposed to be idling until instructed to do something. 😕

If you want to lock in the hidden state "for real", this implementation is all you need. This should work about as fast as using the filter feature.(The code should be no different than when you implement it with filters.)

# Python Imports
#   Itertools
import itertools

# Source.Python Imports
#   Filters
from filters.players import PlayerIter
#   Listeners
from listeners import OnTick

@OnTick
def on_tick():
    for player, other in itertools.combinations(PlayerIter(), 2):
        if (not player.dead and not other.dead and
            player.team_index == other.team_index and
            player.origin.get_distance(other.origin) < 2000):
            player.hide_entity(other.index)
            other.hide_entity(player.index)
        else:
            player.reset_entity(other.index)
            other.reset_entity(player.index)

However, plugin authors should be fully aware of the dangers of using @OnTick and its impact on other plugins. And I don't think we should provide a filter, which is virtually equivalent to @OnTick, as a default feature of Source.Python.

Thank you! This illustrate perfectly the point I was trying to make; in order to have a minimum of flexibility you have to resort to a tick listener. Which not only illustrate that, but illustrate how this would clashes with other plugins; for you to be able to hide/unhide players based on their location, you have to override and prevent all other plugins from using that feature. To bring back my bird analogy; that plugin is that greedy child that wants all the food and don't care if its siblings are starving to death. And SP being the mother bird, this is its responsibility to offer tools that do not promote such terrible designs and practices.

And to say this code is the same as a filter is exaggerating. The key differences with the filters is that they are looping through an existing array of indexes, while you are constructing it on the fly, they don't do anything unless a plugin wants them to, while you are doing it anyways, etc. Regardless, the main point isn't that it does more than global switches, but that it does less than a SetTransmit hook which is what this issue is all about.

I guess we will have to agree to disagree as I just feel like I'm repeating myself over and over at this point; the goal is to offer a faster alternative to a SetTransmit hook. If you are stripping all the flexibility to achieve that goal, promote clashes between plugins, then this is not addressing the issue at hand. If you want to explore other possibilities that;

Then please be my guest. But your current global switches proposal is unfortunately not it for the many reasons mentioned above.

CookStar commented 3 years ago

Hence why there is a need for dynamic filtering, and the optimization comes by allowing scripters to have predefined static states so they can limit and reduce the calls to their filters for stuff that do not change. This attempt to offer the best of both worlds; the flexibility of the original hook, with reduced calls to their callback.

First of all, let me point out that this behavior is dangerous. Not executing a callback because it doesn't affect the hide state or show state (just because the Ture has already been returned) is against the user's expected behavior. There's no guarantee that callback won't rewrite other data. Filter should execute all functions that match the criteria and then decide whether to hide or show them.(Like SourceMod actually does.)

from entities.transmit import TransmitCriteria
from entities.transmit import TransmitFilter

@TransmitFilter(criteria=TransmitCriteria.is_player)
def transmit_filter(entity, player):
    return True

@TransmitFilter(criteria=TransmitCriteria.is_player)
def transmit_filter_second(entity, player):
    print("This will not be called.")
    print("Users may rewrite critical data.")
    return True

This is why you can't reduce the number of function calls.

The ultimate goal here is, as mentioned in the title, to provide an optimized way to achieve what can be done through a SetTransmit hook without the burden of a SetTransmit hook. If the solution isn't as flexible as said hook, then this isn't addressing the issue as stated above.

That's because the Source SDK, or Source Enigne, doesn't have functions like SetHideState() or properties like Player.hide_state, so we are forced to hook SetTransmit to use it, right? Sure, the goal of this issue is to increase the performance of SetTransmit, but what the plugin authors really need is a way to manipulate the hidden/shown state directly with high performance.(Such as team, noclip, god mode, freeze, voice mute, etc...) And a feature that will help users to implement new and creative plugins.

I totally agree. Which is the very reason I originally pitched that hide/unhide design because it was eliminating the need to call Python entirely. However, I quickly realized that it wouldn't be a good replacement because there are too many moving parts within the game and dynamic lookup is a must more often than not (just search around and you will see how it is used by plugins).

Yeah, I researched it, and quite a lot of it, too. Most plugins just add a hook when creating an entity or when a player switches teams to prevent certain entities from being displayed.

https://forums.alliedmods.net/showthread.php?t=290948 https://github.com/Franc1sco/ZR-Glowing/blob/master/zr_glow.sp Code to simulate:

# Python Imports
#   Itertools
import itertools
#   Weakref
from weakref import WeakValueDictionary

# Source.Python Imports
#   Events
from events import Event
#   Filters
from filters.players import PlayerIter
#   Player
from players.entity import Player

hidden_skin = WeakValueDictionary()

first = False

@Event("round_start")
def on_round_start(game_event):
    global first
    first = True

def ZR_OnClientInfected(player):
    global first
    if first:
        for other in PlayerIter("alive"):
            if ZR_IsClientHuman(other.index):
                skin_index = set_skin(other.index)
                skin_entity = Entity(skin_index)
                skin_entity.hide()
                hidden_skin[skin_index] = skin_entity

        first = False

    if has_skin(player):
        Entity(get_skin(player)).remove()
    for skin_index in hidden_skin:
        player.reset_entity(skin_index)

def ZR_OnClientHumanPost(player):
    for other in PlayerIter("alive"):
        if ZR_IsClientHuman(other.index):
            if has_skin(other):
                Entity(get_skin(other)).remove()

            skin_index = set_skin(other.index)
            skin_entity = Entity(skin_index)
            skin_entity.hide()
            hidden_skin[skin_index] = skin_entity

    for other, skin_index in itertools.product(PlayerIter("alive"), hidden_skin):
        if ZR_IsClientZombie(other.index):
            other.reset_entity(skin_index)

@Event("player_death")
def on_player_death(game_event):
    player = Player.from_userid(game_event["userid"])
    for skin_index in hidden_skin:
        player.hide_entity(skin_index)

https://github.com/vinci6k/floating-damage-numbers/blob/15ffcc15f9d65d1e726dc2d6b44647a08525a285/addons/source-python/plugins/fdn/fdn.py#L28 https://github.com/vinci6k/floating-damage-numbers/blob/15ffcc15f9d65d1e726dc2d6b44647a08525a285/addons/source-python/plugins/fdn/core/floating_number.py#L85 Sorry to use it as a demonstration. Code to simulate:

self.world_text = Entity.create('point_worldtext')
self.world_text.hide()
self.world_text.reset_from(index_from_userid(self.recipient))

https://forums.sourcepython.com/viewtopic.php?f=31&t=1329 Code to simulate:

# Python Imports
#   Weakref 
from weakref import WeakValueDictionary

# Source.Python Imports
#   Listeners 
from listeners import OnClientActive
from listeners import OnNetworkedEntityCreated
#   Player
from players.entity import Player

hidden_entity = WeakValueDictionary()

@OnClientActive
def on_client_active(index):
    player = Player(index)
    if player.name == "test_name":
        hidden_entity[index] = player
        player.hide()
    for entity_index in hidden_entity:
        player.hide_entity(entity_index)

@OnNetworkedEntityCreated
def on_networked_entity_created(entity):
    if entity.classname == "prop_physics_multiplayer":
        hidden_entity[entity.index] = entity
        entity.hide()

Please let me know about the edge case.

To be honest, I find it rather ironic for you to play the it is preferable to have the code executed only when it is necessary card, when your proposal relies on being active by default whenever SP is loaded as opposed to be idling until instructed to do something.

If you're talking about the handle_filters, that's not the case. Most of the handle_filters are a memory copy, which should be much faster than a Python script. The maximum copy size with 64 human players isn't that big either(32KB).

https://github.com/Source-Python-Dev-Team/Source.Python/blob/f66ba09697f9aeb25f0eee5dc5cb8599780690fc/src/core/modules/entities/entities_transmit.cpp#L404-L419

https://godbolt.org/z/8hsYbh

Thank you! This illustrate perfectly the point I was trying to make; in order to have a minimum of flexibility you have to resort to a tick listener. Which not only illustrate that, but illustrate how this would clashes with other plugins; for you to be able to hide/unhide players based on their location, you have to override and prevent all other plugins from using that feature. To bring back my bird analogy; that plugin is that greedy child that wants all the food and don't care if its siblings are starving to death. And SP being the mother bird, this is its responsibility to offer tools that do not promote such terrible designs and practices.

That code is to recreate the same limitations that the filter would produce without the extra handling. I do not recommend or advocate writing that kind of code. I expressly mentioned that in the comment.

If you want to lock in the hidden state "for real"

This is the kind of code that should really be implemented, and it should leave room for other plugins to take control. And this is the code that should be recommended even in the case of filter.

# Python Imports
#   Collections
from collections import defaultdict
#   Itertools
import itertools

# Source.Python Imports
#   Filters
from filters.players import PlayerIter
#   Listeners
from listeners import OnTick

hidden_player = defaultdict(lambda: defaultdict(bool))

@OnTick
def on_tick():
    for player, other in itertools.combinations(PlayerIter(), 2):
        if (not player.dead and not other.dead and
            player.team_index == other.team_index and
            player.origin.get_distance(other.origin) < 2000):
            if hidden_player[player.index][other.index] is False:
                hidden_player[player.index][other.index] = True
                player.hide_entity(other.index)
            if hidden_player[other.index][player.index] is False:
                hidden_player[other.index][player.index] = True
                other.hide_entity(player.index)
        else:
            if hidden_player[player.index][other.index] is True:
                hidden_player[player.index][other.index] = False
                player.reset_entity(other.index)
            if hidden_player[other.index][player.index] is True:
                hidden_player[other.index][player.index] = False
                other.reset_entity(player.index)

or

# Python Imports
#   Collections
from collections import defaultdict

# Source.Python Imports
#   Entities
from entities.transmit import TransmitCriteria
from entities.transmit import TransmitFilter

hidden_player = defaultdict(lambda: defaultdict(bool))

@TransmitFilter(criteria=TransmitCriteria.is_player)
def transmit_filter(entity, player):
    if (not player.dead and not entity.dead and
        player.team_index == entity.team_index and
        player.origin.get_distance(entity.origin) < 2000):
        if hidden_player[player.index][entity.index] is False:
            hidden_player[player.index][entity.index] = True
            return True
    else:
        if hidden_player[player.index][entity.index] is True:
            hidden_player[player.index][entity.index] = False
            return False

And to say this code is the same as a filter is exaggerating. The key differences with the filters is that they are looping through an existing array of indexes, while you are constructing it on the fly, they don't do anything unless a plugin wants them to, while you are doing it anyways, etc.

I'm not quite sure what you mean. This plugin does not construct anything. You may be referring to the TransmitStates_t hide and TransmitStates_t show already assigned to the human player, but as mentioned above, only memory copy will be executed.

  • Offer as much flexibility as a SetTransmit hook.

Tell me about a situation where my implementation doesn't actually provide flexibility.

  • Do not promote clashes between plugins or encourage bad designs.

It's your filtering system that's introducing the clash in the first place, and changing the behavior of function execution just because the hide state/show state doesn't change is dangerous and should be avoided. Admin needs to know, and possibly rewrite, the filters used by all plugins. There is also a risk of serious debugging problems depending on the order in which the plugins are imported.

  • That isn't registering such noisy hooks unless it is being used by a plugin.

The callback is effectively the equivalent of a noisy hook (@OnTick with for p, e in itertools.product(PlayerIter(), EntityIter(criteria)), and there is no way to reduce it itself.

CookStar commented 3 years ago

I don't think I am missing your point. You want your plugin to be able to override every others whenever you say so. Which as I said; promote clashes between plugins. You need to look at it from an API standpoint, not a single plugin. Just like you were a mother bird that have to feed all its children and have them all happy and fed. And to do so, you have to listen to all of them and ensure one do not eat the food of the others, etc.

Also, to put it bluntly, it's offensive to say it like this. It's outrageous that you would assume like that, even though I haven't made or distributed any plugins. And it's not polite to say it in the above way.

I'm just making suggestions and opinions to help the Source.Python community so that all plugin creators can benefit equally from the features. If in the end my suggestions are not implemented, there is no problem there, and that's fine by me. I'm just doing what I can do.

jordanbriere commented 3 years ago

First of all, let me point out that this behavior is dangerous. Not executing a callback because it doesn't affect the hide state or show state (just because the Ture has already been returned) is against the user's expected behavior. There's no guarantee that callback won't rewrite other data. Filter should execute all functions that match the criteria and then decide whether to hide or show them.(Like SourceMod actually does.)

from entities.transmit import TransmitCriteria
from entities.transmit import TransmitFilter

@TransmitFilter(criteria=TransmitCriteria.is_player)
def transmit_filter(entity, player):
    return True

@TransmitFilter(criteria=TransmitCriteria.is_player)
def transmit_filter_second(entity, player):
    print("This will not be called.")
    print("Users may rewrite critical data.")
    return True

This is why you can't reduce the number of function calls.

How is it dangerous? They are filters, not listeners. If the first callback determined it wants to override the transmission then there is nothing else to filter and there is no reason for the second one to be called. If the first were to return False, then the second would have its chance at filtering the transmission. Basically, the filters are being interrogated one-by-one in the order they were registered until a decision is being made, and once that is done, there is no reason for the remaining to be asked anything because what they have to say at that point is irrelevant and unnecessary computation. It would only make sense for all of them to be called if they were listening for entity transmissions, as opposed to filtering them.

Also, if you re-read my previous posts, you will notice that I said that calling a callback just for it to return True in all case was making no-sense which encouraged the override feature to address that meaning there is no Python calls being performed because the response is known ahead of time. Though locally that feature is also removed and is assumed to be set when you have an active filter that do not have a callback bound to it.

That's because the Source SDK, or Source Enigne, doesn't have functions like SetHideState() or properties like Player.hide_state, so we are forced to hook SetTransmit to use it, right? Sure, the goal of this issue is to increase the performance of SetTransmit, but what the plugin authors really need is a way to manipulate the hidden/shown state directly with high performance.(Such as team, noclip, god mode, freeze, voice mute, etc...) And a feature that will help users to implement new and creative plugins.

Although we are using a hook on that method for that purpose and aliasing it as hiding, that is technically not hiding. The SetTransmit method is used by the engine to determine whether an entity should be transmitted or not (aka filtering). The engine have properties that could be used to hide an entity, such as render modes, alpha, no draw effect, etc. The limitation is that you cannot filter transmission on a per-player basis, which is where that hook become a necessity. However, not transmitting an entity technically does much more than just hiding it. This means all its parented children are not transmitted, its physics isn't updating client-side, there is no more prediction going on (such as pre-collisions, etc.). As for the noclip, etc. properties, I was actually against those because they are promoting conflict between plugins (e.g. #p4362). For pretty much the same reasons I'm against global hidden states here. Hence why I strongly prefer isolated switches as posted above:

from events import Event
from players.entity import Player

hidden_entities = BaseTransmitCriteria()
TransmitFilter(criteria=hiddens_entities, override=True).initialize()

@Event('player_say')
def player_say(game_event):
    player = Player.from_userid(game_event.get_int('userid'))
    entity = player.view_entity
    if game_event['text'] == 'hide':
        hidden_entities[entity.index] = True
    else:
        hidden_entities[entity.index] = False

Though as also mentioned above, I don't really like the current syntax and that is something that I addressed locally but that illustrate that you have the power of the hidden switches you are proposing without promoting clashes between plugins because they are the sole responsible for their states that won't be set by anyone else.

Yeah, I researched it, and quite a lot of it, too. Most plugins just add a hook when creating an entity or when a player switches teams to prevent certain entities from being displayed.

I'm not saying plugins that do not need dynamic lookup do not exist, I'm saying the ones that does, does exist. The filtering system I'm proposing attempt to address both scenarios; static AND dynamic lookups. The global switches you are proposing only addresses static ones, and have to either resort to a tick listener if you need to do dynamic lookup as you shown above. Which is definitely not the same as a filter, for the reasons I will address below.

I'm not quite sure what you mean. This plugin does not construct anything. You may be referring to the TransmitStates_t hide and TransmitStates_t show already assigned to the human player, but as mentioned above, only memory copy will be executed.

I mean that into your tick listener, you are constructing and looping through a list of player entities, while filters use the list that is fed to CheckTransmit, that you are testing all players every tick, while only the ones that are being transmitted should be. For example, if there are 50 players on the server, your tick listener will test those 50 players against each others and update their state constantly. While through a filter, it would only test the players that are in range (or in "Potential Visibility Set" as named by the engine). Meaning that out of those 50, you might test 2, then on another frame 5, etc. depending of where players are on the map and if they are visible to each other. Both can't be compared and are definitely not equivalent.

Tell me about a situation where my implementation doesn't actually provide flexibility.

If you want to conditionally hide entities based on their location, you can't do that because you don't have the flexibility of dynamic lookup as I've been pretty much repeating for my last posts in here.

The callback is effectively the equivalent of a noisy hook (@OnTick with for p, e in itertools.product(PlayerIter(), EntityIter(criteria)), and there is no way to reduce it itself.

Definitely not as I've explained above, but that isn't the point I was trying to make. SP without plugins should do as less as possible. It shouldn't register a noisy hook on CheckTransmit by default, nor do anything in that hook unless it was instructed to do so by a plugin.

Also, to put it bluntly, it's offensive to say it like this. It's outrageous that you would assume like that, even though I haven't made or distributed any plugins. And it's not polite to say it in the above way.

I'm just making suggestions and opinions to help the Source.Python community so that all plugin creators can benefit equally > from the features. If in the end my suggestions are not implemented, there is no problem there, and that's fine by me. I'm just doing what I can do.

Well, I'm not really sure how what I said was outrageous or offensive in any way but I'm sorry if it was perceived that way. Was certainly not my intention. Though, this is literally what I interpret by the post I was quoting you said I was missing your point, and that results should be externally manipulable (by admins, etc.). Please clarify if that isn't what you were trying to say. Your input is certainly appreciated, and is helpful in brainstorming what is best and this is what this thread is all about in the end. I just think there is much more to address than just static conditions.

CookStar commented 3 years ago

How is it dangerous? They are filters, not listeners. If the first callback determined it wants to override the transmission then there is nothing else to filter and there is no reason for the second one to be called. If the first were to return False, then the second would have its chance at filtering the transmission. Basically, the filters are being interrogated one-by-one in the order they were registered until a decision is being made, and once that is done, there is no reason for the remaining to be asked anything because what they have to say at that point is irrelevant and unnecessary computation. It would only make sense for all of them to be called if they were listening for entity transmissions, as opposed to filtering them.

Also, if you re-read my previous posts, you will notice that I said that calling a callback just for it to return True in all case was making no-sense which encouraged the override feature to address that meaning there is no Python calls being performed because the response is known ahead of time. Though locally that feature is also removed and is assumed to be set when you have an active filter that do not have a callback bound to it.

The reason it always returns True is because it's code for demonstration purposes, and that in itself doesn't mean anything.

Please look at this code.

# Python Imports
#   Collections
from collections import defaultdict

# Source.Python Imports
#   Entities
from entities.transmit import TransmitCriteria
from entities.transmit import TransmitFilter

hidden_player = defaultdict(lambda: defaultdict(bool))

@TransmitFilter(criteria=TransmitCriteria.is_player)
def transmit_filter(entity, player):
    if (not player.dead and not entity.dead and
        player.team_index == entity.team_index and
        player.origin.get_distance(entity.origin) < 2000):
        if hidden_player[player.index][entity.index] is False:
            hidden_player[player.index][entity.index] = True
            return True
    else:
        if hidden_player[player.index][entity.index] is True:
            hidden_player[player.index][entity.index] = False
            return False

This is intentionally written so that it can clash with other plugins. This requires writing to an external variable and needs to be done all the time.

What if the plugin wants a list of hidden entities?

@TransmitFilter(criteria=TransmitCriteria.is_player)
def transmit_filter(entity, player):
    #The filter that might hide the player, which you didn't create.
    return True

hidden_set = set()

@TransmitFilter(criteria=TransmitCriteria.is_player)
def transmit_filter(entity, player):
    if (not player.dead and not entity.dead and
        player.team_index == entity.team_index and
        player.origin.get_distance(entity.origin) < 2000):
        hidden_set.add(entity.index)
        return True
    else:
        hidden_set.discard(entity.index)
        return False

@Event("player_say")
def on_player_say(game_event):
    if game_event["text"] == "hidden_list":
        for index in hidden_set:
            print(Player(index).name)
            # Nothing will be printed.

As long as the callback may be writing outside the filter, not calling it is a problem. Since the filter is applied dynamically, to know if the filter actually rewrites the hide state/show state, you need to either allow external writes, i.e. perform all callbacks, or apply all entities and all players to the filter again when you want to know, and see the results. Edit: But since we don't know what will be written, all callbacks should be executed. A plugin might count the times it is hidden, a reference to something might rewrite memory, and so on.

The fact that @PreHook and @PostHook execute a callback even when function execution is suppressed, while @TransmitFilter has no guarantee that it will be executed is inconsistent and also strange.

Although we are using a hook on that method for that purpose and aliasing it as hiding, that is technically not hiding. The SetTransmit method is used by the engine to determine whether an entity should be transmitted or not (aka filtering). The engine have properties that could be used to hide an entity, such as render modes, alpha, no draw effect, etc. The limitation is that you cannot filter transmission on a per-player basis, which is where that hook become a necessity. However, not transmitting an entity technically does much more than just hiding it. This means all its parented children are not transmitted, its physics isn't updating client-side, there is no more prediction going on (such as pre-collisions, etc.). As for the noclip, etc. properties, I was actually against those because they are promoting conflict between plugins (e.g. #p4362).

I understand the purpose of SetTransmit and the reason for hooking it. I can also see why you don't like the feature that introduces clashes between plugins. But it is also true that at some point someone will need to introduce a clashes to manage the server. My biggest fear is that when a player is hidden in @TransmitFilter(criteria=TransmitCriteria.is_player) it could be used by a malicious player to cheat, hack, etc., over the eyes of Admin or other players. Maybe I'm overthinking this, but I don't like the idea of being able to casually hide a player. We need the ability to inhibit someone's change to the hide state to suppress it, but maybe I'm overthinking that too.

I mean that into your tick listener, you are constructing and looping through a list of player entities, while filters use the list that is fed to CheckTransmit, that you are testing all players every tick, while only the ones that are being transmitted should be. For example, if there are 50 players on the server, your tick listener will test those 50 players against each others and update their state constantly. While through a filter, it would only test the players that are in range (or in "Potential Visibility Set" as named by the engine). Meaning that out of those 50, you might test 2, then on another frame 5, etc. depending of where players are on the map and if they are visible to each other. Both can't be compared and are definitely not equivalent.

I didn't know that. It seems that I wasn't able to find that behavior in testing. I apologize for that.

Then we need a filter. In my opinion, it is better to use a combination of both implementations.

Well, I'm not really sure how what I said was outrageous or offensive in any way

This is the part that was offensive.

You want your plugin to be able to override every others whenever you say so.

I only suggest features that I think other plugin makers should be able to use. I would never suggest or give an opinion on a feature just for my own benefit. (And if I only need something for myself, I'll just implement it in my own dev branch.)

and that results should be externally manipulable (by admins, etc.)

And this is because of the fear of abuse by malicious people, as described above.

Your input is certainly appreciated, and is helpful in brainstorming what is best and this is what this thread is all about in the end. I just think there is much more to address than just static conditions.

You helped me to understand this feature completely. Thank you for your teaching!

CookStar commented 3 years ago

Definitely not as I've explained above, but that isn't the point I was trying to make. SP without plugins should do as less as possible. It shouldn't register a noisy hook on CheckTransmit by default, nor do anything in that hook unless it was instructed to do so by a plugin.

All right. I will change the implementation to hook on the first hide/show.

jordanbriere commented 3 years ago

Please look at this code.

    # Python Imports
    #   Collections
    from collections import defaultdict

    # Source.Python Imports
    #   Entities
    from entities.transmit import TransmitCriteria
    from entities.transmit import TransmitFilter

    hidden_player = defaultdict(lambda: defaultdict(bool))

    @TransmitFilter(criteria=TransmitCriteria.is_player)
    def transmit_filter(entity, player):
        if (not player.dead and not entity.dead and
            player.team_index == entity.team_index and
            player.origin.get_distance(entity.origin) < 2000):
            if hidden_player[player.index][entity.index] is False:
                hidden_player[player.index][entity.index] = True
                return True
        else:
            if hidden_player[player.index][entity.index] is True:
                hidden_player[player.index][entity.index] = False
                return False

This is intentionally written so that it can clash with other plugins. This requires writing to an external variable and needs to be done all the time.

Am I missing something obvious? I fail to see how this would clash with other filters. The only thing that will happen is that the transmission won't be filtered past the first frame until the first condition re-evaluates to False.

What if the plugin wants a list of hidden entities?

@TransmitFilter(criteria=TransmitCriteria.is_player)
def transmit_filter(entity, player):
    #The filter that might hide the player, which you didn't create.
    return True

hidden_set = set()

@TransmitFilter(criteria=TransmitCriteria.is_player)
def transmit_filter(entity, player):
    if (not player.dead and not entity.dead and
        player.team_index == entity.team_index and
        player.origin.get_distance(entity.origin) < 2000):
        hidden_set.add(entity.index)
        return True
    else:
        hidden_set.discard(entity.index)
        return False

@Event("player_say")
def on_player_say(game_event):
    if game_event["text"] == "hidden_list":
        for index in hidden_set:
            print(Player(index).name)
            # Nothing will be printed.

As long as the callback may be writing outside the filter, not calling it is a problem. Since the filter is applied dynamically, to know if the filter actually rewrites the hide state/show state, you need to either allow > external writes, i.e. perform all callbacks, or apply all entities and all players to the filter again when you want to know, and see the results. Edit: But since we don't know what will be written, all callbacks should be executed. A plugin might count the times it is hidden, a reference to something might rewrite memory, and so on.

The fact that @PreHook and @PostHook execute a callback even when function execution is suppressed, while @TransmitFilter has no guarantee that it will be executed is inconsistent and also strange.

I don't think that is inconsistent nor strange. As long as the behaviour is documented I don't think this is an issue. The likelihood of someone needing to do anything else than filtering the transmission in their callbacks is very slim and doesn't worth the trade-off in extra computation. To me filters are not hooks, nor listeners, nor events, and should really be interpreted as being a "condition" rather than a callback. To take your example, it should really just do:

@TransmitFilter(criteria=TransmitCriteria.is_player)
def transmit_filter(entity, player):
    return (not player.dead and not entity.dead and
        player.team_index == entity.team_index and
        player.origin.get_distance(entity.origin) < 2000)

The hidden_set in your example would also be unreliable, because a transmission can be filtered out by the game, another filter, or a hook for that matters. If there is a need to know entities that were transmitted or not, I think it would be overall better to simply offer a listener that give them access to such info. For example, simply calling an OnCheckTransmit listener after the filtering is done would give them access to the entire transmission set containing the final states without the overhead of attempting to treat filters as listeners. Other than that, I really can't see any generic usages that could justify the extra computation it would introduce to treat them differently.

I understand the purpose of SetTransmit and the reason for hooking it. I can also see why you don't like the feature that introduces clashes between plugins. But it is also true that at some point someone will need to introduce a clashes to manage the server. My biggest fear is that when a player is hidden in @TransmitFilter(criteria=TransmitCriteria.is_player) it could be used by a malicious player to cheat, hack, etc., over the eyes of Admin or other players. Maybe I'm overthinking this, but I don't like the idea of being able to casually hide a player. We need the ability to inhibit someone's change to the hide state to suppress it, but maybe I'm overthinking that too.

Yeah, I think you are overthinking it. If a transmission filter can be exploited on your server, you probably have worst security issues to be concerned about. If the exploit is in the plugin itself, then an admin can simply unload it and report the exploit to their developers, etc. for it to be fixed. As long as there is no exploit in the API itself, and that it does exactly what it is meant for, its misuse should be none of our concerns as this becomes the responsibility of the scripters to ensure their plugins can't be exploited and admins to ensure they don't install plugins from malicious sources.

CookStar commented 3 years ago

I don't think that is inconsistent nor strange. As long as the behaviour is documented I don't think this is an issue. The likelihood of someone needing to do anything else than filtering the transmission in their callbacks is very slim and doesn't worth the trade-off in extra computation. To me filters are not hooks, nor listeners, nor events, and should really be interpreted as being a "condition" rather than a callback.

Doesn't that conflict with the same flexibility as SetTransmit?

The hidden_set in your example would also be unreliable, because a transmission can be filtered out by the game, another filter, or a hook for that matters.

That may be a problem, but it has nothing to do with the issue here.

If there is a need to know entities that were transmitted or not, I think it would be overall better to simply offer a listener that give them access to such info. For example, simply calling an OnCheckTransmit listener after the filtering is done would give them access to the entire transmission set containing the final states without the overhead of attempting to treat filters as listeners. Other than that, I really can't see any generic usages that could justify the extra computation it would introduce to treat them differently.

What the plugin wants to know is which entities match the filter's conditions, not which entities are finally hidden. To find out which entities are hidden by the filter, the plugin actually has to check all the (hidden) entities against the filter and then re-match them with the condition.

This is a plugin where a player within 500 units will leave a footprint when they are hidden. This plugin is for example.

footprint

But as I said before, this kind of plugin will not work with other filters.

# Python Imports
#   Collections
from collections import defaultdict

# Source.Python Imports
#   Effects
from effects.base import TempEntity
#   Engines
from engines.server import engine_server
#   Entities
from entities.transmit import TransmitCriteria
from entities.transmit import TransmitFilter
#   Events
from events import Event
#   Filters
from filters.recipients import RecipientFilter
#   Player
from players.entity import Player

def init_recipient():
    filter = RecipientFilter()
    filter.remove_all_players()
    return filter

hidden_dict = defaultdict(init_recipient)

@TransmitFilter(criteria=TransmitCriteria.is_player)
def transmit_filter_else(entity, player):
    # The filter that might hide the player, which you didn't create.
    return True

@TransmitFilter(criteria=TransmitCriteria.is_player)
def transmit_filter(entity, player):
    if (not entity.dead and not player.dead and
        entity.team_index == player.team_index and
        entity.origin.get_distance(player.origin) < 500):
        hidden_dict[entity.index].add_recipient(player.index)
        return True
    else:
        hidden_dict[entity.index].remove_recipient(player.index)
        return False

@Event("player_footstep")
def on_player_footstep(game_event):
    player = Player.from_userid(game_event["userid"])
    if player.index in hidden_dict:
        foot_print = TempEntity("Footprint Decal")
        foot_print.origin = player.origin
        player.view_angle.get_angle_vectors(right=foot_print.direction)
        foot_print.decal_index = engine_server.precache_decal("footprint.vmt", True)
        foot_print.entity_index = 0
        foot_print.create(hidden_dict[player.index])

But it works when you hook SetTransmit.

@PreHook(Entity(0).set_transmit)
def pre_set_transmit_else(args):
    # The hook that might hide the player, which you didn't create.
    entity = make_object(Entity, args[0])

    if entity.is_player():
        entity = Player(entity.index)
    else:
        return

    player = Player(
        index_from_edict(make_object(CheckTransmitInfo, args[1]).client))

    if entity.index == player.index:
        return

    return False

@PreHook(Entity(0).set_transmit)
def pre_set_transmit(args):
    entity = make_object(Entity, args[0])

    if entity.is_player():
        entity = Player(entity.index)
    else:
        return

    player = Player(
        index_from_edict(make_object(CheckTransmitInfo, args[1]).client))

    if entity.index == player.index:
        return

    if (not entity.dead and not player.dead and
        entity.team_index == player.team_index and
        entity.origin.get_distance(player.origin) < 500):
        hidden_dict[entity.index].add_recipient(player.index)
        return False

    hidden_dict[entity.index].remove_recipient(player.index)

And that's why I said this.

The fact that @PreHook and @PostHook execute a callback even when function execution is suppressed, while @TransmitFilter has no guarantee that it will be executed is inconsistent and also strange.

I'm not arguing here that this plugin doesn't work. I'm just saying that TransmitFilter doesn't offer the same flexibility as SetTransmit, and since we don't know what the plugin creators are implementing in SetTransmit, we need to provide an easy and intuitive solution to this problem.

The likelihood of someone needing to do anything else than filtering the transmission in their callbacks is very slim and doesn't worth the trade-off in extra computation.

You have a point, but I think you also said that we need the same flexibility as SetTransmit. I think if we are implementing Filter, it's worth running all the Filters, even if it's an extra computation. Otherwise, won't the plugin creators end up using SetTransmit?

And Sourcemod doesn't seem to have this problem either...

Yeah, I think you are overthinking it. If a transmission filter can be exploited on your server, you probably have worst security issues to be concerned about. If the exploit is in the plugin itself, then an admin can simply unload it and report the exploit to their developers, etc. for it to be fixed. As long as there is no exploit in the API itself, and that it does exactly what it is meant for, its misuse should be none of our concerns as this becomes the responsibility of the scripters to ensure their plugins can't be exploited and admins to ensure they don't install plugins from malicious sources.

I'm not talking about an exploit. I'm saying that malicious players encourage the use of cheats and hacks by not being transmited. I've seen all sorts of things like speed hacks done by malicious players when no one is watching, and an increase of WH when no spectators are present. A malicious player will always know they are not being transmited, and will use this to fool the Admin or player.

jordanbriere commented 3 years ago

What the plugin wants to know is which entities match the filter's conditions, not which entities are finally hidden. To find out which entities are hidden by the filter, the plugin actually has to check all the (hidden) entities against the filter and then re-match them with the condition.

As I previously stated, scripters shouldn't use a TransmitFilter, nor a SetTransmit hook as a listener. Both are not guaranteed to be called. It all comes down to; when the transmission is being filtered out (which can be through a state, a flag, ShouldTransmit, UpdateTransmitState, etc.). Take the following example:

from entities.entity import Entity
from entities.hooks import EntityPreHook
from entities.hooks import EntityCondition
from players.entity import Player

pl = Player(1)
awp = Entity.find_or_create('weapon_awp')
awp.teleport(pl.view_coordinates)
awp.spawn()

@EntityPreHook(
    EntityCondition.equals_entity_classname('weapon_awp'),
    'set_transmit'
)
def pre_set_transmit(stack_data):
    if stack_data[0] != awp.pointer:
        return
    print('set_transmit')

This will create an awp in front of you and spam the console with set_transmit. Now simply flag it as no longer transmitted:

awp.edict.state_flags |= 1 << 4 # FL_EDICT_DONTSEND

And your pre_set_transmit callback will not be called anymore because the game already filtered it out of the transmission. In your example, this should be the responsibility of your plugin to build that list when it is relevant (e.g. every footsteps) then from there should update its filter to override the transmission. Otherwise, your plugin will not draw footsteps regardless if the transmission was filtered out by a filter or the game itself.

In conclusion; none of them should be used as listener, period. Both provides the same amount of incertitude whether they will be interrogated or not. This is the difference between a listener that is used to listen and is called no matter what and a filter that is used to filter. If there is nothing left to filter, there is no reason to interrogate further. I actually had the same opinion about what is the role of a filter almost 8 years ago and I will still have the same 8 years from now. 😛

CookStar commented 3 years ago

As I previously stated, scripters shouldn't use a TransmitFilter, nor a SetTransmit hook as a listener. Both are not guaranteed to be called. It all comes down to; when the transmission is being filtered out (which can be through a state, a flag, ShouldTransmit, UpdateTransmitState, etc.). Take the following example:

from entities.entity import Entity
from entities.hooks import EntityPreHook
from entities.hooks import EntityCondition
from players.entity import Player

pl = Player(1)
awp = Entity.find_or_create('weapon_awp')
awp.teleport(pl.view_coordinates)
awp.spawn()

@EntityPreHook(
    EntityCondition.equals_entity_classname('weapon_awp'),
    'set_transmit'
)
def pre_set_transmit(stack_data):
    if stack_data[0] != awp.pointer:
        return
    print('set_transmit')

This will create an awp in front of you and spam the console with set_transmit. Now simply flag it as no longer transmitted:

awp.edict.state_flags |= 1 << 4 # FL_EDICT_DONTSEND

And your pre_set_transmit callback will not be called anymore because the game already filtered it out of the transmission.

Yes, I know. And I'm not sure what you mean by that. Flags can be given and stripped at any time, and that's what other plugins actually do within SetTransmit, isn't it?

Examples: https://github.com/vinci6k/floating-damage-numbers/blob/15ffcc15f9d65d1e726dc2d6b44647a08525a285/addons/source-python/plugins/fdn/fdn.py#L28 https://git.empiresmod.com/sourcemod/EmpStats/-/blob/3cfaf36757299fc2c5aedac656c0ec1bd59518c6/dist/addons/sourcemod/scripting/empstats.sp#L2819

If you want a guarantee that SetTransmit will run, you just need to manipulate the flag.

As I previously stated, scripters shouldn't use a TransmitFilter, nor a SetTransmit hook as a listener. Both are not guaranteed to be called. It all comes down to;

TransmitFilter aside, we shouldn't dictate how plugins use SetTransmit. Plugin creators are free to use hooks in any way they want, as long as there is a way around the problem.

In your example, this should be the responsibility of your plugin to build that list when it is relevant (e.g. every footsteps) then from there should update its filter to override the transmission. Otherwise, your plugin will not draw footsteps regardless if the transmission was filtered out by a filter or the game itself.

I don't know what exactly you mean by this one, either. Is this equivalent to what I said about this?

To find out which entities are hidden by the filter, the plugin actually has to check all the (hidden) entities against the filter and then re-match them with the condition.

Examples:

@TransmitFilter(criteria=TransmitCriteria.is_player)
def transmit_filter(entity, player):
    if (not entity.dead and not player.dead and
        entity.team_index == player.team_index and
        entity.origin.get_distance(player.origin) < 500):
        return True

@Event("player_footstep")
def on_player_footstep(game_event):
    player = Player.from_userid(game_event["userid"])
    foot_print = TempEntity("Footprint Decal")
    foot_print.origin = player.origin
    player.view_angle.get_angle_vectors(right=foot_print.direction)
    foot_print.decal_index = engine_server.precache_decal("footprint.vmt", True)
    foot_print.entity_index = 0
    filter = RecipientFilter()
    filter.remove_all_players()
    for index in [other.index for other in PlayerIter() if transmit_filter(player, other)]:
        filter.add_recipient(index)
    filter.remove_recipient(player.index)
    foot_print.create(filter)

And this, as I said, comes down to this.

we need to provide an easy and intuitive solution to this problem.

We need either a direct feature that solves the problem or solid documentation that shows how plugin creators can get around the limitations.

Still, though, in my opinion, I don't think we should create such a limitation.

jordanbriere commented 3 years ago

Yes, I know. And I'm not sure what you mean by that. Flags can be given and stripped at any time, and that's what other plugins actually do within SetTransmit, isn't it?

Examples: https://github.com/vinci6k/floating-damage-numbers/blob/15ffcc15f9d65d1e726dc2d6b44647a08525a285/addons/source-python/plugins/fdn/fdn.py#L28 https://git.empiresmod.com/sourcemod/EmpStats/-/blob/3cfaf36757299fc2c5aedac656c0ec1bd59518c6/dist/addons/sourcemod/scripting/empstats.sp#L2819

If you want a guarantee that SetTransmit will run, you just need to manipulate the flag.

I gave that flag as an example to illustrate that a SetTransmit hook isn't guaranteed to be called. Just like a TransmitFilter shouldn't. If the transmission is filtered out by anything else as mentioned above (e.g. a ShouldTransmit you can't manipulate), you can't do anything regardless. And even if you can get away with it by manipulating that flag, you have no guarantee it won't be externally mutated. We shouldn't use the fact that scripters may or may not use a hook on a function primarily used for filtering as if it was a listener guaranteed to be called to justify an increase of P*E*C calls. Where P is the amount of players, E the amount of networked entities and C the amount of extra callbacks. For example, let's assume there is currently 64 players, 1000 props, and that all of them are rendered by everyone. You have 3 plugins on your servers that wish to filter the transmission of props:

# Plugin A
@TransmitFilter(criteria=is_prop_physics)
def transmit_filter(entity, player):
    return True

# Plugin B
@TransmitFilter(criteria=is_prop_physics)
def transmit_filter(entity, player):
    pass

# Plugin C
@TransmitFilter(criteria=is_prop_physics)
def transmit_filter(entity, player):
    pass

Plugin A already filtered the transmission, there is no reason to interrogate plugin B nor C whatsoever. The transmission is already filtered and should be done with. Doing so, just in case they are using an intended filter as listener would means 64*1000*2 = 128,000 extra calls every frame. Plus, computing any comparisons they make in their filters, etc. This can't be justified in an effort to accommodate and facilitate what is considered to be a misuse to begin with.

I don't know what exactly you mean by this one, either. Is this equivalent to what I said about this?

To find out which entities are hidden by the filter, the plugin actually has to check all the (hidden) entities against the filter and then re-match them with the condition.

You don't have to test twice, you can simply set their states in your criteria when you want to override their transmission. Regardless, the point is that you shouldn't use a TransmitFilter nor a SetTransmit hook as if it was an OnTickAndPVS listener because they aren't. Not only they are not guaranteed to be called because their use is to filter the transmissions, but it is much more noisy than when it is relevant for your plugin; on footstep. This is where you should do your work, because not only it is called way less frequently (every other frames, and only when players are actually moving), but is guaranteed to be called because this is what events are for; listen.

We need either a direct feature that solves the problem or solid documentation that shows how plugin creators can get around the limitations.

I'm not opposed to add features that would make it easier, and I actually have some ideas I will experiment with when I get the chance. All I know for now is that; calling all filters can't be justified in any way shapes or forms I can think about.

CookStar commented 3 years ago

If the transmission is filtered out by anything else as mentioned above (e.g. a ShouldTransmit you can't manipulate), you can't do anything regardless.

You can. Just hook ShouldTransmit and return FL_EDICT_ALWAYS(1<<3). I am not aware of any situation where SetTransmit is not possible.

We shouldn't use the fact that scripters may or may not use a hook on a function primarily used for filtering as if it was a listener guaranteed to be called to justify an increase of P*E*C calls.

I understand this as well. This is one of the main reasons I tried to scrap the callback itself. I think what we need is an enforceable system that fundamentally reduces the number of entities to filter.

Also on this issue I came up with a flag that would force a filter, but it hasn't come together yet.

You don't have to test twice, you can simply set their states in your criteria when you want to override their transmission.

No. As the filter is dynamic, you need to test it twice or move the test itself to Tick.

I'm not opposed to add features that would make it easier, and I actually have some ideas I will experiment with when I get the chance. All I know for now is that; calling all filters can't be justified in any way shapes or forms I can think about.

I have no issue if there is a feature that allows us to get around this problem. Even if there was no justification for using SetTransmit for that purpose (OnTickAndPVS listener) it would still be used that way to avoid the design problem, so I think the filter needs a solution that doesn't create such a problem in the first place. I'm looking forward to your ideas.

jordanbriere commented 3 years ago

You can. Just hook ShouldTransmit and return FL_EDICT_ALWAYS(1<<3). I am not aware of any situation where SetTransmit is not possible.

I'm not sure if I should laugh or cry at this point. I know that you understand the point very well, and I too could go around and find ways that would break that, etc. but that would just be silly. Regardless if you can get around with it through extra hooks, flags, waiting for all the stars and planets to be perfectly aligned, etc. it doesn't change the fact that in itself, it isn't guaranteed to be called and no matter how hard you hack your way to get it called it can easily be broken by external sources you have no control over. I don't plan to extend any further than that on this matter because we are just going circle anyways.

No. As the filter is dynamic, you need to test it twice or move the test itself to Tick.

Not if you use a static filter instead. You test players when it is relevant to do so (on footsteps) and override their states there. You don't need to test players every frames in that specific example because the main factor that determine whether you should draw decals and hide players is through that event. That may prove to be difficult now that I think about it, because the current public branch only support 1 criteria per filter but from the start the plan was to allow for more that can either test the player or the entity through TransmitTarget, etc. so that you could have one criterion that validate the player, then another that uses static states that you set on footsteps, etc.

I have no issue if there is a feature that allows us to get around this problem. Even if there was no justification for using SetTransmit for that purpose (OnTickAndPVS listener) it would still be used that way to avoid the design problem, so I think the filter needs a solution that doesn't create such a problem in the first place. I'm looking forward to your ideas.

It will more be for convenience than anything else. I don't think there is anything to "fix" because I don't see that as a "problem" over it being a misuse that accidentally works in a very specific scenario.

Anyways, I've unsubbed from receiving notifications from this thread for the time being because I personally find the discussion to have become stale and going nowhere. And that I spend more time replying here, when I should rather use that time to write code instead. I will come back when I have progress to share, though as usual, no ETA on my part.

CookStar commented 3 years ago

I'm not sure if I should laugh or cry at this point. I know that you understand the point very well, and I too could go around and find ways that would break that, etc. but that would just be silly. Regardless if you can get around with it through extra hooks, flags, waiting for all the stars and planets to be perfectly aligned, etc. it doesn't change the fact that in itself, it isn't guaranteed to be called and no matter how hard you hack your way to get it called it can easily be broken by external sources you have no control over. I don't plan to extend any further than that on this matter because we are just going circle anyways.

What I'm trying to say is that plugin creators will use anything to solve the problem. It doesn't matter what the game is designed to do, or what the API designers think, claim, or whether they are right or not. If all you have is a hammer, everything looks like a nail. http://users.svn.alliedmods.net/viewvc.cgi/ShouldTransmit/transmit.inc?root=pred&view=markup

Not if you use a static filter instead. You test players when it is relevant to do so (on footsteps) and override their states there. You don't need to test players every frames in that specific example because the main factor that determine whether you should draw decals and hide players is through that event.

Decal's part, yes, but not with hide. In your words:

Also, admin swap a player from T to CT; your plugin is broken until next round.

The very fact that we are having this long discussion is proof that the problem is deep-rooted.

It will more be for convenience than anything else. I don't think there is anything to "fix" because I don't see that as a "problem" over it being a misuse that accidentally works in a very specific scenario.

The problem is that there is no way for the plugin to know which entities are hidden, or would be hidden, by its own filters. We need either a direct feature that can solve the problem or proper documentation to get around the problem.

If you still think there is no problem in the first place, then I'm not saying anything else.

jordanbriere commented 3 years ago

Hmm, I still get emails for some reasons. I think the "Participating" setting have stronger priority than "Watching". Anyways, I will just reply to your last post this time and then I will take a break from this thread because I feel exasperated every times I post and I'm sure you feel the same way and backing off for a bit may be best for both of us.

If all you have is a hammer, everything looks like a nail.

I must admit this is a very true statement. It remembered me of the old ES days where we were left to find hacky ways to circumvent all its limitations and lack of features.

Decal's part, yes, but not with hide. In your words:

Also, admin swap a player from T to CT; your plugin is broken until next round.

The very fact that we are having this long discussion is proof that the problem is deep-rooted.

You are right. I overlooked the fact that you are also testing for same teams and based the overall goal of your plugin on the following statement:

This is a plugin where a player within 500 units will leave a footprint when they are hidden.

I'm not sure if this would be a real-use scenario, but this sure shed lights on the possibility that it -might- and I guess this is what you were trying to illustrate. Which I think would be addressed with the idea I was thinking about. I have plan to add an "interval" system which allows to cache the result of a filter and re-use that result for the set amount of frame so that scripters can make their callbacks less noisy when a per-frame precision isn't required. For example:

@TransmitFilter(interval=10)
def transmit_filter(player, entity):
    ...

Which on a 100 tick server, would only test that filter and its criteria 10 times per seconds instead of 100, etc. and would for the most part not even be noticeable from a game-play perspective but would considerably reduce the behind the scenes computation even more. So anyways, the idea I mentioned there:

I actually have some ideas I will experiment with when I get the chance.

Is to simply allows access to such cache through:

transmit_filter[player.index][entity.index]

Which would return the state if it was tested, or test it if the entity wasn't recently transmitted to that player. This would allows you to do an if on that inside your player_footstep callback without testing twice on the same frame, etc. From a theory point of view, I think this would address the scenario you are mentioning, right?

Velocity-plus commented 2 years ago

Is there any issues with handling settransmit the same way SDKHooks from SourceMod does it? As far as I know, this hook has never been a problem in sourcemod performance wise.

https://github.com/alliedmodders/sourcemod/blob/master/extensions/sdkhooks/extension.cpp#L1321

CookStar commented 2 years ago

If I'm not mistaken, SourceMod is faster than Source.Python because it only hooks into specific entities, but that doesn't mean it doesn't impact performance.

I wouldn't try to use it with a ton a people on the server, usually those are when the server starts to see performance drops due to the SetTransmit happening every frame. https://forums.alliedmods.net/showpost.php?p=2628712&postcount=81

Me and jordanbriere have proposed different implementations for further performance improvements, but they have been halted by disagreements. I am planning a new implementation based on the feedback, but I can't promise anything.

If you have any comments on the implementation or usage of SetTransmit, please don't hesitate to let us know.

Velocity-plus commented 2 years ago

I usually use SetTransmit combined with ServerPassEntityFilter, to disable collisions between entities and hide specific entities at the same time. For example, func_wall entity, so for some players the wall is hidden and there is no collision allowing them to walk through it, while others can still see it and cannot walk through it. I have raised my concerns with the pass entity filter here tho, which also requires some work: https://sourcepython.com/viewtopic.php?f=20&t=2624&sid=9fd857c0418a721de4aea6ac2b7dd8bb

I must say I do not have any performance issues when implemented in sourcemod, but with sourcepython the 'server var' starts to go crazy after a few players.

Nonetheless, It seemed like you guys already exchanged many ideas already, rather implement them all than none of them, more choice, some of the implementations might be best in certain situations, where 'one has it all' might not exist. Anyways, at this point reeeeaaaally looking forward to trying anything that is faster than the current implementation in sourcepython, or just same as sourcemod at least.

CookStar commented 2 years ago

The biggest difference between jordanbriere and my implementation is how much dynamic processing is left.

I implemented the hidden state/shown state like a flag. https://github.com/Source-Python-Dev-Team/Source.Python/pull/353

Please let me know if you want to try it or if you have any other suggestions. I will distribute binaries.

Velocity-plus commented 2 years ago

Yes I would love to try it out. I'm on Linux.

CookStar commented 2 years ago

Yes I would love to try it out. I'm on Linux.

I assume it's CS:GO, but here it is. csgo.zip

Copy the .py file from #353 and overwrite it, or just import transmit_manager via from _entities._transmit import transmit_manager and use it.

Velocity-plus commented 2 years ago

You assumed correctly! Sorry :) I'll try to implement it and see if I have any problems then I will let you know :)

Velocity-plus commented 2 years ago

Server crash: Host_Error: SV_CreatePacketEntities: GetEntServerClass failed for ent 172. Host_Error: SV_CreatePacketEntities: GetEntServerClass failed for ent 172.

Edit 1: I think there is an issue if an entity is removed while having transmit flag on/off Edit 3: I can confirm the issue. I worked around it by calling reset() before deletion.

Also, this forces somewhat very recursive behavior, imagine having 64 players and different preferences for who can see who. You have for each player loop through all players to set the correct hide_from and show_from with a stack depth of 4096, but I guess this is the way to do it.

Edit 2: Now that I think about it, an improvement might to keep track of shown and hidden entities, you have a get_hidden_entities, but maybe get_shown_entities (entities that are called by show_from, or show and not all in general) would be good too, but I could keep track of it myself I guess.

Velocity-plus commented 2 years ago

After doing the workarounds, it works like a charm :)

CookStar commented 2 years ago

Server crash: Host_Error: SV_CreatePacketEntities: GetEntServerClass failed for ent 172. Host_Error: SV_CreatePacketEntities: GetEntServerClass failed for ent 172.

Edit 1: I think there is an issue if an entity is removed while having transmit flag on/off Edit 3: I can confirm the issue. I worked around it by calling reset() before deletion.

I should have made the fixes, sorry about that.

Also, this forces somewhat very recursive behavior, imagine having 64 players and different preferences for who can see who. You have for each player loop through all players to set the correct hide_from and show_from with a stack depth of 4096, but I guess this is the way to do it.

Yes, you are correct. However, it is also unavoidable. When there are only a few entities to manipulate, SetTransmit or SourceMod implementations are relatively straightforward and lightweight, the problem is when there are many entities to manipulate.

Velocity-plus commented 2 years ago

I should have made the fixes, sorry about that.

Np, this is what feedback is for. Feel free to let me know when it's fixed if you got the time :)

Velocity-plus commented 2 years ago

I have some new information regarding bugs I have found, besides one already reported. I think there are inconsistencies with .show_from and .hide_from happening in the backend. I cannot see anything in server console, but are they updated properly? What happens if you do .show_from twice on the same entity, etc. It appears sometimes its not working, what happens on map change etc... Does it still keep track of old entities?

Also, I think a set_transmit listener like in SourceMod would be good too (where the filtering on entities is handled on c++ side), specially when plugins need .show_from and .hide_from to change all the time between entities (I do not think that this method is worth it in all scenarios, but definitely some). Probably be better to have a combination of those.