If the same chunk is set into multiple different places in the world, the chunk's position is no longer able to be relied on, because it will have the position of the last place it was set. This results in chunks not getting sent correctly when the same chunk is set in multiple places.
This avoids the bug by using known valid coordinates (using chunk hashes) to establish the real coordinates, and also adds an assert to notify developers should they unintentionally set a duplicate chunk by mistake.
- Removed `Server->getScheduler()`. All plugins now have their own scheduler which is accessible using `Plugin->getScheduler()`. Aside from being syntactically more concise and pleasant, this also allows much more effective management of tasks when plugins are disabled.
- Removed `PluginTask` class. Before this PR it was necessary for plugin tasks to descend from `PluginTask` to ensure that the server could clean them up correctly on plugin disable. This is no longer necessary, so the `PluginTask` class has been removed. Plugins may now utilize the `Task` class as a base if they like.
- Added `Server->getAsyncPool()`. Since the global scheduler does not exist any more, it does not manage the server's `AsyncPool` any more. Additionally, `ServerScheduler` was previously bloated by a lot of `AsyncTask` related methods, which are now not necessary because direct access to `AsyncPool` is granted instead.
- `ServerScheduler`:
- `ServerScheduler` has been renamed to `TaskScheduler` since it is now a general-purpose task scheduler which is non-dependent on the user. This allows much greater flexibility and also makes it possible to unit-test.
- All `AsyncTask`/`AsyncPool` related methods have been removed - the task scheduler does not manage the async pool anymore.
- Calls to `Server->getScheduler()->scheduleAsyncTask()` should be replaced with `Server->getAsyncPool()->submitTask()`.
- Calls to `Server->getScheduler()->scheduleAsyncTaskToWorker()` should be replaced with and `Server->getAsyncPool()->submitTaskToWorker()`.
## Backwards compatibility
This poses significant backwards compatibility breaks for any plugins utilizing Tasks or AsyncTasks. These breaks are described above, along with basic upgrade steps. The upgrade process is quite straightforward.
## Follow-up
A large part of the goal with this pull request is to modularize these parts of the code so that they can be reused and also unit-tested. I would like to remove the existing test set from TesterPlugin at some stage when the AsyncPool can operate without a Server.
Because of the above, I am considering making further backwards incompatible changes directly to `AsyncTask` to remove the `Server` parameters from `onCompletion()` and `onProgressUpdate()`. These shouldn't be too difficult to upgrade from and can be prepared for in advance.
This commit contains quite a few breaking changes with respect to how AsyncTasks are handled. This is necessary to allow separation of the ServerScheduler and the AsyncPool, because in the future the ServerScheduler may be removed and instead there will be isolated per-plugin sync-task schedulers - but we cannot have every plugin with its own worker pool for memory usage reasons if nothing else.
The following things have changed:
- ServerScheduler: scheduleAsyncTask(), scheduleAsyncTaskToWorker(), getAsyncTaskPoolSize(), increaseAsyncTaskPoolSize() and similar methods have all been removed. Additionally the static \$WORKERS field has been removed.
- Server: added API method getAsyncPool(). This grants you direct access to the server's AsyncPool. Calls to getScheduler()->scheduleAsyncTask() and scheduleAsyncTaskToWorker() should be replaced with getAsyncPool()->submitTask() and submitTaskToWorker() respectively.
Once upon a time, these userland functions were faster than calling builtins, but not anymore. According to my test the Math functions are twice slower in PHP 7.2 with typehints and 50% slower without typehints.
Inlining is slightly faster than using builtins, but the difference is very small - not worth making the code look any more ugly than it does already.
This is the same bug that Entity->getBlocksAround() had, except this actually checks for BB intersections. What this means is that as a result of the bug, one extra layer of blocks is unnecessarily checked on the max sides of the BB.
For example:
Assume you have a BB with maxY -5.5
You're definitely colliding with block -6 (because you're inside it) and you want to check an extra block in case you hit something weird like a fence.
So you want to check _at most_ up to block -5 (inclusive).
Following this maths:
-5.5 + 1 = -4.5
ceil(-4.5) = -4
This causes us to check block -4 unnecessarily. This may be a performance waste - depending on the BB size it could be proportionally a lot of blocks getting unnecessarily checked. This has not been benchmarked.
this is only used in one place, where it's being given floats, and it's 10% faster to use int for this because it won't convert it.
It is also 25% faster to remove typehints and 60% faster to inline it. We really need a proper PHP preprocessor for inlining.
long overdue... this isn't quite as extensible as the original api3/blocks system was, but this is primarily intended to replace Item->useOn(). If plugins want to use it it can be extended later on.
this was causing unexpected behaviour particularly on burning trees, whereby fire would be unconditionally extinguished if the block below it was removed.
This happened when a block was set into the world with a direct update, when an entry for that block was already present in the changedBlocks map. This fixes the bug by removing the entry from the changedBlocks map to avoid sending outdated block updates in batches.
Chunks were used by tiles for a couple of things:
- 1. for coordinates - which can be gotten using bitshifts
- 2. setChanged() - which is unnecessary as seen in the previous commit
Removing this circular dependency was actually remarkably easy to do.
