Blood-Asp / GT5-Unofficial

Decompiled and modified version of GT5.07.07
160 stars 97 forks source link

Recipe loading from json assets #1583

Open MauveCloud opened 4 years ago

MauveCloud commented 4 years ago

Originally somewhat inspired by Ender IO's recipe system, this will: 1. Allow more flexible recipe inputs, with some allowing ore dictionary entries or even alternative options that don't share an ore dictionary entry.

  1. Allow recipes that would normally be added by GT_MachineRecipeLoader to be read from assets in the jar.
  2. Allow user-specified recipes to be read from the config folder, without needing GTTweaker.
  3. Allow recipes with more inputs or outputs than normal for a given machine - these may not show correctly in NEI, but they can still work if the machine is in a Processing Array.
  4. Allow other machines to have recipes read without requiring a new function for each (as long as adding the recipe isn't supposed to perform extra steps, like the assembly line for handling the research items, which isn't currently handled)
  5. Allow disabling or overriding an individual recipe via config recipes without forcing the new recipe to use a different duration. (at least for most recipes added by GT_MachineRecipeLoader; probably wouldn't work for recipes added in the "oreprocessing" classes) There may be other advantages I haven't thought of yet.

At Techlone's suggestion, these recipes are specified in JSON instead of XML. Defininite remaining tasks: [ ] Get some help with testing from other users (always better to have at least one person besides the developer do some testing) Look through the other methods in GT_MachineRecipeLoader and the "oreprocessing" classes for more recipes that can be transferred to json (recipes that run per material and just check for subtags are probably best left there, but there are some that check for specific oredicts) Uncertain remaining tasks (i.e. I'm uncertain these are worth the effort): Come up with a way to handle some of the more complicated recipe handling in GT_MachineRecipeLoader via json, such as recipes currently added with GT_ModHandler instead of GT_Values.RA, which may mean the recipes are added to machines of other mods as well if loaded.

[x] Revert machines etc. to original recipe system. [x] Change json reading code to return a list of GT_Recipe.

glowredman commented 4 years ago

Wow that is a lot of work! I hope this get some attention... Out of curiosity, why did you implement such a complex JSON reader? Wouldn't something like this worked the same?

MauveCloud commented 4 years ago

@glowredman

Out of curiosity, why did you implement such a complex JSON reader? Wouldn't something like this worked the same?

  1. I was not previously aware of that method. I don't remember now exactly how I came across the JsonReader class from Google gson, but the library was already used by vanilla Minecraft, and seemed to meet my needs well enough, so I hadn't thought to look for a simpler method.
  2. That method/class seems to be for serializing objects, more than allowing user-specified data, and it isn't clear to me from the javadoc how well it handles optional entries, and it presumably wouldn't handle alternative names for entries (admittedly not a critical feature), or convenience features like generating multiple recipes for different cutting/soldering fluids, and looking up items in the GT or IC2 item list classes.
draknyte1 commented 4 years ago

Don’t break backwards compat, EVER.

GT_Recipes.GT_Recipe_Map.X cannot be refactored to GT_Recipes.X. This will essentially break every addon ever made alone.

Please don’t EVER rename classes or change function descriptions/args or names.

MauveCloud commented 4 years ago

Harsh. I can refactor things back to hopefully deal with the backwards compatibility, after I take a break to try to calm down.

Are there other addons besides GT++ I need to worry about?

draknyte1 commented 4 years ago

Several of those mods are still in active development. https://github.com/Blood-Asp/GT5-Unofficial/blob/77a3c956e510b7c8ef4654054bcb16a0fccb86f7/src/main/java/gregtech/api/GregTech_API.java#L71

Backwards compatibility will always take precedence on PRs, being so late into 1.7.10s lifespan. We cannot assume any addon will be updated past the next release, so it’s on us to make sure we don’t break things. (That’s just good programming ethics anyway, it’s not harsh. Blood was responsible for breaking the API several times between 5.07.07 and 5.09.15~, it’s a real fucking pain in the ass to support addFurnaceRecipe with three different functions depending on version. 🤷🏻‍♀️)

MauveCloud commented 4 years ago

That’s just good programming ethics anyway

😕 That's a good point, and shame on me for not considering that better before I started. Fortunately, I was careful to leave the original GT_Recipe.java file alone to be on the safe side, so I think I can salvage the JSON reading code to return a list of that instead (and revert the machine classes I had changed, so they go back to using it instead of the new recipe classes). Unfortunately, I think doing so will prevent addressing the complaint from @leagris about "so many NEI pages displaying recipes of same output but many different item alternatives."

draknyte1 commented 4 years ago

From what little code I can actually review, most of it seems rather straight forward to adjust.

As for Leagris’ issue, I don’t think that’s quite relevant for this PR though at all? Easier to do 20 small PRs than one big convoluted pile that’s impossible to test. Plus keeps things more readable, cause 100+ files changed for this seems a lil absurd. Totally can’t review this on mobile :p

