SoulboundItem#getByItem(ItemStack) always returns instance of SlimefunItem

mfnalex commented 4 years ago

When using SoulboundItem#getByItem(ItemStack), it always returns an instance of SlimefunItem instead of SoulboundItem.

It would be nice if the method would return a SoulboundItem instead. I need that for my AngelChest plugin to be able to detect SoulboundItems. Could that be added?

The following class should @Override the getByItem(ItemStack) method if possible: https://github.com/TheBusyBiscuit/Slimefun4/blob/master/src/main/java/io/github/thebusybiscuit/slimefun4/implementation/items/magical/SoulboundItem.java

WalshyDev commented 4 years ago

Firstly, you can just PR this you know. Secondly, you can also just instanceof and cast it.

TheBusyBiscuit commented 4 years ago

You can always do an instanceof check and cast it, adding it to each implementation of SlimefunItem wouldn't be practical in my opinion.

But more importantly, if you are trying to check if an ItemStack is soulbound, then this is the wrong class to look for, only some soulbound SlimefunItems extend SoulboundItem, there is an interface called Soulbound which is used to indicate this behaviour. And even then, using a Soulbound Rune, any ItemStack can be marked as "soulbound", so what you are really looking for is the following method: SlimefunUtils.isSoulbound(ItemStack) which covers both, predefined Soulbound Items and customly made ones.

I would also remind you to please not use our Bug Tracker for this, we have a discord server for these things. There are much more people around and they can usually answer much faster.

TheBusyBot commented 4 years ago

Your issue was closed, it may fall under one or more of the following categories. Please wait for an admin to tick off the points that apply.

Please respond below, if you have any questions. Do not open a new Issue unless explicitly told otherwise, comment below or edit your post instead.

[x] You did not follow our template. Please follow the Issue template to help us identify your issue more effectively.
[x] You did not provide any information about your versions (We absolutely need the exact version numbers that are installed on your Server, \"latest\" is not helpful)
[ ] You did not provide a proper description to the problem. Try to write at least 4-6 sentences on how to reproduce this.
[ ] We were unable to reproduce issue, if you think your issue still persists then please comment down below and give a better description on how to reproduce it.
[ ] Your issue was posted in a foreign language, we only accept english issues on here.
[ ] Your issue is not a bug, it is either intended or something we cannot change.
[x] Your issue is not a bug, please only use this issue tracker to report bugs. Any other kind of communication should happen on discord.
[ ] Your issue was already reported, it is a duplicate. Check the other issues first before posting!
[ ] You posted an error without using pastebin. Please always post errors via pastebin otherwise they become nearly unreadable.
[ ] You seem to be reporting multiple bugs at once. Please make a seperate issue for each bug you encountered, so we can properly handle them individually.
[ ] Your issue has already been fixed in a later version of Slimefun or CS-CoreLib, you should update.
[ ] You are using an outdated and unsupported version of Slimefun / CS-CoreLib, again, you should update.
[ ] You are using an unofficially modified build of Slimefun. We only support official versions of Slimefun - for obvious reasons.
[ ] You are using an unsupported version of Minecraft. We only provide support for the Minecraft versions Slimefun was developed for, older versions are not supported anymore.
[ ] You are using a \"stable\" version of Slimefun (prefixed with \"RC - \"), your issue may have been fixed in a development build, so we only accept bug reports from those.
[ ] You are on the wrong issue tracker. We would like to remind you that this Issue Tracker is only for Slimefun. To report bugs on any addons, head to the corresponding issue tracker of that addon.

Make sure to check out our article on How to report bugs for even more information.

mfnalex commented 4 years ago

Firstly, you can just PR this you know. Secondly, you can also just instanceof and cast it.

Firstly, I know that second. Secondly, no I cannot:

            if (SoulboundItem.getByItem(item) instanceof SoulboundItem) {
                System.out.println("soulbound");
                return true;

            }

Never returns true for soulbound items.

mfnalex commented 4 years ago

You can always do an instanceof check and cast it, adding it to each implementation of SlimefunItem wouldn't be practical in my opinion.

But more importantly, if you are trying to check if an ItemStack is soulbound, then this is the wrong class to look for, only some soulbound SlimefunItems extend SoulboundItem, there is an interface called Soulbound which is used to indicate this behaviour. And even then, using a Soulbound Rune, any ItemStack can be marked as "soulbound", so what you are really looking for is the following method: SlimefunUtils.isSoulbound(ItemStack) which covers both, predefined Soulbound Items and customly made ones.

I would also remind you to please not use our Bug Tracker for this, we have a discord server for these things. There are much more people around and they can usually answer much faster.

Thanks, I will try that. Although I find it quite pointless to tell me to not use your bugtracker for what I thought was a bug. If a method is called "getXYZ", it should return XYZ.

TheBusyBiscuit commented 4 years ago

Firstly, you can just PR this you know. Secondly, you can also just instanceof and cast it.

Firstly, I know that second. Secondly, no I cannot:
            if (SoulboundItem.getByItem(item) instanceof SoulboundItem) {
                System.out.println("soulbound");
                return true;

            }
Never returns true for soulbound items.

That's because it's the wrong class like I explained above.

Thanks, I will try that. Although I find it quite pointless to tell me to not use your bugtracker for what I thought was a bug. If a method is called "getXYZ", it should return XYZ.

I just wanted to remind you that these sorts of conversations get handled quicker and better on discord. Plus you didn't stick to the format of a bug report, hence why this definitely wouldn't qualify as one. Even if something would qualify as a bug, we still recommend people (in our bug reporting guide) to come onto discord first and exchange with other players who may or may not be able to reproduce the behaviour. It just helps us combat bug report pollution and makes it easier to manage and focus on important fixes.

I can understand the confusion behind this method but I don't think it would really be practical, as it would have to be added to all dozens of subclasses to SlimefunItem if we were to enforce this behaviour. I am open to a debate or pull request on this but I personally don't really see much of a benefit over a simple instanceof check as of right now.

Anyway, I am just asking you kindly to use the correct channels to contact us. I'm still replying here on GitHub, it's just a kind recommendation.

Slimefun / Slimefun4

SoulboundItem#getByItem(ItemStack) always returns instance of SlimefunItem #2105