-
Notifications
You must be signed in to change notification settings - Fork 32
Fix and upgrade Pyrotech integration #333
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
base: master
Are you sure you want to change the base?
Fix and upgrade Pyrotech integration #333
Conversation
Some machines can't have inputs with stack sizes larger than one.
- Update examples to show inheritance. - Add inheritance. Inherited recipe will be added to the machines more advanced than the machine. - Add support for missing machines: sawmills, crucibles and Mechanical Compacting Bin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to run /gs generateWiki
(or enable it in gradle.properties
) and then fill in the missing lang keys.
} | ||
|
||
private static <T> T register(Class<? extends ModuleBase> moduleClass, Supplier<T> supplier) { | ||
if (ModPyrotech.INSTANCE.isModuleEnabled(moduleClass)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can replace with
@Override public boolean isEnabled() { return ModPyrotech.INSTANCE.isModuleEnabled(moduleClass) }
in the relevant classes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That doesn't work because when ForgeRegistryWrappers initialize, they need to get ForgeRegistry that's not null. Which is the reason why it crashes in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, i see. do you think making a changer higher up and modifying the ForgeRegistryWrapper with something like this.registry = isEnabled() ? Objects.requireNonNull(registry) : null
would be reasonable?
src/main/java/com/cleanroommc/groovyscript/compat/mods/pyrotech/StoneCrucible.java
Show resolved
Hide resolved
if (inherit) { | ||
String name = null; | ||
if (tier.ordinal() < 2) { | ||
name = tier.name().toLowerCase(Locale.US) + "_anvil"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think Locale.ROOT
or Locale.ENGLISH
? is this actually needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Locale.ROOT can lead to Turkish which will lowercase I to ı
src/main/java/com/cleanroommc/groovyscript/compat/mods/pyrotech/Anvil.java
Show resolved
Hide resolved
@Override | ||
public void validate(GroovyLog.Msg msg) { | ||
validateItems(msg, 1, 2, 1, 1); | ||
msg.add(duration < 0, "duration must be a non negative integer, yet it was {}", duration); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this allows a duration of 0
, which is annotated @Property(comp = @Comp(gte = 1))
. one of these should be adjusted
public @Nullable MechanicalCompactingBinRecipe register() { | ||
if (!validate()) return null; | ||
MechanicalCompactingBinRecipe recipe = new MechanicalCompactingBinRecipe(output.get(0), input.get(0).toMcIngredient(), input.get(0).getAmount(), hits.isEmpty() ? ModuleTechBasicConfig.COMPACTING_BIN.TOOL_USES_REQUIRED_PER_HARVEST_LEVEL : hits.toIntArray()).setRegistryName(super.name); | ||
input.get(0).setAmount(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also need to apply spotless (its task 6)
gradle.properties
Outdated
debug_prodigytech = false | ||
debug_projecte = false | ||
debug_pyrotech = false | ||
debug_pyrotech = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reset to false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would prefer having the init be inline. maybe also sort the lines?
@Property | ||
private AnvilRecipe.EnumType type; | ||
|
||
@Property | ||
private AnvilRecipe.EnumTier tier; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should have comp = @Comp(not = "null")
for both of these, since thats how its validated. this is likely also true for any other enums in this compat
msg.add(hits <= 0, "duration must be a non negative integer that is larger than 0, yet it was {}", hits); | ||
msg.add(type == null, "type cannot be null."); | ||
msg.add(tier == null, "tier cannot be null."); | ||
msg.add(super.name == null, "name cannot be null."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all msg.add(super.name == null, "name cannot be null.");
should be replaced with validateName();
. should also be overriding getRecipeNamePrefix
return new RecipeBuilder(); | ||
} | ||
|
||
public BloomeryRecipe add(String name, ItemStack output, IIngredient input) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
several methods in this class, including this method, are not annotated - they should be annotated
|
||
@Override | ||
public void validate(GroovyLog.Msg msg) { | ||
validateItems(msg, 1, 2, 1, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the second ingredient is set, this also needs to validate that its in ModuleTechMachineConfig.STONE_SAWMILL.SAWMILL_BLADES
, otherwise it will not be able to be inserted into that slot
removeByOutput(output, true); | ||
} | ||
|
||
public void removeByOutput(IIngredient output, boolean log) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
annotate @GroovyBlacklist
, and the other methods you dont want being callable from groovyscript
@Property(property = "recipeFunction") | ||
@Property(property = "recipeAction") | ||
@Property(property = "name") | ||
public static class Shapeless extends AbstractCraftingRecipeBuilder.AbstractShapeless<WorktableRecipe> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also needs @Property(property = "replace")
msg.add(rec != null && rec.getRecipe().canFit(1000, 1000), () -> "a recipe with that name already exists! Either replace or remove the recipe first"); | ||
if (msg.postIfNotEmpty()) return null; | ||
ShapelessCraftingRecipe recipe = new ShapelessCraftingRecipe(output.copy(), ingredients, recipeFunction, recipeAction); | ||
rec = new WorktableRecipe(recipe, tool == IIngredient.EMPTY ? null : tool.toMcIngredient(), damage, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe add stages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't want to get into another rabbit hole so I double it and give it to myself in future.
@Property(property = "recipeFunction") | ||
@Property(property = "recipeAction") | ||
@Property(property = "name") | ||
public static class Shaped extends AbstractCraftingRecipeBuilder.AbstractShaped<WorktableRecipe> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also needs @Property(property = "replace")
if (slag == null) { | ||
slag = ItemStack.EMPTY; | ||
} | ||
int minOutput = !bloom.isEmpty() ? 0 : 1; | ||
if (slag == null) { | ||
slag = bloom.isEmpty() ? new ItemStack(ModuleTechBloomery.Items.SLAG, 4) : ItemStack.EMPTY; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have an if (slag == null) slag = ItemStack.EMPTY
here, and then check if (slag == null)
again just a few lines down. this selection also has an inverted ternary - !x ? a : b
should be flipped to x ? b : a
com.codetaylor.mc.athenaeum.util.IngredientHelper.fromStackWithNBT(recipe.getOutputBloom()), | ||
anvilHit, | ||
anvilType, | ||
Arrays.copyOf(recipe.getAnvilTiers(), recipe.getAnvilTiers().length), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dont need to copy the array
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think some of the add(x/y/z)
methods are so complex as to not be worthwhile, and should be removed and the recipe requires the recipeBuilder
directly.
like,
add(String, IIngredient, ItemStack, int, int)
add(String, IIngredient, ItemStack, int, int, boolean)
add(String, IIngredient, IIngredient, ItemStack, int, int)
add(String, IIngredient, IIngredient, ItemStack, int, int, boolean)
this is too complex.
theres also several comments (ie in PitKiln) that have not been modified or responded to.
groovyscript.wiki.pyrotech.bloomery.addBloom0.inherit=Adds recipes in the format `name`, `bloomOutput`, `input`, `inherit` | ||
groovyscript.wiki.pyrotech.bloomery.addBloom0=Adds recipes in the format `name`, `bloomOutput`, `input` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should explain what "addBloom" actually does - it just says "adds recipes" but whats the difference between that and just "add"?
return anvilType(AnvilRecipe.EnumType.HAMMER); | ||
} | ||
|
||
@RecipeBuilderMethodDescription |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs (field = "anvilType")
@Property(comp = @Comp(gt = 0)) | ||
private int anvilHit = ModuleTechBloomeryConfig.BLOOM.HAMMER_HITS_IN_ANVIL_REQUIRED; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
er, set defaultValue
to indicate it pulls from a config?
@RecipeBuilderMethodDescription | ||
public RecipeBuilder anvilTier(AnvilRecipe.EnumTier tier) { | ||
anvilTiers.add(tier); | ||
return this; | ||
} | ||
|
||
@RecipeBuilderMethodDescription(field = "anvilTier") | ||
public RecipeBuilder tierGranite() { | ||
return anvilTier(AnvilRecipe.EnumTier.GRANITE); | ||
} | ||
|
||
@RecipeBuilderMethodDescription(field = "anvilTier") | ||
public RecipeBuilder tierIronclad() { | ||
return anvilTier(AnvilRecipe.EnumTier.IRONCLAD); | ||
} | ||
|
||
@RecipeBuilderMethodDescription(field = "anvilTier") | ||
public RecipeBuilder tierObsidian() { | ||
return anvilTier(AnvilRecipe.EnumTier.OBSIDIAN); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all of these point to anvilTier
, which doesnt exist - needs to be anvilTiers
groovyscript.wiki.pyrotech.brick_sawmill.input.value=Sets the input required for the recipe. Second input value is an optional value for setting saw blade. | ||
groovyscript.wiki.pyrotech.brick_sawmill.duration.value=Sets the time required for the recipe to complete | ||
groovyscript.wiki.pyrotech.brick_sawmill.wood_chips.value=Sets the amount of wood chip to drop. | ||
groovyscript.wiki.pyrotech.brick_sawmill.add=Adds recipes in the format `name`, `input`, `(optional) blade`, `output`, `duration`, `woodChips` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(optional) blade
, split into two different lang keys
groovyscript.wiki.pyrotech.soaking_pot.description=Converts an item and liquid into a new item. Can require a campfire below | ||
groovyscript.wiki.pyrotech.soaking_pot.time.value=Sets the time required for the recipe to complete | ||
groovyscript.wiki.pyrotech.soaking_pot.campfireRequired.value=Sets if a campfire is required underneath | ||
groovyscript.wiki.pyrotech.soaking_pot.add=Adds recipes in the format `name`, `input`, `output`, `time` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
recipe takes 5 inputs, this names 4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are two missing lang keys, groovyscript.wiki.pyrotech.wither_forge.note0
and wood_chips
. wood_chips
needs to be changed to groovyscript.wiki.pyrotech.stone_sawmill.wood_chips.value
or similar (since both stone + brick sawmills use the same key, perhaps groovyscript.wiki.pyrotech.sawmill.wood_chips.value
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same issues that apply to the Bloomery also apply to the Wither Forge.
changes in this PR: