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

Improve CraftGuide integration #1513

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
4 participants
@Uristqwerty
Contributor

Uristqwerty commented May 26, 2015

A bug had crept in that caused the CraftGuide to NPE once for each non-AE2 crafting recipe, extending CraftGuide recipe loading time by tens of seconds.

While doing that, I cleaned up the rest of the file by removing two unnessecary interface implementations, and added support for inscriber and grinder recipes.

I hope that I adequately imitated the code style of AE2.

@yueh

View changes

Show outdated Hide outdated src/main/java/appeng/integration/modules/CraftGuide.java
import net.minecraft.init.Blocks;
import net.minecraft.item.ItemStack;
import net.minecraft.item.crafting.CraftingManager;
import net.minecraft.item.crafting.IRecipe;

This comment has been minimized.

@yueh

yueh May 27, 2015

Member

The blank lines between the import groups are intentional.

@yueh

yueh May 27, 2015

Member

The blank lines between the import groups are intentional.

@yueh

View changes

Show outdated Hide outdated src/main/java/appeng/integration/modules/CraftGuide.java
new ChanceSlot( 59, 12, 16, 16, true).setRatio( 10000 ).setFormatString( " (%1$.2f%% chance)" ).drawOwnBackground().setSlotType( SlotType.OUTPUT_SLOT ),
new ChanceSlot( 59, 30, 16, 16, true).setRatio( 10000 ).setFormatString( " (%1$.2f%% chance)" ).drawOwnBackground().setSlotType( SlotType.OUTPUT_SLOT ),
new ExtraSlot( 22, 12, 16, 16, handle ).clickable( true ).showName( true ).setSlotType( SlotType.MACHINE_SLOT ),
new ExtraSlot( 22, 30, 16, 16, grindstone ).clickable( true ).showName( true ).setSlotType( SlotType.MACHINE_SLOT ) };

This comment has been minimized.

@yueh

yueh May 27, 2015

Member

Could the be extracted to a constant similar to smallCraftingSlots? Or can it not share the same instance, which makes smallCraftingSlots also questionable? It is a slight performance and memory regression when creating an actual constant object for each recipe.

@yueh

yueh May 27, 2015

Member

Could the be extracted to a constant similar to smallCraftingSlots? Or can it not share the same instance, which makes smallCraftingSlots also questionable? It is a slight performance and memory regression when creating an actual constant object for each recipe.

@yueh

View changes

Show outdated Hide outdated src/main/java/appeng/integration/modules/CraftGuide.java
new ItemSlot( 21, 3, 16, 16 ).drawOwnBackground(),
new ItemSlot( 21, 39, 16, 16 ).drawOwnBackground(),
new ItemSlot( 50, 21, 16, 16, true ).drawOwnBackground().setSlotType( SlotType.OUTPUT_SLOT ),
new ExtraSlot( 31, 21, 16, 16, inscriber ).clickable( true ).showName( true ).setSlotType( SlotType.MACHINE_SLOT ) };

This comment has been minimized.

@yueh

yueh May 27, 2015

Member

Same as grinderSlots

@yueh

yueh May 27, 2015

Member

Same as grinderSlots

@yueh

View changes

Show outdated Hide outdated src/main/java/appeng/integration/modules/CraftGuide.java
this.addCraftingRecipes( craftingTemplate, smallTemplate, shapelessTemplate, generator );
final IAppEngApi api = AEApi.instance();
IBlocks aeBlocks = api.definitions().blocks();

This comment has been minimized.

@yueh

yueh May 27, 2015

Member

We usually mark variable/fields as final to prevent unintentional mutation of them.
As this is an ongoing project, not every class has already be changed.
It would be nice, if you could change it and save us a bit of work.

@yueh

yueh May 27, 2015

Member

We usually mark variable/fields as final to prevent unintentional mutation of them.
As this is an ongoing project, not every class has already be changed.
It would be nice, if you could change it and save us a bit of work.

This comment has been minimized.

@Uristqwerty

Uristqwerty May 28, 2015

Contributor

Do you mean just mark aeBlocks as final? Or every variable possible?

@Uristqwerty

Uristqwerty May 28, 2015

Contributor

Do you mean just mark aeBlocks as final? Or every variable possible?

@JackTheRedCreeper

This comment has been minimized.

Show comment
Hide comment
@JackTheRedCreeper

JackTheRedCreeper May 29, 2015

So this'll fix the huge load time? Finally! :D

So this'll fix the huge load time? Finally! :D

Adjusted CraftGuide integration based on pull request feedback
Changed grinderSlots and inscriberSlots to static fields, changed ExtraSlot
to ItemSlot in them, as they didn't benefit from additional features of
ExtraSlot, but it would otherwise require grinderSlots and inscriberSlots
to not be static.

Re-added blank line grouping imports (Eclipse must have automatically
removed it earlier). Also removed two now-unused imports.

Made aeBlocks final (I hope this was what was being referred to).
@thatsIch

This comment has been minimized.

Show comment
Hide comment
@thatsIch

thatsIch Jun 2, 2015

Member

@Uristqwerty

I send you a cleanup PR,
you probably require a rebase before you can pull it,
squash that,

then I can pull it.

Member

thatsIch commented Jun 2, 2015

@Uristqwerty

I send you a cleanup PR,
you probably require a rebase before you can pull it,
squash that,

then I can pull it.

@JackTheRedCreeper

This comment has been minimized.

Show comment
Hide comment
@JackTheRedCreeper

JackTheRedCreeper Jun 8, 2015

Hopefully this damn freeze gets fixed because half the AE2 recipes won't show anyways :l

Hopefully this damn freeze gets fixed because half the AE2 recipes won't show anyways :l

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