leagris commented 4 years ago

I regret that you have to abandon ore dictionary and alternative stacks ingredients in recipes because of backward compatibility, really. I had hope the old recipe interfaces would persist, but new interfaces would support lists of ingredients definitions.

MauveCloud commented 4 years ago

@leagris I haven't so much abandoned it as deferred it to a future PR. I have already thought of a way I could handle that which shouldn't break backward compatibility. There is already a class "GT_Recipe_WithAlt" (currenly only used for assembly line recipes), so I could probably improve it (maybe in a subclass) to also check inputs against those alts.

MauveCloud commented 4 years ago

Okay, I've finished my own testing, though there's still a chance I missed something or introduced a new issue while fixing what I found from testing, so I'd still prefer if somebody else could check it as well (though I realize it's a big PR and could take some time).

I actually discovered some minor issues with the current code while testing:

  1. Ferrite mixture in mixer didn't work before (recipe collides with invar dust). Handled by changing mixer recipe to 3 invar, 1 zinc, 2 iron (total amount of each element comes out the same).
  2. Fluid extractor recipe for Forestry "phosphor" item was trying to use "Phosphorus" (probably overzealous search and replace from bug about "Nettlesome phopshor name"). Corrected in json recipe.
  3. Some recipes referred to items from other mods by outdated internal names (sometimes indirectly by setting the ItemList enum entries to use said names). Examples: Railcraft Steel Anvil, Railcraft Strengthened Glass, Thermal Expansion Hardened Glass. Additional entries have been added to the json files so that whichever internal name is valid, the recipe will work (ofc if the mod is not installed, neither will resolve, and the recipe will be ignored for having all-null outputs)
  4. Distillery recipes for light/medium/heavy oil to just "oil" with circuit configuration 4 collide with universal distillation recipes that distill these to refinery gas. I'm guessing these are legacy recipes from before the big changes to oil processing.
  5. Fermenter recipe for rum to vinegar collides with hidden recipe for rum to pirate brew.
  6. Fluids "potion.slowness.strong", "potion.weakness.strong" and "potion.damage.long" don't exist, at least in vanilla MC 1.7.10. Recipes left in json files in case there is a mod that adds them.
  7. There is a hammer recipe for Endium ore (from Hardcore Ender Expansion) to crushed endium (as well as a pulverizer recipe), but the "type" flags for Endium in the Materials enum forbid it from having a "crushed" variant. Should generate plates/rods etc., but doesn't - possibly because GT_Proxy is waiting for "oreHeeEndrium" event instead of "oreHeeEndium". Left alone for now. The AOBD mod adds crushed Endium, but oredicted as "crushedHeeEndium", plus GT already adds so many crushed ores and dusts, one wouldn't expect to need that mod.
  8. There is a blast furnace recipe for "gemIlmenite" to wrought iron nuggets and tiny rutile dust, but there is no "gemIlmenite".
  9. Meteoric Steel doesn't show up as a material even with Galacticraft installed, so the Blast Furnace recipe from Meteoric Iron only produces dark ashes.
  10. LCR recipe for distilled water produces 1000L instead of the 3000L produced by the small chemical reactor from the same amounts of hydrogen and oxygen (line 1220 of GT_MachineRecipeLoader, compare to line 1218), and GT++ adds a colliding recipe for "hydroxide" fluid (1 oxygen cell and 1 hydrogen cell in the small chemical reactor produces 2000L hydroxide fluid and 2 empty cells, in the LCR it's just 1000L oxygen and 1000L hydrogen with no programmed circuit)
  11. Cutter recipes for sandstone and stone brick slabs from respective blocks require metadata 0 (for the full block), but could probably use the wildcard instead. Left alone for now.
  12. There is a cutter recipe for glowstone block to 4 glowstone plates, but glowstone plates don't seem to exist.
  13. A pyrolyse oven recipe for ic2biomass with circuit config 2 to fermented biomass, which I had not transferred to json files, is missing from NEI (and this also applies to an instance without the json changes). GT_RecipeAdder.addPyrolyseRecipe returns false if the input item is null.
  14. The multiblock-only recipe for Circuit 24, sulfur dioxide, oxygen, and water to sulfuric acid (previously line 3054, only active when "MoreComplicatedChemicalRecipes" is true) collides with the shared recipe for sulfur dioxide and oxygen to sulfur trioxide (with an empty cell in the small machine, no circuit) from line 3044.
  15. Two recipes for making Chlorobenzene (with "MoreComplicatedChemicalRecipes" true) collide with recipes for Dichlorobenzene, since they use the same input types including circuit config. See lines 3093-3094 and compare to 3083-3084.
MauveCloud commented 3 years ago

I haven't seen any indication of anyone else testing this, and there's more that can be done in future PRs to expand on this, but my part is basically done, so I've removed the "WIP" prefix from the title of the PR.