webbukkit / dynmap

A set of Minecraft mods that provide a real time web-based map system for various Minecraft server implementations.
https://www.reddit.com/r/Dynmap/
Apache License 2.0
2.03k stars 412 forks source link

Support for Async Chunk Loading (Paper) #2345

Closed aikar closed 2 years ago

aikar commented 5 years ago

Hi mike, it's been a long time since we spoke!

Paper has an API addition that allows plugins to request chunks asynchronously. This API has existed since 1.9 roughly.

I'm requesting DynMap add support for this by detecting if World#getChunkAtAsync exists, and if it does, use an alternative chunk loader that uses the Paper API method.

By doing this, Paper will be able to asynchronously load all chunks. We also have a World#isGenerated(x,z) method that you may find useful too since you were using NMS for that logic already.

I am also planning to add a new method in 1.13 to "getChunkAtAsyncWithoutLoading" kind of concept, that would suit dynmaps needs, in that we can load the chunk but never post it into the world.

This would remove your need to immediately unload the chunk.

Many server owners are switching to Paper, and I'm hoping you can add this support so that dynmap can be nearly free in performance cost.

I would love if you would join the Paper discord, and I can speak more with you on there if you need :) https://paperdiscord.emc.gs

Thanks, hope to reconnect soon!

smmmadden commented 5 years ago

Hi @aikar as mentioned in Discord not sure if you saw this or not or if it's something Mike would need to make changes for, but wanted to share nonetheless. https://github.com/webbukkit/dynmap/issues/2344

FWIW - I don't know whether my timings are typical or atypical but it is at least one benchmark. -Steve

jacklollz2 commented 5 years ago

I'd love to see Async chunks implemented! The server performance runs very poorly when dynmap generates chunks.

generrosity commented 5 years ago

I'm going to out on a limb, but my understanding from talking to the only dev is that 'Spigot' and 'Forge' are the two primary platforms that he works on and support, and SpongeForge and Paper do get consideration but no active work. There isn't a Paper fork, so comiling a feature not in Spigot would likely throw errors on other platforms.

I'm sure multithreading is on the back of his mind as an optimisation, but at this stage he is likely focused purely on 1.13 implementation.

If you are interested in the code, I think passive chunk updates are done as on-thread events, and the /dynmap fullrender is a distinct command but could be easiest split off if there was a "easy" way to a) confirm we were running Paper not Spigot and b) could run the async execution without upsetting Spigot for just having the command nearby

I hope that makes sense?

smmmadden commented 5 years ago

I'm running my servers on PaperSpigot 1.12.2 and 1.13.2 with Dynmap and haven't seen any issues using it. Everything I did find earlier on in the 1.13.x testing has been addressed already. Hope that helps.

IAmGadget commented 3 years ago

bump

rautamiekka commented 3 years ago

Absolutely good idea.

crystall1nedev commented 3 years ago

@aikar

@generrosity To your comment, maybe it could be an option in the configuration? I use FastChunkPregenerator from SpigotMC and it has an option to turn async on and off, also preventing async chunk loading from even working on non-PaperMC servers. This is a good idea, could I see about getting to work on this?

jacklollz2 commented 3 years ago

Dynmap really needs an improvement on performance, it's pretty bad compared to alternatives now.

crystall1nedev commented 3 years ago

@jacklollz2 I have been looking around at PaperMC's specific API library and found a function called getChunkAtAsync() that can get the job done. The docs say that it has compatibility with Spigot, using the normal synced loading as a fallback if Paper is not available.

I can start working on enabling async chunk loading right now, on my fork's v3.0-async branch that I just created. I don't know how long it will take but it looks like there's only a couple of files that need to be adapted for async...

EDIT: Scratch that, I forgot I has to add the Paper Libraries to Gradle. Still going to work on it, however.

crystall1nedev commented 3 years ago

As you can see, my draft pull request will contain the functionality for async.

generrosity commented 3 years ago

Im not a dev 😁 but if you folk, confirm working, get a few other people confirming its working, then it becomes the new great thing.

(Im not sure where the dev on this is at now - a few of us were assisting, but we suddenly got radio silence before we got access to help out and I haven't heard word back)

crystall1nedev commented 3 years ago

Ahh, okay. Well, either way I'm still working on it for the people that use Dynmap.

crystall1nedev commented 3 years ago

I'm restarting my work, it's all a mess and trying to reverse it is only making it worse.

Drift91 commented 2 years ago

@Doregon Are you still working on this or plan to work on it sometime in the near future? I'm just asking so I know whether or not to get my hopes up. 😃

Apparently this issue is caused by lack of async chunk support. So I'm hopeful that adding support will fix the rendering.

crystall1nedev commented 2 years ago

@Drift91 I hate to be the bearer of bad news but I've abandoned that pull request as I've stepped on to do other things, and working with Objective-C has weakened my understanding of Java. I too hope this gets implemented for future people to take advantage of, but due to the lack of a Minecraft server myself anymore and what was mentioned above, I'm not going to continue to work on this.

Drift91 commented 2 years ago

@Doregon No worries, man. From my testing it seems that the lack of async chunk support isn't even causing the issue I mentioned. I'm curious though, what effect does the lack of support have? Is it just a performance improvement in the API that isn't being taken advantage of?

crystall1nedev commented 2 years ago

It's a performance hit that causes rendering time to skyrocket.

FedUpWith-Tech commented 2 years ago

Not a duplicate but would fix all issues linked in #2485, going to lock this now as it should be tracked in separate PRs where appropriate

FedUpWith-Tech commented 2 years ago

Thank you for your feature request. Due to the volume of feature requests we receive we have relocated all of our features requests to a single issue #3461.

generrosity commented 2 years ago

@Doregon - if you haven't already, come join Discord and the #dev channel. Having someone who runs spigot/paper would be really good, and we can chat and support you on this easier (coz I imagine it wasn't as easy as swapping sync for async).

generrosity commented 2 years ago

Same @aikar - Mike might be the only person truely experianced with spigot, so having your input would be highly valued