-
Notifications
You must be signed in to change notification settings - Fork 8
Feat: Add event classes for brewing #129
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
base: master
Are you sure you want to change the base?
Conversation
4df9dc5 to
e59821f
Compare
dd384bd to
45dc006
Compare
49b7283 to
3ab61b0
Compare
|
(assigning all the team as reviewers, as it's in dupe sensitive territory) |
Jsinco
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ItemTransaction data class kind of scares me. I understand that it has to be exposed to the API but I'd consider marking the next release as experimental at least for a little while
|
|
||
| private boolean cancelled = false; | ||
| @Getter | ||
| private boolean denied = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe don't use lombok here and document what denied is? Not super apparent at a glance, but that could just be me
|
|
||
| public interface Permissible extends Cancellable { | ||
|
|
||
| boolean isDenied(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like denied and cancelled have too similar of a meaning in these events. Maybe just use cancelled instead of both? Or possibly take a page out of Bukkit's book and use an enumerator to more explicitly declare what is happening
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look again, I changed how it works now.
| inventoryAccessible, | ||
| new ItemTransaction.RawPosition(event.getRawSlot()), | ||
| inventoryPosition, | ||
| event.getCurrentItem(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The event.getCurrentItem() and other items referenced by the event are mapped to real items in memory (either items or slots, Spigot docs aren't clear on this), so Spigot could modify the item before the next tick. Is this properly accounted for in the scheduled task? If not, we may need to clone ItemStacks before passing them to ItemSource.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems to be true, as when I cloned it, the issue with items not converting format got fixed. I think not cloning it is just going to cause undefined behavior, which is why it probably worked when I tested it.
|
A small bug: Taking a brew with expanded lore out of a barrel does not reset the lore to how it should appear in-inventory. |
I only tested with a distillery, which is where it looks like it worked, it should be the same with barrels. How did you take out the brew? |
Shift-clicking and left-click pickup both cause the issue. You have to leave the brew in a barrel long enough for it to update and get expanded lore. |
|
I forgot to include hopper based inventory transactions as well. Those need to be tracked too 😃 |
|
And inventory drag event? It will probably take a while until I would be able to complete this myself, any help is appreciated. |
c62114a to
eda0ec5
Compare
eda0ec5 to
8f1e111
Compare
cauldron extract is not an inventory transaction, a single BrewBasedSource is sufficient
|
Seems like most of the issues have been worked out now. The events need a way to modify the item/brew being inserted or extracted (possibly a simple |
See ItemTransactionSession, or whatever that is called. |
One for each structure type