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

🚀 Add crafting inventory slots expression (result/matrix) #4410

Closed

Conversation

AyhamAl-Ali
Copy link
Member

@AyhamAl-Ali AyhamAl-Ali commented Nov 5, 2021

Description

🚀 This PR aims to add a new expression for crafting inventory slots such as the result item and matrix
Some utility methods have been added, if anyone have better way to achieve their results let me know.

NOTE: While testing I have found a potential bug when you try to change the matrix in preparing craft event that will trigger the event again and get into infinite loop, I will try to report that to spigot later when I can but for now I have disabled that behavior in Skript and sent a warning when someone try to use it.

NOTE: This PR should be merged after #4409 to be able to use event-inventory


Target Minecraft Versions: Any
Requirements: None
Related Issues: #4908

@TPGamesNL TPGamesNL changed the title 🚀 Add carfting inventory slots expression (result/matrix) 🚀 Add crafting inventory slots expression (result/matrix) Nov 5, 2021
@TPGamesNL TPGamesNL added the feature Pull request adding a new feature. label Nov 5, 2021
@TheLimeGlass
Copy link
Collaborator

TheLimeGlass commented Nov 22, 2021

I believe we should be using ItemStack for everything. ItemType is used as a literal. This way we can allow for named, lore, enchanted items etc.

This will also remove the need for the Utils toItemTypes method, which I personally don't believe we need to be adding.

@AyhamAl-Ali
Copy link
Member Author

I believe we should be using ItemStack for everything. ItemType is used as a literal. This way we can allow for named, lore, enchanted items etc.

This will also remove the need for the Utils toItemTypes method, which I personally don't believe we need to be adding.

This is an important note, I remember I saw a PR about replacing ItemStacks with ItemTypes but didn't follow up with why
This is the PR #2510 if itemtypes aren't the good choice we better replace them with ItemStacks probably

@TheLimeGlass
Copy link
Collaborator

TheLimeGlass commented Nov 22, 2021

It really needs to be explained; So basically ItemTypes are supposed to be used as a Literal. Syntaxes such as on right click with %itemtypes% makes sense, because on rightclick with a diamond sword can be used. An ItemStack will return all the details of the item, this can be used anywhere in Spigot API, it gives the user more information about the value.

Another example can be in SkQuery recipes, it only accepted ItemTypes, which restricted the user to not allow for named, enchanted or lored items. Where as Tuske later added the ability to use ItemStacks, so you could check if the item was named. This is good for like quests or missions where the item must be named. SkQuery recipes have since been removed.

TPGamesNL also mentioned that we should be doing this for primitive number types. Returning an Integer, or a Double rather than just returning Number. But when it comes to accepting a change, you can be using Number, and just number.intValue() or whatever on it, so the output is certain, and the input can be anything avoiding the cannot understand effect error.

Same goes with EntityData and Entity. EntityData is a literal, like on spawn of a cow or a skeleton

Copy link
Member

@APickledWalrus APickledWalrus left a comment

Choose a reason for hiding this comment

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

Nothing super major :D (mainly style suggestions)

I think this will be a useful feature

AyhamAl-Ali and others added 2 commits March 5, 2022 15:46
Co-authored-by: APickledWalrus <apickledwalrus@gmail.com>
public class ExprCraftingSlots extends SimpleExpression<ItemStack> {

static {
Skript.registerExpression(ExprCraftingSlots.class, ItemStack.class, ExpressionType.COMBINED,
Copy link
Member

Choose a reason for hiding this comment

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

Skript.registerExpression(ExprCraftingSlots.class, ItemStack.class, ExpressionType.COMBINED,
"[the] crafting [inventory] result (slot|item) [of %inventories%]",
"%inventories%'[s] crafting [inventory] result (slot|item)",
"[the] crafting [inventory] (matrix|grid) [(slots|items|shape)] [of %inventories%]",
Copy link
Member

Choose a reason for hiding this comment

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

Do you think it would be better to allow the user to access the individual crafting grid slots?
Maybe something like.
crafting slot %*number% [of %inventories%]

This may also work better with returning InventorySlots

AyhamAl-Ali and others added 4 commits August 7, 2022 00:03
Co-authored-by: APickledWalrus <apickledwalrus@gmail.com>
Co-authored-by: APickledWalrus <apickledwalrus@gmail.com>
Co-authored-by: APickledWalrus <apickledwalrus@gmail.com>
Co-authored-by: APickledWalrus <apickledwalrus@gmail.com>
@Moderocky Moderocky marked this pull request as draft April 8, 2023 11:55
@AyhamAl-Ali AyhamAl-Ali changed the base branch from master to dev/feature April 6, 2024 01:49
@sovdeeth
Copy link
Member

Do you still intend to work on this?

@sovdeeth
Copy link
Member

closing due to inactivity

@sovdeeth sovdeeth closed this Sep 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Pull request adding a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants