Mineclonia
/
Mineclonia
Archived
12
17
Fork 13
Code Issues 105 Pull Requests 19 Projects Releases Wiki Activity

PLAYER/mcl_playerplus: send player object props only if changed #147

Merged
erlehmann merged 2 commits from fix-active-object-message-spam into master 2021-12-09 10:31:11 +01:00
Conversation 4 Commits 2 Files Changed 1 +129 -23
cora commented 2021-09-13 23:54:50 +02:00
Member
Copy Link
Problem

TRACKING ISSUE: #128
Mineclonia sends a huge amount of TOCLIENT_ACTIVE_OBJECT_MESSAGE even if none of the target properties have changed. This (or at least to like 99%) is caused by the globalstep function in PLAYER/mcl_playerplus/init.lua which in turn performs a set of player:set_bone_position() and player:set_properties() - these minetest functions send the data over network regardless if it was changed even though it is also stored in the player object.

Solution

Since the engine does not check if properties have changed we're going to have to do it ourselves.

Details

This PR implements two wrapper functions for player:set_bone_position() and player:set_properties.

These functions perform the corresponding get_, compare the new values with the already stored ones and only send the packet when they differ (enough).

The values for some player properties seem to be adjusted minimally by the engine so that the numbers have to be rounded to two decimal places before comparing.

Testing Steps
Reproducing and testing the fix
  1. Start Mineclonia without this PR in an empty single node world.
  2. Start a traffic measurement tool like jnettop (jnettop -i lo to see local connections)
  3. After initial bursts traffic should settle to about 10 kb/s without this PR and to about 100-500 b/s with this PR
Regression tests
  1. Start Mineclonia on this PR
  2. Verify that your eye height (camera position) is visibly lower when you crouch (default: ctrl key)
  3. Log in with a second client
  4. Verify that with the other client you can see the following:
    • head pitch is still visible
    • The nametag disappears when sneaking
    • Charging of a bow is visible (mcl_bows:bow)
    • sitting in a boat makes the player visibly sit (mcl_boats:boat)

To Do

  • Figure out the set_properties part - doesn't seem to work correctly rn.

  • write proper testing instructions

##### Problem TRACKING ISSUE: #128 Mineclonia sends a huge amount of TOCLIENT_ACTIVE_OBJECT_MESSAGE even if none of the target properties have changed. This (or at least to like 99%) is caused by the globalstep function in PLAYER/mcl_playerplus/init.lua which in turn performs a set of player:set_bone_position() and player:set_properties() - these minetest functions send the data over network regardless if it was changed even though it is also stored in the player object. ##### Solution Since the engine does not check if properties have changed we're going to have to do it ourselves. ##### Details This PR implements two wrapper functions for player:set_bone_position() and player:set_properties. These functions perform the corresponding get_, compare the new values with the already stored ones and only send the packet when they differ (enough). The values for some player properties seem to be adjusted minimally by the engine so that the numbers have to be rounded to two decimal places before comparing. ##### Testing Steps ###### Reproducing and testing the fix 1. Start Mineclonia without this PR in an empty single node world. 2. Start a traffic measurement tool like jnettop (jnettop -i lo to see local connections) 3. After initial bursts traffic should settle to about 10 kb/s without this PR and to about 100-500 b/s with this PR ###### Regression tests 1. Start Mineclonia on this PR 2. Verify that your eye height (camera position) is visibly lower when you crouch (default: ctrl key) 3. Log in with a second client 4. Verify that with the other client you can see the following: - head pitch is still visible - The nametag disappears when sneaking - Charging of a bow is visible (mcl_bows:bow) - sitting in a boat makes the player visibly sit (mcl_boats:boat) ### To Do - [x] Figure out the set_properties part - doesn't seem to work correctly rn. - [x] write proper testing instructions
cora reviewed 2021-09-14 00:04:36 +02:00
mods/PLAYER/mcl_playerplus/init.lua Outdated
@ -22,0 +48,4 @@
local function set_properties_conditional(player,props)
local oldprops=player:get_properties()
local p={}
local changed=false
cora commented 2021-09-14 00:04:36 +02:00
Author
Member

