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

Fix #817: Simplify internal recipe building #893

Open
wants to merge 1 commit into
base: 1.21.x
Choose a base branch
from

Conversation

Swedz
Copy link
Contributor

@Swedz Swedz commented Sep 17, 2024

Closes #817

This is the code I've been using in a mixin in Tesseract for a while now, and it seems to work properly. It doesn't feel quite optimized to me though.

@Swedz Swedz marked this pull request as ready for review September 17, 2024 20:55
@Swedz
Copy link
Contributor Author

Swedz commented Sep 18, 2024

It might be worth considering changing ProxyableMachineRecipeType#fillRecipeList to have the RecipeManager and RegistryAccess as parameters instead of the Level. I can make this change too if that would be desirable

@Technici4n
Copy link
Contributor

All the weird cache invalidation should not be called on the client; it might lead to outdated recipes in JEI forever. Passing the recipe manager and registry access is fine.
Speaking of the invalidation, I think it's also time to remove it, and instead invalidate after a data pack (re)load using an event. This should remain server-exclusive behavior.

@Swedz
Copy link
Contributor Author

Swedz commented Sep 19, 2024

Is the cache invalidation something I did in this change? I don't see what would cause it to be invalidating.
If there's something somewhere where this is happening I can change it to use a datapack event listener.

I found it, the stuff with UPDATE_INTERVAL in ProxyableMachineRecipeType, right?

@Swedz
Copy link
Contributor Author

Swedz commented Sep 19, 2024

As for passing the recipe manager and registry access, I'm a little conflicted on it. Passing the level is very useful for me in Extended Industrialization, since it lets me access the PotionBrewing instance for the level and generate recipes using it.

@Technici4n
Copy link
Contributor

I found it, the stuff with UPDATE_INTERVAL in ProxyableMachineRecipeType, right?

Yes. Maybe just making a method without caching works.

Passing the level is very useful for me in Extended Industrialization, since it lets me access the PotionBrewing instance for the level and generate recipes using it.

True.

@Swedz
Copy link
Contributor Author

Swedz commented Sep 20, 2024

Looking for what event(s) to use for invalidating the cache here. I see OnDatapackSyncEvent and AddReloadListenerEvent which seems to be server side only, and RecipesUpdatedEvent which seems to be client side only. Why do you say that this invalidation should only be done server side? Do the recipes generated by fillRecipeList get sent to the client? Or does the client also need to build these themselves?

@Swedz
Copy link
Contributor Author

Swedz commented Oct 2, 2024

Could be moved to #900

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.

Clean up buildRecipes in MachineCategory
2 participants