GregTechCE / GregTech

GregTech rewrite for modern versions of Minecraft
GNU Lesser General Public License v3.0
269 stars 150 forks source link

[Suggestion] Recipe System Rework #1215

Open eutro opened 4 years ago

eutro commented 4 years ago

Is your feature request related to a problem? Please describe. The Recipe class and the entire recipe system revolves around items, fluids and EU being the only requirements for recipes. Unfortunately, this is insufficient, even within GTCE, the EBF checks for coil temperature, which is not implemented very nicely, and is prone to bugs, such as #1157.

Describe the solution you'd like A rework or overhaul of the recipe system, to an extent would be ideal, where certain checks and actions, such as input dirtiness could be delegated to the recipe itself.

LAGIdiot commented 4 years ago

First of all thank you for bringing this up I really like this idea.

Status: Accepted

Reasons: This will open GTCE many possibilities for integration with other mods and even allow as to make machines which would not be possible before. Also this will give us edge against other mods and make us more irresistible to modpack authors.

Additional notes: Of course this will be compatibility nightmare and it has to be designed to break nothing of existing recipes and systems on our side. All changes to API has to be documented. And extended testing period should be allocated to this.

fmarceau commented 3 years ago

I really like the idea as well as I felt it while fixing other issues such as the inventory interaction with Forestry. The current system is very clumsy. However, this issue is more of an epic which should be broken into multiple simpler tasks in order for non-expert contributors, such as myself, to be allowed to chip in and tackle some of those. I will study how other popular, maybe more recent, mods built their recipe system.

idcppl commented 3 years ago

Recipe names would be a nice addition, the current method for finding recipes is so inefficient.

fmarceau commented 3 years ago

It's rather difficult to find a lot of those new general-purpose mods which would be leveraging the crafting table extensively. Most of them have another kind of crafting such as Celestial Altar for Astral or Emporerer for AA.

galyfray commented 3 years ago

a way of achieving this could be to change the way recipe think by making using them a list of predicate or test the test could take a MetaTileEntitie (or anything better just proposing) for being able to check if the right amount of fluid /item/energy is present but also easily add more specific behavior like checking for blast furnace temperature or mekanism gas and maybe even adding the ability to influence output with not consumed input with enchantment (for example allowing to correctly make an electric exnhilo sieve with enchantment of mesh taking in count)

does this way of thinking the problem looks correct for you @LAGIdiot ?

LAGIdiot commented 3 years ago

I did not give this matter enough though yet to came up with implementation guideline. But I can assume that right think would be to push most of work to recipes and give them access to machine working on that recipe itself. And let machine provide something like capabilities (for input/output/"special" slots, tier, heat, whatever possible) which recipe could check and use.

galyfray commented 3 years ago

Hi working on a draft of some new recipes system I made this prototype of the new architecture in a simpler environment. https://github.com/galyfray/recipesTest/tree/master/src/test machines (metatile entities and other) provide checkable (WIP name i'm not good at naming things) element the recipe contains checker that will ... check ? if everything is alright. Checker will be created by recipes builders allowing to not break everything related to recipes (only 99% of them I guess). this way of making recipes should be flexible enough for allowing recipes to take exotics element and ignore basic ones.

I'm posting this here to ask if this way seems good for you @LAGIdiot and if I can start working on implementing this or if it's all worse before spending to much time in. Any comment would and will be appreciated. If you have any idea of methods I should Implement for optimal backward compatibility please tell me I would like this new system to break as less things as possible.

LAGIdiot commented 3 years ago

@galyfray I will check it later as I am bit time constrained

Archengius commented 3 years ago

That's all cool and fancy, but the question is: do we really need it? I don't think system should be overhauled just to support 3 exotic recipes. Besides, I don't like the root idea of recipe being responsible for performing actions there. Recipes are simple data holders, and there are multiple reasons for it - that's why all handling happens on the recipe map and not recipes themselves.

Idea of forcing recipes to check inputs/outputs themselves is pretty questionable too, because machines usually know better how their I/O should be handled, and it allows having optimisations and custom behaviours without tieing them heavily to recipe system itself.