this obviously isn't needed^^

this obviously isn't needed^^
cora marked this conversation as resolved
cora changed title from WIP: mcl_playerplus: only send object props if changed to WIP: mcl_playerplus: send player object props only if changed 2021-09-15 04:24:55 +02:00
cora added the
needs testing
packet spam
performance
 labels 2021-09-22 18:57:14 +02:00
cora force-pushed fix-active-object-message-spam from 76ce59417c to 2015815351 2021-11-09 12:45:22 +01:00 Compare
cora force-pushed fix-active-object-message-spam from 2015815351 to c85892db92 2021-11-09 12:46:28 +01:00 Compare
cora force-pushed fix-active-object-message-spam from c85892db92 to 7199060eaf 2021-11-09 12:48:59 +01:00 Compare
cora force-pushed fix-active-object-message-spam from 7199060eaf to 8086d060b2 2021-11-09 12:54:03 +01:00 Compare
cora force-pushed fix-active-object-message-spam from 8086d060b2 to fa68e01b93 2021-11-21 21:06:46 +01:00 Compare
cora changed title from WIP: mcl_playerplus: send player object props only if changed to mcl_playerplus: send player object props only if changed 2021-11-21 21:20:26 +01:00
cora changed title from mcl_playerplus: send player object props only if changed to PLAYER/mcl_playerplus: send player object props only if changed 2021-11-21 21:24:21 +01:00
cora removed the
needs testing
 label 2021-11-21 21:24:55 +01:00
cora force-pushed fix-active-object-message-spam from fa68e01b93 to d19efef53e 2021-11-21 21:44:50 +01:00 Compare
cora force-pushed fix-active-object-message-spam from d19efef53e to 7db083cfe2 2021-11-21 22:27:01 +01:00 Compare
cora requested review from erlehmann 2021-11-27 20:27:35 +01:00
erlehmann reviewed 2021-12-05 00:29:22 +01:00
mods/PLAYER/mcl_playerplus/init.lua Outdated
@ -24,0 +40,4 @@
return rt
end
local function set_properties_conditional(player,props)
erlehmann commented 2021-12-05 00:29:22 +01:00
Owner

Why did you not overwrite the set_properties function of the player so that the real one is never called from the code below?

Why did you not overwrite the `set_properties` function of the player so that the real one is never called from the code below?
cora commented 2021-12-05 01:46:15 +01:00
Author
Member

I suppose i liked it better that way. It didn't seem necessary to overwrite it for this. Everything happens in one file (in one function to be exact).

I suppose i liked it better that way. It didn't seem necessary to overwrite it for this. Everything happens in one file (in one function to be exact).
cora marked this conversation as resolved
erlehmann requested changes 2021-12-08 18:31:05 +01:00
erlehmann left a comment
Owner

This patch breaks eye level adjustment on sneaking.

The following code prints 1.35 1.65 true:

local function roundN(n, d)
	if type(n) ~= "number" then return n end
    local m = 10^d
    return math.floor(n * m + 0.5) / m
end

local function close_enough(a,b)
	local rt=true
	if type(a) == "table" then
		for k,v in pairs(a) do
			if roundN(v,2) ~= roundN(b[k],2) then
				rt=false
				break
			end
		end
	else
		rt = roundN(a,2) ~= roundN(b,2)
	end
	return rt
end

