Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

1.18.2 - buggy getShape implementation #758

Open
embeddedt opened this issue Mar 11, 2023 · 6 comments
Open

1.18.2 - buggy getShape implementation #758

embeddedt opened this issue Mar 11, 2023 · 6 comments
Labels
1.16.5 Related to the 1.16.5 version 1.18.2 Related to the 1.18.2 version bug stale exempt This issue/PR will not go stale

Comments

@embeddedt
Copy link

embeddedt commented Mar 11, 2023

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
Copy link
Author

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

@Harleyoc1 Harleyoc1 added 1.16.5 Related to the 1.16.5 version 1.18.2 Related to the 1.18.2 version labels Mar 18, 2023
Harleyoc1 added a commit that referenced this issue Mar 31, 2023
Harleyoc1 added a commit that referenced this issue Mar 31, 2023
@github-actions
Copy link

Stale issue message

@Harleyoc1
Copy link
Member

Harleyoc1 commented Jul 20, 2023

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
Copy link
Author

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
Copy link
Member

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
Copy link

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

@github-actions github-actions bot closed this as completed Nov 2, 2023
@Harleyoc1 Harleyoc1 reopened this Nov 2, 2023
@Harleyoc1 Harleyoc1 added stale exempt This issue/PR will not go stale and removed no-issue-activity labels Nov 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.16.5 Related to the 1.16.5 version 1.18.2 Related to the 1.18.2 version bug stale exempt This issue/PR will not go stale
Projects
None yet
Development

No branches or pull requests

2 participants