this was caused by 083a1e1ff6b6ff5c0c1ba14250d2b026af103b90.
This was discovered by a new PHPStan rule I'm working on, which disallows overwriting the values of parameter variables. During the refactor of this function to correct the error, another error appeared: Variable might not be defined.
This is yet another excellent example of why mutability is bad.
this is not as good as phpstan/phpstan-src#769 (e.g. array_key_first()/array_key_last() aren't covered by this, nor is array_rand()) but it does eliminate the most infuriating cases where this usually crops up.
this allows chunks locked for population to be modified. If the PopulationTask detects that the chunk was modified during the onCompletion(), the result of the population will be discarded and rescheduled, so that it includes user modifications.
This reverts commit a5418a019dc2a83210084632130db8ce06f529ea.
The more I assessed this, the more I realized that this implementation
doesn't actually offer any value. Since modcounters don't persist after
chunk unload + reload, they can't be reliably used to detect changes in
chunks without additional event subscriptions.
For the purpose I actually intended to use them for (population task
cancellation) there's a) another solution, and b) modcounts are
unreliable for that too, because of the aforementioned potential for
chunks to get unloaded and reloaded.
For the case of detecting dirty chunks within PopulationTask itself,
they are also unnecessary, since the dirty flags are sufficient within
there, since FastChunkSerializer doesn't copy dirty flags.
In conclusion, this was a misbegotten addition with little real value,
but does impact performance in hot paths.
instead, just ungate this and allow the provider to decide what to do.
Any chunk that contains entities or tiles is already always considered dirty, so the only thing the flags are good for is flagging chunks that previously had tiles and/or entities but no longer do.
In those cases, it's just removing keys from LevelDB anyway, so it's already very cheap.
Avoiding these redundant deletions is not worth the extra complexity and fragility of relying on flags to track this stuff.
This event is currently fired for tree and bamboo growth. Its intended use is for any plant growth that affects multiple blocks at once.
TODO: We could explore using this for cacti and sugarcane?
this makes more sense overall from a reader's perspective.
and also provide a rug-jerk for any idiots using World->close() when they aren't supposed to? ....