local sneak = 1.35
local normal = 1.65
-- eye_height (sneaking)
print(sneak, normal, close_enough(sneak, normal))
This patch breaks eye level adjustment on sneaking. The following code prints `1.35 1.65 true`: ``` local function roundN(n, d) if type(n) ~= "number" then return n end local m = 10^d return math.floor(n * m + 0.5) / m end local function close_enough(a,b) local rt=true if type(a) == "table" then for k,v in pairs(a) do if roundN(v,2) ~= roundN(b[k],2) then rt=false break end end else rt = roundN(a,2) ~= roundN(b,2) end return rt end local sneak = 1.35 local normal = 1.65 -- eye_height (sneaking) print(sneak, normal, close_enough(sneak, normal)) ```
mods/PLAYER/mcl_playerplus/init.lua Outdated
@ -24,0 +42,4 @@
local function set_properties_conditional(player,props)
local oldprops=player:get_properties()
local changed=true
erlehmann commented 2021-12-08 18:25:09 +01:00
Owner

Changed is always true. Why?

Changed is always true. Why?
cora commented 2021-12-09 03:24:56 +01:00
Author
Member

cause i dum

cause i dum
erlehmann commented 2021-12-08 18:41:42 +01:00
Owner
Copy Link

This patch breaks eye level adjustment on sneaking.

It also breaks eye level adjustment on entering a boat and swimming fast.

> This patch breaks eye level adjustment on sneaking. It also breaks eye level adjustment on entering a boat and swimming fast.
erlehmann changed title from PLAYER/mcl_playerplus: send player object props only if changed to WIP: PLAYER/mcl_playerplus: send player object props only if changed 2021-12-08 18:42:15 +01:00
erlehmann reviewed 2021-12-08 18:43:02 +01:00
mods/PLAYER/mcl_playerplus/init.lua Outdated
@ -23,1 +25,4 @@
return math.floor(n * m + 0.5) / m
end
local function close_enough(a,b)
erlehmann commented 2021-12-08 18:43:02 +01:00
Owner

Suggestion: Add a few asserts to show how this function works correctly.

Suggestion: Add a few asserts to show how this function works correctly.
cora commented 2021-12-08 22:49:50 +01:00
Author
Member

yes i did that in this case

yes i did that in this case
erlehmann reviewed 2021-12-08 18:43:34 +01:00
mods/PLAYER/mcl_playerplus/init.lua Outdated
@ -24,0 +44,4 @@
local oldprops=player:get_properties()
local changed=true
local p={}
for k,v in pairs(props) do
erlehmann commented 2021-12-08 18:43:34 +01:00
Owner

Suggestion: Add a few asserts to show how this function works correctly.

Suggestion: Add a few asserts to show how this function works correctly.
cora commented 2021-12-08 23:03:12 +01:00
Author
Member

I think it makes the whole thing a lot more complex and harder to understand but ... sure i'll even make uselessly long variable names.

I think it makes the whole thing a lot more complex and *harder* to understand but ... sure i'll even make uselessly long variable names.
cora commented 2021-12-08 22:31:10 +01:00
Author
Member
Copy Link

you're right there's a simple mistake. i'll push in a second.

you're right there's a simple mistake. i'll push in a second.
cora force-pushed fix-active-object-message-spam from 7db083cfe2 to 95bff518cf 2021-12-08 22:32:32 +01:00 Compare
cora force-pushed fix-active-object-message-spam from c3fdd25f51 to 9600823d74 2021-12-08 23:06:12 +01:00 Compare
cora requested review from erlehmann 2021-12-08 23:50:19 +01:00
cora force-pushed fix-active-object-message-spam from 9600823d74 to db43d87788 2021-12-09 00:08:16 +01:00 Compare
cora force-pushed fix-active-object-message-spam from db43d87788 to 875583cd45 2021-12-09 02:08:10 +01:00 Compare
erlehmann changed title from WIP: PLAYER/mcl_playerplus: send player object props only if changed to PLAYER/mcl_playerplus: send player object props only if changed 2021-12-09 03:59:00 +01:00
erlehmann force-pushed fix-active-object-message-spam from 875583cd45 to 5deaabdb47 2021-12-09 05:02:36 +01:00 Compare
erlehmann commented 2021-12-09 05:11:42 +01:00
Owner
Copy Link