What I definitely agree with though, is the fact that RecipeMap#findRecipe could use more context. I would even go as far as making a context class and passing it to find recipe instead of continuing adding extra arguments. Potentially it would also allow extra extensibility by extending that class and adding extra data to context. That's how you could handle mekanism gases and so on.

galyfray commented 3 years ago

to answer you @Archengius yes we need it. The current recipe system is working but always admit that you will consume energy item and/or liquid but nothing less or more making recipes that need other condition like coil temperature or doesn't consume item or energy needs too much code. It's not like 3 recipes it's more like a new world for add-ons to easily add new recipes type for modpack makers to customize crafts on an other hands this will also offer more liberty for us to add content to the mod.

The idea is not to force recipes to check inputs we can move this anywhere we want the idea is to drastically change how recipes store what's needed for the recipe to complete.

machines usually know better how their I/O should be handled, and it allows having optimisations and custom behaviours without tieing them heavily to recipe system itself. I absolutely agree with you, the current version of the recipes sends its input to the RecipeMap#findRecipe method using something similar but more generic to allow thing like heat, gas or anything needed.

A context class could be a good idea depending on how it is stored as long as it is really easy to add new data to the context without having to edit the context class itself.

Archengius commented 3 years ago

Coil temperature check is 3 lines of code, other conditions can be added in a similar way. I don't see how new recipe system would make it much better than that. You say that it will open new possibilities for modpack/addon makers, but I have one simple question: why not make a separate machine and override CheckRecipe, like EBF currently does with temperature for example? Let's be real, like I said earlier, "normal" machines will have exactly 3 "special" recipes, no more than that. Usually it's a group of recipes that needs something special - so why not just move them to a separate machine? This way you don't need to change anything at all, and current systems works pretty well to customize behavior of recipes in custom machines. So do we really need a system for these 3 "special" recipes in normal machines? I don't think so.

eutro commented 3 years ago

I'd like to point out that the coil temperature check doesn't actually work properly which makes the system unfit for any other special recipes.

Archengius commented 3 years ago

It works properly, it just doesn't handle choosing new recipe over incorrect one. And in fact it's not a reason to change recipe system completely, what we actually need is just a unification of findRecipe parameters into the context object (like I suggested earlier) which could, for example, accept extra predicate which decides whenever recipe can be used before it is returned. This would allow recipe lookup to continue search if matching recipe is not accepted by the temperature requirement. Or, even better, we could just make a checkRecipe method in RecipeMap accepting recipe and context, which could be overriden by recipe map to allow extra checking in machine-independent manner, as long as the machine in context for example implements capability ICoilTemperatureProvider and returns temperature higher than the required minimum.

This said, control on whenever recipe is picked or not remains on RecipeMap and machine implementation, while recipes remain simple lightweight data holder objects handled by the system.

galyfray commented 3 years ago

It works properly, it just doesn't handle choosing new recipe over incorrect one.

That's what I could call working properly ... just my opinion

And in fact it's not a reason to change recipe system completely

If your looking for a great reason to change the recipe system then you won't find there is no great reason there is a plenty of small reason that could not require a full rewrite of the recipes system but as they are numerous rewriting for something better is a solution.

This said, control on whenever recipe is picked or not remains on RecipeMap and machine implementation, while recipes remain simple lightweight data holder objects handled by the system.

No one said that we want recipes for being anything else than a lightweight storage, No one said that the jobs of checking isn't the one of RecipeMap and machine implementation. We are trying to find a way of making recipe more efficient more generic the main problem is the current recipe system is not as generic as possible/needed

There is currently no urge to change this system but it would be nice

the context object (like I suggested earlier) which could, for example, accept extra predicate which decides whenever recipe can be used before it is returned.

yeah then why not simply replace item and liquid system with extra predicate so everything that could be checked for a recipe to run will be predicate ? If we only implement what you says we will have liquid energy item working in a way and other thing all working in the same way .. It will work yes but that not the best we can make to my mind and will still need a pretty huge rework of the recipe system ...