Throttle download net message

[brandonsturgeon]

This should address a relatively minor issue that would allow malicious players to spam-download a text screen's current state.

It simply throttles the download net message to once per second.

Because the throttles are stored on the player, the memory for the throttles should be self-managed as players leave.

I also reworked the net update itself:
Moving the networking into a function on the entity will let other addons control if/when screen updates occur.

[Cherry]

Thanks so much for this PR! I will review this and merge as soon as I get a chance - likely at the weekend. 😄

[zakarybk]

@brandonsturgeon I don't know if I'm missing something, but can you explain what happens when you join a server for the first time with this pull request?

I'm concerned that the user will automatically run the network request textscreens_download from ENT:Initialize() for as many times as there are placed text screens - resulting in data not being sent if the client is capped to 1 request a second, where a different request is required for each entity.

I would instead propose a cap on each individual text screen. I presume the player can only ever request this once per screen as GMod isn't going to try and retry the network requests, and the client only ever calls it when the entity is initialised on the client. So the cap could be once per text screen.

Future updates are directly triggered by the server and sent to the client, so they wouldn't be affected by any caps.

In terms of implementation, I'm thinking of using your existing idea of tying the data to the player entity so that it's cleaned up on disconnect, however, as it needs to keep track of each text screen entity that they have received an update for, you might want to add an EntityRemoved https://wiki.facepunch.com/gmod/GM:EntityRemoved hook to clean up removed text screens from the player's text screen update table.

Hope this helps :)

[brandonsturgeon]

Hey @zakarybk - thanks for the reply.

I'm booked up today, but I'll get some answers for you tomorrow.
Thanks for the well thought-out review!

[Cherry]

Let me know once you've had a chance to address Zak's comments @brandonsturgeon. Thanks again for the PR!

[brandonsturgeon]

Sorry, I'm injured and unable to do much right now. I'll circle back when I recover. Thanks

[Cherry]

Oh no worries at all - wishing you a speedy recovery!

[brandonsturgeon]

Sorry for the long delay. Life got away from me for awhile there :)

what happens when you join a server for the first time with this pull request?

I tested this by spawning 10 screens on top of spawn and then rejoining.

I added a print to ENT:SendLines and the net Receiver for textscreen_download, here are the logs produced from me spawning in:

textscreens_download    Player [1][Phatso]      Entity [94][sammyservers_textscreen]
textscreens_update      Player [1][Phatso]      Entity [94][sammyservers_textscreen]

textscreens_download    Player [1][Phatso]      Entity [101][sammyservers_textscreen]
textscreens_update      Player [1][Phatso]      Entity [101][sammyservers_textscreen]

textscreens_download    Player [1][Phatso]      Entity [102][sammyservers_textscreen]
textscreens_update      Player [1][Phatso]      Entity [102][sammyservers_textscreen]

textscreens_download    Player [1][Phatso]      Entity [103][sammyservers_textscreen]
textscreens_update      Player [1][Phatso]      Entity [103][sammyservers_textscreen]

textscreens_download    Player [1][Phatso]      Entity [98][sammyservers_textscreen]
textscreens_update      Player [1][Phatso]      Entity [98][sammyservers_textscreen]

textscreens_download    Player [1][Phatso]      Entity [99][sammyservers_textscreen]
textscreens_update      Player [1][Phatso]      Entity [99][sammyservers_textscreen]

textscreens_download    Player [1][Phatso]      Entity [100][sammyservers_textscreen]
textscreens_update      Player [1][Phatso]      Entity [100][sammyservers_textscreen]

textscreens_download    Player [1][Phatso]      Entity [104][sammyservers_textscreen]
textscreens_update      Player [1][Phatso]      Entity [104][sammyservers_textscreen]

textscreens_download    Player [1][Phatso]      Entity [105][sammyservers_textscreen]
textscreens_update      Player [1][Phatso]      Entity [105][sammyservers_textscreen]

textscreens_download    Player [1][Phatso]      Entity [106][sammyservers_textscreen]
textscreens_update      Player [1][Phatso]      Entity [106][sammyservers_textscreen]

And here's what the tracking table looked like on my player ent:

me.TextScreenUpdates
	-- 0xe8ae6caa
	{
		[Entity (100) --[[ sammyservers_textscreen ]]] = 212.30303955078,
		[Entity (101) --[[ sammyservers_textscreen ]]] = 200.98484802246,
		[Entity (102) --[[ sammyservers_textscreen ]]] = 200.98484802246,
		[Entity (103) --[[ sammyservers_textscreen ]]] = 200.98484802246,
		[Entity (104) --[[ sammyservers_textscreen ]]] = 212.30303955078,
		[Entity (105) --[[ sammyservers_textscreen ]]] = 212.30303955078,
		[Entity (106) --[[ sammyservers_textscreen ]]] = 212.66667175293,
		[Entity ( 94) --[[ sammyservers_textscreen ]]] = 200.98484802246,
		[Entity ( 98) --[[ sammyservers_textscreen ]]] = 212.30303955078,
		[Entity ( 99) --[[ sammyservers_textscreen ]]] = 212.30303955078
	}

Visually, everything seemed to work fine. All of the screens requested their data and displayed it correctly on the screen 👍

---

I would instead propose a cap on each individual text screen.

This is how it works currently. Each player has a table on them keeping track of the last time they requested the lines for a given screen entity ([ent] = timestamp). Is that what you were thinking?

---

however, as it needs to keep track of each text screen entity that they have received an update for, you might want to add an EntityRemoved

Good point! I added a new OnRemove method to the textscreens that will clean themselves up from any player's updates table. It appears to work as expected locally 👍

[zakarybk]

Hi @brandonsturgeon, nice work. The logic looks sound. Thanks for attaching your tests. I haven't tested locally because I'm sure @Cherry will do that.

[Cherry]

This looks great, thanks Brandon! Sorry for the delay in review on my part. This'll land on the Workshop very shortly.