I have updated the commit messages so future software archaeologists have an easier time following the twisted logic of this patchset.

I have updated the commit messages so future software archaeologists have an easier time following the twisted logic of this patchset.
erlehmann commented 2021-12-09 06:34:41 +01:00
Owner
Copy Link
Testing
Verify bug
  • Start Mineclonia without this PR in an empty single node world.
  • Start a traffic measurement tool like jnettop (jnettop -i lo to see local connections)
  • After initial bursts traffic should settle to about 10 kb/s without this PR […]
Verify patch
  • Start Mineclonia without this PR in an empty single node world.
  • Start a traffic measurement tool like jnettop (jnettop -i lo to see local connections)
  • After initial bursts traffic should settle […] to about 100-500 b/s with this PR

I got 47 to 125 bytes per second with commit 5deaabdb47.

Good job!

Regression tests
  • Start Mineclonia on this PR
  • Verify that your eye height (camera position) is visibly lower when you crouch (default: ctrl key)

I also tested sitting in a boat, as the eye level should be lower there too (it was).

Multiplayer

  • Log in with a second client

  • Verify that with the other client you can see the following:

    • head pitch is still visible
    • The nametag disappears when sneaking
    • Charging of a bow is visible (mcl_bows:bow)
    • sitting in a boat makes the player visibly sit (mcl_boats:boat)
##### Testing ###### Verify bug - [x] Start Mineclonia without this PR in an empty single node world. - [x] Start a traffic measurement tool like jnettop (jnettop -i lo to see local connections) - [x] After initial bursts traffic should settle to about 10 kb/s without this PR […] ###### Verify patch - [x] Start Mineclonia without this PR in an empty single node world. - [x] Start a traffic measurement tool like jnettop (jnettop -i lo to see local connections) - [x] After initial bursts traffic should settle […] to about 100-500 b/s with this PR I got 47 to 125 bytes per second with commit 5deaabdb47454fa3b198d6a28a891bb87f31e545. Good job! ###### Regression tests - [x] Start Mineclonia on this PR - [x] Verify that your eye height (camera position) is visibly lower when you crouch (default: ctrl key) I also tested sitting in a boat, as the eye level should be lower there too (it was). ###### Multiplayer - [x] Log in with a second client - [x] Verify that with the other client you can see the following: - [x] head pitch is still visible - [x] The nametag disappears when sneaking - [x] Charging of a bow is visible (mcl_bows:bow) - [x] sitting in a boat makes the player visibly sit (mcl_boats:boat)
erlehmann approved these changes 2021-12-09 10:30:49 +01:00
erlehmann left a comment
Owner

Useful performance fix with a big impact. Thank you!

Useful performance fix with a big impact. Thank you!
erlehmann merged commit ee77f33ea8 into master 2021-12-09 10:31:11 +01:00
erlehmann deleted branch fix-active-object-message-spam 2021-12-09 10:31:29 +01:00
This repo is archived. You cannot comment on pull requests.
Reviewers
No reviewers
erlehmann
Labels
Clear labels   
blocker

blocks release until resolved

  
bug

   
code quality

   
confirmed

   
critical

crashes, catastrophic bugs

  
discussion

   
high priority

   
incompatibility

incompatible with mcl2

  
incomplete feature

   
invalid

   
low priority

   
missing feauture

   
needs testing

   
packet spam

   
performance

   
project

meta-issues

  
regression

   
translations

   
unconfirmed
  
in review

   
ready for review
No Label
blocker
bug
code quality
confirmed
critical
discussion
high priority
incompatibility
incomplete feature
invalid
low priority
missing feauture
needs testing
packet spam
performance
project
regression
translations
unconfirmed
in review
ready for review
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
2 Participants
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: Mineclonia/Mineclonia#147
No description provided.
Delete Branch "fix-active-object-message-spam"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?