Add damaged slot to PlayerItemDamageEvent by MTM123 · Pull Request #2153 · PaperMC/Paper

MTM123 · 2019-06-09T10:36:24Z

Continuation of #1834
(Unfortunately had to create a new PR because couldn't reopen the original PR)

BillyGalbreath · 2019-10-19T20:13:20Z

+                    } else if (entityplayer.getItemInOffHand() == this) {
+                        slot = 104; //Shield slot: (last armor slot + 1)
+                    } else {
+                        int w = 100; //First armor slot. Based on https://bukkit.org/threads/inventory-slots.431469/


I'm curious why you chose that outdated image to set your slot numbers instead of the API specification? I'm not sure where slot numbers 100-104 are going to come in handy since they cannot be used with setItem(slot), etc.

MTM123 · 2019-10-24T14:01:59Z

I initially used the slots you recommended I believe but there was a debate about future proofing and there was no decision on this and the final decision was up to project leads. I guess I could correct to it to slots you mentioned and it would make more sense to do so.

Proximyst · 2020-11-14T20:05:10Z

+     private static final HandlerList handlers = new HandlerList();
+     private final ItemStack item;
+     private int damage;
+    private final int slot; //Paper - add damaged slot


Suggested change

+ private final int slot; //Paper - add damaged slot

+ private final int slot; // Paper - add damaged slot

Proximyst · 2020-11-14T20:05:41Z

+         super(player);
+         this.item = what;
+         this.damage = damage;
+        this.slot = -1;


Delegate to a new constructor instead

Proximyst · 2020-11-14T20:05:45Z

+         this.damage = damage;
+     }
+
+    //Paper start


Suggested change

+ //Paper start

+ // Paper start

Proximyst · 2020-11-14T20:05:56Z

+                 if (entityplayer != null) {
+-                    PlayerItemDamageEvent event = new PlayerItemDamageEvent(entityplayer.getBukkitEntity(), CraftItemStack.asCraftMirror(this), i);
+
+                    //Paper start - getting slot that was damaged


Suggested change

+ //Paper start - getting slot that was damaged

+ // Paper start - getting slot that was damaged

Proximyst · 2020-11-14T20:06:40Z

+-                    PlayerItemDamageEvent event = new PlayerItemDamageEvent(entityplayer.getBukkitEntity(), CraftItemStack.asCraftMirror(this), i);
+
+                    //Paper start - getting slot that was damaged
+                    int slot = -1;


Should have a look at how e.g. book modification handling is done for getting the slot...

I'm not sure what you are thinking of exactly

Trigary

I am not sure whether using an integer slot ID is a good idea. Why not just use the EquipmentSlot enum? Sure, it would limit the event, but it would provide a much more useful API. In what kind of use case is the slot ID useful? The enum would be a lot more useful in my opinion.

Trigary · 2020-11-30T18:43:55Z

+         super(player);
+         this.item = what;
+         this.damage = damage;
+        this.slot = -1;


// Paper is missing

Trigary · 2020-11-30T18:49:36Z

+    //Paper start
+    /**
+     * Gets the slot where the item took durability damage
+     *


Maybe add an example or how this slot can be used: how to get the ItemStack at this slot from a Player instance. (But that's a stupid use case since the ItemStack is directly exposed in this method.)

MTM123 · 2020-11-30T19:02:34Z

I think using EquipmentSlot would make more sense indeed because you can already get ItemStack from the event so I guess I will change that then.

EDIT: Though this limits plugins that might call this event to slots defined by EquipmentSlot

Machine-Maker · 2020-11-30T19:03:43Z

I am not sure whether using an integer slot ID is a good idea. Why not just use the EquipmentSlot enum? Sure, it would limit the event, but it would provide a much more useful API. In what kind of use case is the slot ID useful? The enum would be a lot more useful in my opinion.

I suppose its possible in the future for itemstacks to be "damaged" even if they are not in one of the slots covered by the EquipmentSlot enum. That is already possible via nms, just calling the isDamaged method on the nms ItemStack.

Trigary · 2020-11-30T19:07:50Z

Exposing both the slot ID and the equipmentslot enum is still an option. Maybe calculate the slot id in the constructor based on the enum value (if the constructor with the enum value was used).

stale · 2021-01-29T20:11:34Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

MTM123 · 2021-02-02T15:47:02Z

Talking about this in discord there were few changes suggested by @Trigary and @yannicklamprecht:

Expose raw slot to PlayerItemDamageEvent and create a new event EquipmentDamageEvent that extends PlayerItemDamageEvent that would expose just the EquipmentSlot. This would give ability to somewhat futureproof the event also exposing the raw slot to PlayerItemDamageEvent would allow plugin authors to call the event on items in other slots other than the ones defined by EquipmentSlot.
Stick to just PlayerItemDamageEvent and add both raw slot and EquipmentSlot making EquipmentSlot nullable giving plugin authors the same ability as stated in first solution.

kennytv · 2021-04-24T07:22:23Z

The second should be future-proof enough (and less confusing imo) for plugin devs to already check for the nullable equipment slot, depending on what they need.

BillyGalbreath reviewed Oct 19, 2019

View reviewed changes

Proximyst added status: rebase required type: feature Request for a new Feature. labels Aug 24, 2020

Proximyst suggested changes Nov 14, 2020

View reviewed changes

MTM123 added 2 commits November 29, 2020 18:00

Add damaged slot to PlayerItemDamageEvent

a12dc7b

Update to latest

d389925

MTM123 force-pushed the damaged-item-slot branch from 3ae7d24 to d389925 Compare November 30, 2020 18:17

MTM123 changed the base branch from ver/1.14 to master November 30, 2020 18:37

MTM123 requested a review from a team as a code owner November 30, 2020 18:37

Trigary suggested changes Nov 30, 2020

View reviewed changes

stale Bot added the resolution: stale label Jan 29, 2021

stale Bot removed the resolution: stale label Feb 2, 2021

MTM123 closed this Jun 17, 2021

	+ private final int slot; //Paper - add damaged slot
	+ private final int slot; // Paper - add damaged slot

	+ //Paper start - getting slot that was damaged
	+ // Paper start - getting slot that was damaged

Uh oh!

Conversation

MTM123 commented Jun 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MTM123 commented Oct 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trigary left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MTM123 commented Nov 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Machine-Maker commented Nov 30, 2020

Uh oh!

Trigary commented Nov 30, 2020

Uh oh!

stale Bot commented Jan 29, 2021

Uh oh!

MTM123 commented Feb 2, 2021

Uh oh!

kennytv commented Apr 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

MTM123 commented Nov 30, 2020 •

edited

Loading

kennytv commented Apr 24, 2021 •

edited

Loading