Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

requestrevivalstatus is an expensive netmessage #1386

Open
EntranceJew opened this issue Feb 8, 2024 · 12 comments
Open

requestrevivalstatus is an expensive netmessage #1386

EntranceJew opened this issue Feb 8, 2024 · 12 comments
Labels
type/bug Something isn't working type/rework Big changes or overhaul of an existing feature

Comments

@EntranceJew
Copy link
Contributor

Your version of TTT2 (mandatory)

  • Latest GitHub version

Video

image
image

Describe the bug (mandatory)

requestrevivalstatus is an expensive netmessage

To reproduce

  1. Find a dead person
  2. Try to revive them with a defib, or potentially other means
  3. Produce 60 netmessages while this occurs.

Expected behavior

The client doesn't produce so many netmessages.

Context (please provide as much as you can)

  • The code is probably firing in UI code or some trace/tick situation
  • While it isn't long-running or time consuming to process like ttt2_net_stream, it does still burden the server.
  • It was a highly occurring message in proportion to the amount of times it was utilized during the playsession.
@TimGoll
Copy link
Member

TimGoll commented Feb 8, 2024

I remember we had a discussion about this when I added the revival UI. I wanted to have that variable shared so that I can access it in targetID. But we ultimately decided on not having it shared. This means that every addon that needs that info (the targetID of the defi for example) has to poll that information from the server.

Maybe it is now time to revert that change and make the value shared again.

@EntranceJew
Copy link
Contributor Author

image
can you spot where i was holding a defib

@TimGoll
Copy link
Member

TimGoll commented Feb 8, 2024

A band-aid fix would be to limit the defi to send this request only once per second or so. I would still prefer to just have that value shared

@Histalek
Copy link
Member

Histalek commented Feb 8, 2024

i'm somewhat inclined to agree with Tim on making this shared.

IIRC our last discussion about this ended up with the netmessage way because we wanted to prevent other players snooping on revives, but seeing the sheer amount of netmessages players could at least assume that someone is getting revived.

Maybe there is some middle ground to be found.

@TimGoll
Copy link
Member

TimGoll commented Feb 8, 2024

i'm somewhat inclined to agree with Tim on making this shared.

IIRC our last discussion about this ended up with the netmessage way because we wanted to prevent other players snooping on revives, but seeing the sheer amount of netmessages players could at least assume that someone is getting revived.

Maybe there is some middle ground to be found.

The cleanest solution would probably be a polling in a good intrerval. But it is way more complex then a shared variable which would reduce the traffic a lot.

Because by polling the client has to request the data. If the server only updates the data when changed, then the traffic is vastly reduced

@saibotk
Copy link
Member

saibotk commented Feb 8, 2024

I think this is not a direct issue for TTT2 but rather the defib repo.

Even though fixing this might also need some changes in TTT2. The defib only needs this for the TargetID, idk about the functionality and what it really does, like is this for other players to see that the corpse is being revived?
Sounds very niche if that is the case and id rather suggest removing this for now.

@TimGoll should we rather move this to the defib repo?

@TimGoll
Copy link
Member

TimGoll commented Feb 8, 2024

If we move this to the defi, we need the same custom systen for the marker and the necro.

I'm still in favor of changing this in TTT2

@saibotk
Copy link
Member

saibotk commented Feb 8, 2024

TTT2 does not expose anything like this and i don't really see what you would "change"?
This is some custom functionality added by the defib alone. Just for displaying it in the target id.

@TimGoll
Copy link
Member

TimGoll commented Feb 8, 2024

TTT2 exposes the revival status on the server. I have three addons that need it on the client.

The best solution would be to have this exposed on the client as well. The other solution would be to implement a better custom syncing system in all three addons, which means I have to maintain in three times. Therefore I'd like to just use a shared variable on the player ent in TTT2. It is easy, vastly reduces net traffic ans pretty stable

This is also how I implemented it years ago before you requested me to remove it. As I see the problems with the removal now clearly, I want to add it back

@saibotk
Copy link
Member

saibotk commented Feb 8, 2024

Do that no one is against that.
But you did not answer my question. What is this used for and in which context? Without this it is much harder to reason about possible solutions

@TimGoll
Copy link
Member

TimGoll commented Feb 8, 2024

I'm against implementing the same syncing code in three addons and maintaining it in three addons. So I don't know which solution noone is against

It is used to show the holder of the defi if they are able to use the defi on a player

@EntranceJew
Copy link
Contributor Author

I think the base game should just handle it FWIW, it's not just the defib that could potentially break with a corpse being revived. For example, cannibalizing a swapper prior to respawning might break things. Cannibalizing a player killed by the infected could also break things prior to their respawn. There's a bunch of situations where authors should be able to consider a corpse "engaged". Fultoning a burnt coprse, cannibalizing a fultoned corpse, reviving a burning corpse, cannibalizing a burning corpse ...

@TimGoll TimGoll added type/bug Something isn't working keepalive labels Feb 12, 2024
@TimGoll TimGoll added this to the v0.14.0b milestone Feb 12, 2024
@Histalek Histalek added the type/rework Big changes or overhaul of an existing feature label Feb 26, 2024
@Histalek Histalek modified the milestones: v0.14.0b, Feature Backlog Mar 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug Something isn't working type/rework Big changes or overhaul of an existing feature
Projects
None yet
Development

No branches or pull requests

4 participants