SkriptLang
Fix event-itemstack for ComplexRecipe
Description
With CraftEvent, the item stack is obtained through looking at the recipe's result. However, in the majority of cases, when the recipe is a ComplexRecipe, this result is air. This PR adds a check to instead use the currentItem method in such cases, which solves the issue without altering the behaviour with 'normal' crafts.
This would potentially be an issue, as scripts relying on this bug would stop working. However, it would eliminate some confusion, and allow to tell the items in question apart (as far as I know, there was no way to previously do so in such cases).
Target Minecraft Versions: any Requirements: none Related Issues: #5762
In my opinion, this fix is unreliable, and not what the user would want. Though, there is an existing recipe PR, which might include this
Though, there is an existing recipe PR, which might include this
It does not
In my opinion, this fix is unreliable, and not what the user would want. Though, there is an existing recipe PR, which might include this
Sadly due to restrictions there really isn't any other fix, the only thing I could think of is implementing it as a future event-value to retain existing behavior, but since the method name is current it doesn't make sense for it to be future
Right, I've run the test tasks and wrote a simple script that shows what items are crafted, but I'll try to see if some more advanced scripts (updated to work with the patch) still hold up.
In my opinion, this fix is unreliable, and not what the user would want. Sadly due to restrictions there really isn't any other fix.
I would argue that the users probably would want the item to make sense, and not simply be a bogus "air" for some recipes.
Ideally a new separate event could be created to distinguish regular crafts from "special" ones, but due to the way events are managed, I can't find an easy way to do so.
Another solution could be exposing the Recipe to Skript for CraftItemEvent or some other way for the script to figure out what kind of craft it is processing.
Although, perhaps exposing the recipes for just this event isn't particulatily interesting as is. But it could also potentially be useful for PrepareItemCraftEvent or whenever custom recipes get implemented (#7150).
Another solution could be exposing the
Recipeto Skript forCraftItemEventor some other way for the script to figure out what kind of craft it is processing.
recipes will be exposed when the recipe pr is merged, a custom event would be discouraged by me since it's an extra step for no benefit. Only concern I have is possible breaking behavior in skripts, but the only two solutions would be
a) register a future event-value, which doesn't make sense as it's present
b) register a new expression along the lines of the method name but it's overly broad current item and current crafted item aren't great
So I'm fine with just adding it on top of the existing event-value
the only thing I could think of is implementing it as a future event-value to retain existing behavior
a) register a future event-value, which doesn't make sense as it's present b) register a new expression along the lines of the method name but it's overly broad current item and current crafted item aren't great
Oh, my bad I am not super familiar with Skript, I wasn't aware of the time states system.
These two fixes would be the least disruptive ones. The new item syntax makes sense, but I agree that future item is a bit awkward, and to me, the crafted item syntax doesn't look too off. And both of them could have the benefit of referencing directly the crafted item stack itself rather than the supposed virtual result (although I haven't tested that yet, so still to be confirmed).
If what I just said turns out true, and we do go for completely removing the bug, wouldn't it make more sense to broaden the patch for all crafting recipes, after making sure, of course, that everything works fine that way too?
https://github.com/SkriptLang/Skript/blob/8f8561931fbb76ca1d138fead874048ab149d563/src/main/java/ch/njol/skript/classes/data/BukkitEventValues.java#L389C3-L393C6
the event-item will also need to be updated.
Sure! I don't have much time to work on it this week but I think I'll be able to look into @sovdeeth's comment this week-end. Sorry by the way for seeing it this late.
My bad, I misclicked. I'm confused as to what @sovdeeth means in their comment, isn't event-item already updated? Or are you talking about the entity? Also, I'm slightly confused as to the what the requested changes are. Is it the changes I made and you requested to merge, or changes you want me to do? I can't figure out where to see them. I've never had any real contribution experience, so sorry for my lack of understanding.
See the link I sent, there's an event-item for InventoryClickEvent that needs updating
Wow, my bad. For some reason I thought I was looking at some completely different part of the code, now I see what you mean. I've tested with just removing the check, and it seems to work fine. But I'm having concerns regarding why the check was there in the first place. Is there really supposed to be a difference between the recipe's result and the obtained item? That also brings me to think that maybe it should just look at the new item everywhere, regardless of wherer it's a complexRecipe or not.
Thanks for your first pull request, @ChickChicky! We appreciate your contribution 🎉 If you have any ideas for improvements or find any bugs, don’t hesitate to open another PR. Looking forward to more contributions from you 😄