Skip to content

Configurable map update interval#940

Closed
minhh2792 wants to merge 4 commits into
PurpurMC:ver/1.18.2from
minhh2792:feature/map-update-interval
Closed

Configurable map update interval#940
minhh2792 wants to merge 4 commits into
PurpurMC:ver/1.18.2from
minhh2792:feature/map-update-interval

Conversation

@minhh2792

Copy link
Copy Markdown

Mojang decided to increase the update interval of map to 5 ticks, in some server which utilize it this is kinda slow

This PR made the value configurable so the server admin can adjust as their liking

I tested this and having no issue, however I can't record a comparison video of it because of my potato pc :'(

Comment thread patches/server/0272-Configurable-map-update-interval.patch Outdated
Comment thread patches/server/0272-Configurable-map-update-interval.patch Outdated
@minhh2792

Copy link
Copy Markdown
Author

Moved to per world config, everything has been tested. This PR is now ready to merge

@kev626

kev626 commented Mar 28, 2022

Copy link
Copy Markdown

This patch makes no sense and should not be included.

First, you can see in this patch, if the map contains dirty data, then it will always be immediately rendered with no delay. Configuring the update interval will have no noticeable impact on performance nor will it have any noticeable impact on player appearance to the game due to this dirty check.

Second, there's no way your players are noticing quarter-second delay in maps, even if it were happening.

@minhh2792

minhh2792 commented Mar 28, 2022

Copy link
Copy Markdown
Author

This patch is not meant to be "performance improvement" anyway

A Minecraft CS:GO server that utilizes the Map, and 5 ticks delay per update is really slow and noticeable

Edit link: https://youtu.be/i_0NrTb2Rsc

@kev626

kev626 commented Mar 28, 2022

Copy link
Copy Markdown

This patch is not meant to be "performance improvement" anyway

A Minecraft CS:GO server that utilizes the Map, and 5 ticks delay per update is really slow and noticeable

Edit link: https://youtu.be/i_0NrTb2Rsc

Then do you explain the presence of the “dirty” check?

@minhh2792

Copy link
Copy Markdown
Author

What dirty check

@minhh2792

Copy link
Copy Markdown
Author

This patch has nothing to do with the check tho, it just make the interval configurable

@kev626

kev626 commented Mar 28, 2022

Copy link
Copy Markdown

What dirty check

if (!this.dirtyData && …)
Did you even read the patch you were modifying?

@minhh2792

Copy link
Copy Markdown
Author

This patch has nothing to do with the check tho, it just make the interval configurable

@kev626

kev626 commented Mar 28, 2022

Copy link
Copy Markdown

This patch has nothing to do with the check tho, it just make the interval configurable

This is why I think you didn’t even read the patch you’re attempting to modify. If the data used to render the map is changed, the map already updates immediately. In this sense, the interval doesn’t even matter.

The interval only serves to update the map data if for some reason the data is changed without the dirty flag being set (which should be never). Having the interval configurable at all is totally useless, as it’s value does not have any impact whatsoever on gameplay.

@minhh2792

minhh2792 commented Mar 28, 2022

Copy link
Copy Markdown
Author

I appreciate the clarification, in my case, the map (you can watch the video, it's faster) is slow. Changing the value works for me so I make this PR, maybe other servers also having this problem as well

If this PR is closed then I will just port it to my fork :fork:

@amadeusmz

amadeusmz commented Mar 30, 2022

Copy link
Copy Markdown

@kev626 It will not be immediately rendered, it will check for dirty every 5 ticks as I told you on Pufferfish Discord.
This patch worked for me and I really need it.

@kev626

kev626 commented Mar 30, 2022

Copy link
Copy Markdown

@kev626 It will not be immediately rendered, it will check for dirty every 5 ticks as I told you on Pufferfish Discord.

This patch worked for me and I really need it.

If you need a map to be immediately rendered to a player, you can do this via the API by calling Player#sendMap(MapView). There is no need for any patch.

Regardless, it's not my project and purpur is free to add whatever they like, but this is truly not necessary.

@cittyinthecloud

Copy link
Copy Markdown

This patch actually fixes a mismatch between API behavior in Spigot and Paper. Perhaps this would be better brought up upstream?

@minhh2792 minhh2792 closed this Apr 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants