Advanced-Rocketry / AdvancedRocketry

Space mod for minecraft
http://arwiki.dmodoomsirius.me/
MIT License
215 stars 276 forks source link

Fix Chemical Reactor NPE and other recipe bugs, version number issues #2316

Closed Brycey92 closed 1 year ago

slava110 commented 2 years ago

Lol, first time reviewing stuff so don't judge me c: I thought it works per commit so I've clicked on each commit. And yeah

ReiDaTecnologia commented 2 years ago

IMO this fix is kinda dirty. Reloading recipes when server is starting. Other machines don't do stuff like that but this one is special. Also how would it work with ARTweaker which only executes scripts on loading?

i'm starting to think you didn't saw Brycey's diary on discord about this lol

slava110 commented 2 years ago

IMO this fix is kinda dirty. Reloading recipes when server is starting. Other machines don't do stuff like that but this one is special. Also how would it work with ARTweaker which only executes scripts on loading?

i'm starting to think you didn't saw Brycey's diary on discord about this lol

Depends on what you're talking about. He asked if it's possible to add CraftTweaker recipes after world load but they should be registered on MC loading. This fix doesn't apply to this special recipe only. It will remove all CraftTweaker recipes on world load replacing them with XML recipes. Also XML recipes are reloaded and this behavior is different from other machines for no reason from player perspective. And that's the problem

Brycey92 commented 2 years ago

I can find a way to selectively clear and reload these armor recipes, but I haven't found a way to generate the recipes with enchantments before the enchantment IDs are generated by world-load.

Brycey92 commented 2 years ago

IMO this fix is kinda dirty. Reloading recipes when server is starting. Other machines don't do stuff like that but this one is special. Also how would it work with ARTweaker which only executes scripts on loading?

i'm starting to think you didn't saw Brycey's diary on discord about this lol

Depends on what you're talking about. He asked if it's possible to add CraftTweaker recipes after world load but they should be registered on MC loading. This fix doesn't apply to this special recipe only. It will remove all CraftTweaker recipes on world load replacing them with XML recipes. Also XML recipes are reloaded and this behavior is different from other machines for no reason from player perspective. And that's the problem

I now clear and reload JUST the special armor recipes instead of all of the ones for the machine. Still a dirty fix, but better until we figure out a final solution.

ReiDaTecnologia commented 2 years ago

Btw, what happens if the mod author decide to change or remove the recipe? will it cause any issue with this?

Brycey92 commented 2 years ago

Btw, what happens if the mod author decide to change or remove the recipe? will it cause any issue with this?

If someone else changes or removes an armor airtight seal recipe in another mod or in Crafttweaker, my changes will negate that.

I could potentially redo it so that external changes are kept.

Brycey92 commented 2 years ago

Btw, what happens if the mod author decide to change or remove the recipe? will it cause any issue with this?

If someone else changes or removes an armor airtight seal recipe in another mod or in Crafttweaker, my changes will negate that.

I could potentially redo it so that external changes are kept.

I re-implemented it 5 more times and finally got it to keep removals of special recipes, but making it keep changes would require a LOT of extra code or a real fix at the source of this problem.

Recipes removed and re-created in CT and other mods will be kept, so that's a way to "change" the recipes.

It's notable that any time the command is run to reload recipes, all AR recipes will be reset and reloaded from generation and XML. Changes, removals, and additions by other mods will be wiped out until next game load. This was the existing behavior before I began making changes to the Chemical Reactor, and I haven't touched it.

Brycey92 commented 2 years ago

Looks like the reloadRecipes AR command can't work in 1.12.2 the way it's designed: java.lang.IllegalStateException: The object net.minecraftforge.oredict.ShapedOreRecipe@1318ba97 (name advancedrocketry:coilgold) is being added too late.