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

Do not provide world context during off-thread tooltip retrieval #710

Merged

Conversation

embeddedt
Copy link
Contributor

The world/entity are not thread safe, so they should not be provided to Item#getTooltip calls from the search thread.

Note that the issue described in #709 mainly affects NeoForge, as it exposes the passed world to mods, while vanilla just uses it for the tick rate & registry managers. https://github.com/neoforged/NeoForge/blob/42023dc3e3284b7046791403db04649999389ba3/patches/net/minecraft/world/item/Item.java.patch#L221-L222

@emilyploszaj
Copy link
Owner

Is this an acceptable resolution? If I remember correctly, this context was added in the past due to errant mods crashing without it, will this not just revert that behavior? This seems like a rock and a hard place of trusting downstream mods and needing multithreading. Insight would be appreciated.

@SiverDX
Copy link

SiverDX commented Sep 24, 2024

But doesn't vanilla do this as well (on 1.21 at least)?
SessionSearchTrees -> updateRecipes / updateCreativeTooltips

NeoForge has annotated the method itself as @Nullable as well so personally I'd say it falls on the mods to handle this properly
Also as I understand it this is mostly an 1.21 issue since the level is provided by NeoForge (which means if mods have problems it would be reasonable to expect them to fix this themselves since it is the latest version as of now)

@SiverDX
Copy link

SiverDX commented Sep 24, 2024

You seem to have added the level for #575
But the PR would use vanilla behaviour which still supplies access to the registries

@embeddedt
Copy link
Contributor Author

As above, we still provide registry context (as the registry should be thread-safe) but we cannot provide the whole level safely when not on the main thread, as mods can easily do things like roll the RNG in it.

@emilyploszaj
Copy link
Owner

Alright, reasonable enough. I'll be getting crash reports of one of the two either way.

@emilyploszaj emilyploszaj merged commit cc0e0e7 into emilyploszaj:1.21 Sep 25, 2024
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants