search

jaredlll08 / ModTweaker

ModTweaker is an addon for CraftTweaker, which provides Integration for an amount of mods.
MIT License
68 stars 63 forks source link

Ingredient support for Blood Magic orbs #689

Closed noobanidus closed 5 years ago

noobanidus commented 5 years ago

Currently, if you want to use a blood magic orb in a recipe and only use orbs of a specific tier or higher, you have to manually chain those orbs together in a pre-defined list using Ingredient.or. This provides an (converted to when no group is specified) bracket handler that returns an Ingredient for orbs "master" and above, according to the Blood Magic orb registry.

Probably not the best way to code it, possibly not even the correct, but it functions considerably less painfully than the alternative.

kindlich commented 5 years ago

I do not believe this is necessary, as adding another Bracket handler will only increase load time. Also, your code doesn't quite match the MoT standards.

The same result can easily be achieved by storing the orbs in a custom variable or static field within the scripts.

noobanidus commented 5 years ago

The bracket handler could easily be removed allowing simply for the function to access it. A function that interacts with the OrbRegistry would better support mods that create additional tiers of orbs, whereas storing the orbs in a custom variable would require you have a list of every orb available.

It's not impossible, it's just inconvenient: for default Blood Magic, you'd need to have variables chaining each orb together with the staying-in-grid function, and each of those recipes would cease to work in a situation where an additional orb was added by another mod.

Additionally, what part of the code isn't up to standard? As I'm relatively new to the codebase, I wasn't sure the best method to convert a list of ItemStacks into an Ingredient.

jaredlll08 commented 5 years ago

I'm not too fond of adding new bracket handlers, especially for something like this, while I personally think that this PR is not needed, since you can just store the orbs in a var, if you remove the bracket handler and leave the Orbs class (with cleanup, no need to have 2 methods, just put the _getOrb method inside the getOrb method) I'll merge it.

noobanidus commented 5 years ago

That's fine, I've removed the bracket handler and refactored the code. From my perspective, having this function allows for flawless handling of any additional orbs added by other mods, whereas otherwise you would need to hard-code them.

noobanidus commented 5 years ago

NBT handling turned out to be stranger than I expected. The "simpler" solution of specifying them all as a variable may actually be the only method that works.