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

Make event values return item stacks instead of item types if possible #4128

Merged
merged 4 commits into from
Jul 15, 2022

Conversation

TPGamesNL
Copy link
Member

Description

Makes some event values return ItemStacks instead of ItemTypes, since that is more direct.
The issue in #4109 was caused by the event-value getter first converting the ItemStack to an ItemType, and after that it got converted back to an ItemStack, where the reference got lost, as the converter called ItemType#getRandom, which clones the ItemStack.


Target Minecraft Versions: any
Requirements: none
Related Issues: #4109

@TPGamesNL TPGamesNL added the bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. label Jun 28, 2021
@TheLimeGlass
Copy link
Collaborator

TheLimeGlass commented Jun 29, 2021

Haven't we done this in the past and it caused more issues then we could count? I have a feeling that Skript will try to convert and cause the majority of the issues.

@TPGamesNL
Copy link
Member Author

@Wealthyturtle would you be able to weigh in on this (because of #2510)?

@TPGamesNL TPGamesNL marked this pull request as draft June 30, 2021 11:40
@Wealthyturtle
Copy link
Member

@Wealthyturtle would you be able to weigh in on this (because of #2510)?

I think this change is fine since it fixes conversion between ItemStack and ItemType (so long as it doesn't break other stuff)

@TPGamesNL
Copy link
Member Author

I think this change is fine since it fixes conversion between ItemStack and ItemType (so long as it doesn't break other stuff)

Well this PR itself doesn't fix any conversion, but there is a ItemStack -> ItemType converter which should do this:

Converters.registerConverter(ItemType.class, ItemStack.class, new Converter<ItemType, ItemStack>() {
@Override
@Nullable
public ItemStack convert(final ItemType i) {
return i.getRandom();
}
});
Converters.registerConverter(ItemStack.class, ItemType.class, new Converter<ItemStack, ItemType>() {
@Override
public ItemType convert(final ItemStack i) {
return new ItemType(i);
}
});

But I'm not sure if the converters are always checked, so it could still give issues. If this gets tested properly, it should be fine.

@TPGamesNL TPGamesNL marked this pull request as ready for review July 2, 2021 13:02
Copy link
Member

@AyhamAl-Ali AyhamAl-Ali left a comment

Choose a reason for hiding this comment

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

This change sounds better for me. Hopefully it doesn't break anything unexpectedly
LGTM STICKER

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.

I think we are fine to trial this out for the 2.7 alphas

@TPGamesNL TPGamesNL merged commit 215939f into SkriptLang:master Jul 15, 2022
@TPGamesNL TPGamesNL deleted the fix/event-values-itemstacks branch July 15, 2022 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue that needs to be fixed. Alternatively, a PR fixing an issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants