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

Potential Off-Hand Duplication fix. #67

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

LogGits
Copy link

@LogGits LogGits commented Dec 19, 2017

This is a potential (I am saying potential because I cant compile the plugin to test it) fix for the off-hand duplication method present in this plugin. Please test before considering merging.

This is a potential (I am saying potential because I cant compile the plugin to test it) fix for the off-hand duplication method present in this plugin. Please test before considering merging.
@LogGits
Copy link
Author

LogGits commented Jan 18, 2018

@LogGits Also this does not compile against 1.12.2 spigot. You are missing imports as it can't find symbols on compile.

Yes I am aware of this most of the maven repositories he is using aren't working and therefore even I was unable to compile it. There are workarounds but im not too sure.

@LogGits Since the primary dev happens to be inactive, could you perhaps look into fixing this issue with deprecated api use for recipe construction?

I will look into it. I am very busy atm but we will see.

getItemInHand is also deprecated, should use explicit methods in PlayerInventory.

Will update :)

What exactly is the off-hand duplication issue? When I test getting on an easel with an item in my offhand and get off the easel, the item isn't kept in my off-hand but is instead moved to main inventory or dropped if inventory is full. There is no duplication, nor can I grab any items from the artkit via off-hand slot out of the easel.

Hold an easel in your offhand and right click on the ground. It isn't taking the easel out because its looking for the item in their main hand (he didn't take into account that the easel is in the second/off hand).

@mibby
Copy link

mibby commented Jan 18, 2018

Yes I am aware of this most of the maven repositories he is using aren't working and therefore even I was unable to compile it.

I'm able to compile the main upstream source by removing many of the optional dependencies from the pom and compatibility modules, such as Residence. As well as compile it against 1.12.2 spigot/bukkit fine after doing so. Doing the same with your commit, it isn't able to find the methods defined so you seem to be missing an important bukkit import. I tried adding import org.bukkit.inventory.PlayerInventory; but that wasn't it. Also tried import org.bukkit.inventory.* but that only resolved 1 of the 4 missing symbols.

Hold an easel in your offhand and right click on the ground. It isn't taking the easel out because its looking for the item in their main hand (he didn't take into account that the easel is in the second/off hand).

Oh! That's what you meant by off-hand duplication! Yep, can confirm that is a problem. Only seems to happen when your main hand is empty (since you can't place items set in off-hand if you have an item in your main hand as well).

However I was also able to experience another issue upon trying to test off-hand issues. If every slot in your inventory is full and you have 1 item in your off-hand, upon sitting on an easel, the off-hand item is brought into the artkit inventory when sitting.

  • Fill entire inventory full of dirt (just to take up slots).
  • Put a sponge in your off-hand slot.
  • Sit on an easel.
  • Notice the sponge is now a part of the easel artkit set.

This works with any item in your off-hand. I can't seem to reproduce any issues with it duplicating the item, only losing the item. As well as this only happens when your entire inventory is full.

I will look into it. I am very busy atm but we will see.

Would be much appreciated!

@mibby
Copy link

mibby commented Jan 18, 2018

Regarding the deprecated recipe API use;

This is a pretty trivial fix for all plugins, and just requires passing a NamespacedKey to the recipe they're creating https://hub.spigotmc.org/javadocs/spigot/org/bukkit/NamespacedKey.html

@LogGits
Copy link
Author

LogGits commented Jan 19, 2018

@mibby Do you have a discord account or spigot account that I can message you about this?

@mibby
Copy link

mibby commented Jan 19, 2018

@LogGits You can reach me on spigot via @mibby, if not on github.

@LogGits
Copy link
Author

LogGits commented Apr 13, 2018

Yeah I will work on a full fix/update soon when I have the time. Got a bunch of assignments and exams to study for and complete first. After that I'll fix it. Its a relatively simple fix theoretically. Just need to know how to properly compile it, to test.

@LogGits
Copy link
Author

LogGits commented Apr 29, 2018

For reference, this is how the Magic plugin solved the recipe issue. Hopefully it is quite trivial to fix and import a similar implementation. :(

I know how to fix the issue as it is quite easy. I am just having troubles compiling.

@mibby
Copy link

mibby commented May 1, 2018

I know how to fix the issue as it is quite easy. I am just having troubles compiling.

What issues are you having compiling? I'm able to compile the main upstream branch fine. If you can push the commit to fix the issue, I can check and see if I can compile your fork with some of the changes noted here LogGits@8193e77#commitcomment-26954834.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants