Skip to content

Railcraft compat#384

Open
LonelyxiyaOVO wants to merge 8 commits intoCleanroomMC:masterfrom
LonelyxiyaOVO:Railcraft-compat
Open

Railcraft compat#384
LonelyxiyaOVO wants to merge 8 commits intoCleanroomMC:masterfrom
LonelyxiyaOVO:Railcraft-compat

Conversation

@LonelyxiyaOVO
Copy link
Copy Markdown

bro Railcraft compat

Copy link
Copy Markdown
Contributor

@Wizzerinus Wizzerinus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a maintainer of this mod, just mentioning some things I noticed ;)

}

@MethodDescription(example = @Example("item('railcraft:ingot:1')"))
public void removeByOutput(ItemStack output) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

afaik removeBy... methods usually should return a boolean

return;
}
if (!getRecipes().removeIf(recipe -> {
if (recipe.getInput().test(input.getMatchingStacks()[0])) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea to either adjust the function to accept an ItemStack, or test against each matching stack

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing lang files for all compat scripts. they're needed to generate the wiki (you can test this in a dev env by using /gs generateWiki)

public @Nullable IBlastFurnaceCrafter.IRecipe register() {
if (!validate()) return null;
ItemStack outputStack = output.get(0);
Ingredient inputIngredient = Railcraft.toIngredient(input.get(0));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure there's already a way in Groovyscript to make Forge ingredients

IBlastFurnaceCrafter.IRecipe recipe = new IBlastFurnaceCrafter.IRecipe() {
@Override
public net.minecraft.util.ResourceLocation getName() {
return new net.minecraft.util.ResourceLocation("groovyscript", "blastfurnace_" + System.currentTimeMillis());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

possibly allow configuring the recipe name similar to crafting table

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

definitely do this

public void validate(GroovyLog.Msg msg) {
validateItems(msg, 1, 1, 1, 1);
validateFluids(msg);
if (time < 0) time = 1800;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (time < 0) should emit a error instead

Comment on lines +128 to +129
if (time < 0) time = 1280;
if (slag < 0) slag = 1;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should emit errors on these

// Restore from backup
restoreFromBackup().forEach(entry -> {
try {
mods.railcraft.api.fuel.FluidFuelManager.addFuel(entry.fluid, entry.heatValue);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason you're not importing this?

Crafters.rollingMachine().getRecipes().clear();
}

public static class ShapedRecipeBuilder {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could use the AbstractShapedRecipeBuilder to not have to manage matrix/etc manually? (I'm not sure what's the name of it is, but it is used for MC tables, Astral Sorcery tables, etc)

Copy link
Copy Markdown
Collaborator

@WaitingIdly WaitingIdly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a few other things:

  1. instead of magic numbers as defaults, use the railcraft defaults directly - ie IRollingMachineCrafter.DEFAULT_PROCESS_TIME.
  2. run /gs generateWiki or enabling dev_generate_wiki in gradle.properties - check the log to ensure no warnings/missing lang keys.
  3. wizz also brought up great points, listen to those.

Comment thread gradle.properties
debug_prodigytech = false
debug_projecte = false
debug_pyrotech = false
debug_railcraft = false
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sort

Comment thread dependencies.gradle
'tinkers_construct-74072:2902483' : [debug_tinkers_construct],
'woot-244049:2712670' : [debug_woot],
'railcraft-51195:3853491' : [debug_railcraft],
'woot-244049:2712670' : [debug_woot],
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whitespace

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should only contain the generated script, which is obtained via ie /gs generateExamples or by enabling dev_generate_examples in gradle.properties.

Comment on lines +37 to +38
builder.time = time;
builder.slag = slag;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

call builder methods instead. this applies in all places, to all builders. observe how other similar methods do this

Comment on lines +102 to +105
@Property(comp = @Comp(gte = 0))
private int time = 1280;
@Property(comp = @Comp(gte = 0))
private int slag = 1;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add defaultValue = "whatever" to the annotation whenever a builder field is non-default


@Property(comp = @Comp(gte = 0))
private int time = 200;
private final List<IOutputEntry> outputs = new ArrayList<>();
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

annotate this with @Property. i would also suggest renaming this to output

Comment on lines +46 to +47
List<IRollingMachineCrafter.IRollingRecipe> recipes = new ArrayList<>(Crafters.rollingMachine().getRecipes());
recipes.removeIf(recipe -> recipe.getRegistryName().equals(r.getKey()));
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

create a new arraylist and then remove from that list? are you sure thats correct

List<IRollingMachineCrafter.IRollingRecipe> recipes = new ArrayList<>(Crafters.rollingMachine().getRecipes());
recipes.removeIf(recipe -> recipe.getRegistryName().equals(r.getKey()));
});
restoreFromBackup().forEach(r -> ModSupport.RAILCRAFT.get().rollingMachine.add(r.getKey(), r.getValue()));
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cant do this, because RollingMachine#add runs #addScripted

Comment on lines +33 to +46
if (time <= 0) {
GroovyLog.msg("Error adding Railcraft Blast Furnace recipe")
.error()
.add("time must be greater than 0, got: {}", time)
.post();
return null;
}
if (slag < 0) {
GroovyLog.msg("Error adding Railcraft Blast Furnace recipe")
.error()
.add("slag must be non-negative, got: {}", slag)
.post();
return null;
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

validate in the builder

Comment on lines +141 to +148
if (time <= 0) {
msg.add("time must be greater than 0, got: {}", time);
time = IBlastFurnaceCrafter.SMELT_TIME;
}
if (slag < 0) {
msg.add("slag must be non-negative, got: {}", slag);
slag = 1;
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if there are any messages, the validation fails. just do

Suggested change
if (time <= 0) {
msg.add("time must be greater than 0, got: {}", time);
time = IBlastFurnaceCrafter.SMELT_TIME;
}
if (slag < 0) {
msg.add("slag must be non-negative, got: {}", slag);
slag = 1;
}
msg.add(time <= 0, "time must be greater than 0, got: {}", time);
msg.add(slag < 0, "slag must be non-negative, got: {}", slag);

Comment on lines +116 to +117
@Property
private FluidStack fluid;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of this, use fluidOutput from parent

@RegistryDescription
public class FluidFuels extends VirtualizedRegistry<FluidFuels.FuelEntry> {

private final Map<String, Integer> backupFuels = new HashMap<>();
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is now completely unused


public boolean remove(IRollingMachineCrafter.IRollingRecipe recipe) {
return Crafters.rollingMachine().getRecipes().removeIf(r -> {
if (r == recipe) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if r == recipe well you can just do addBackup(recipe) + Crafters.rollingMachine().getRecipes().remove(recipe) then

Comment thread buildscript.properties
# Version of your mod.
# This field can be left empty if you want your mod's version to be determined by the latest git tag instead.
modVersion = 1.4.3
modVersion = 1.4.4
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

undo changing the version

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants