DynamicTreesTeam / DynamicTrees

Minecraft Forge mod providing dynamic trees that progressively grow from seed to maturity.
MIT License
218 stars 96 forks source link

1.18.2 - buggy getShape implementation #758

Open embeddedt opened 1 year ago

embeddedt commented 1 year ago

I received a crash report. when Dynamic Trees is installed alongside my performance mod ModernFix. Upon further investigation, it seems that Dynamic Trees has a buggy getShape implementation for its blocks. On some seeds this leads to the game entirely deadlocking when it tries to generate chunks (e.g. on seed -8079293905500261883 with no other mods except Dynamic Trees and ModernFix installed); in other cases it seems to lead to an endless recursive loop of calling shape methods on blocks until the game crashes (see the log given in the linked issue).

The reason these issues are not visible when ModernFix isn't installed is because vanilla caches block shapes immediately upon launch, while ModernFix defers the building of this cache and runs it in the background. This means that your block's getShape handler can get invoked with a real world rather than the fake empty world vanilla uses. I believe it is actually an oversight on your end that the cache is even used for your blocks at all, as it seems that you're intending to provide a shape based on nearby branches. You probably need to set the dynamicShape flag when initializing your block properties.

The only mods required to reproduce this issue are Dynamic Trees and ModernFix.

embeddedt commented 1 year ago

I've received a similar crash report on 1.16; I assume the implementation is pretty much the same.

github-actions[bot] commented 1 year ago

Stale issue message

Harleyoc1 commented 1 year ago

I've done some more experimenting with this as someone bringing up compatibility issues with ModernFix reminded me of this issue (whether or not the recent reports are related to this I have yet to determine).

Firstly, setting dynamicShape to true does not fix the issue at all. In fact, it makes the deadlock occur almost instantly on any world load (with or without ModernFix). Regardless I'm pretty sure this is for blocks whose shape needs to change without a block update, as I've seen no other consequences of it being disabled.

Anyway, the instant deadlock with dynamicShape enabled allowed me to debug this issue quite easily. It turns out to be relating to vanilla's threaded light engine calling getOcclusionShape on a branch at the same time as the chunk is still loading. Still looking for a clean solution to this which doesn't involve disabling occlusion (which is also obviously not an option).

embeddedt commented 1 year ago

When dynamicShape is off, vanilla will call most of the shape methods once on an empty world with a position of (0, 0, 0) and cache the result, rather than repeatedly calling them, which is why it prevents the deadlock. ModernFix didn't perfectly emulate that behavior in all scenarios before; nowadays it does due to compatibility issues like this.

Regardless I'm pretty sure this is for blocks whose shape needs to change without a block update, as I've seen no other consequences of it being disabled.

Yes, basically you need dynamicShape if your shape depends at all on the surrounding blocks/context in the world, and not just on having a given blockstate.

someone bringing up compatibility issues with ModernFix reminded me of this issue (whether or not the recent reports are related to this I have yet to determine).

The other issue is with an optimization in ModernFix that isn't on by default; I think it's not related to this.

Harleyoc1 commented 1 year ago

Yes, basically you need dynamicShape if your shape depends at all on the surrounding blocks/context in the world, and not just on having a given blockstate.

The problem with this is it seems to majorly slow down the game whenever near dynamic trees, seen as without the cache things like the light engine call getShape a lot. Which in turn means a lot of adjacent block checks.

github-actions[bot] commented 9 months ago

This issue has gone stale due to inactivity. Please comment again if it is still an issue.