Skip to content
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

added lightningcraft compat #283

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Wizzerinus
Copy link
Contributor

literally no one asked for this but here we go

oh wait Lord_Chaos did

Copy link
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.

there is also lightning transforming recipes, according to MoreTweaker, although those might deserve a similar treatment to Pneu explosion (creation of native functionality + suggestion to use that instead of custom compat)

@Wizzerinus
Copy link
Contributor Author

Wizzerinus commented Dec 22, 2024

I've heard the LC's own lightning transform is really buggy (see: DJ2 where you can only craft electricium in a specific order), which is why I haven't implemented it. Do not mind trying to implement lightning transform properly under in-world crafting.

@brachy84 brachy84 added the mod compat Relating to compatability with a mod or features of a mod label Dec 26, 2024
@Wizzerinus
Copy link
Contributor Author

updated

public @Nullable LightningInfusionRecipe register() {
if (!validate()) return null;

ItemStack centerItem = this.centerItem.getMatchingStacks()[0];
Copy link
Collaborator

Choose a reason for hiding this comment

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

should loop for each of getMatchingStacks() instead of only taking the first.

if (!validate()) return null;

ItemStack centerItem = this.centerItem.getMatchingStacks()[0];
ItemStack[] inputs = input.stream().map(i -> i.getMatchingStacks()[0]).toArray(ItemStack[]::new);
Copy link
Collaborator

Choose a reason for hiding this comment

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

while it is not currently on main and is still just in the Betweenlands PR, IngredientHelper.cartesianProductItemStacks would be very useful here to, once again, use all values of getMatchingStacks() instead of just the first.

Comment on lines +48 to +49
@Property(defaultValue = "determined automatically based on the input items")
private boolean nbtSensitive = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there a reason why having this be directly configurable by the player is required? if it can be automatically determined, why not simply not expose it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because there are cases where it's automatically configured value is wrong. For example, if you want to allow only input item that has no NBT in it (say, it's some tool that stores durability in NBT). You need to enable nbtSensitive = true, and there's not really another way to do it

@Wizzerinus
Copy link
Contributor Author

going to wait until betweenlands is merged for the cartesian product thing because I do not feel like reinventing the wheel and also because I feel like I'll have to rewrite to it anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mod compat Relating to compatability with a mod or features of a mod
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants