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

Simplify offhand swapping #5772

Open
wants to merge 11 commits into
base: dev/feature
Choose a base branch
from
Open

Conversation

TheLimeGlass
Copy link
Collaborator

@TheLimeGlass TheLimeGlass commented Jun 28, 2023

Description

Simplify number key and swap offhand inventory click events.

So this may be hard to understand, but i'll explain.

I was helping a Skript user on SkUnity Discord and their problem was a hard understanding of how to only cancel an event when a specific item was swapped using the number keys in an inventory click event from the hotbar to the offhand player slot when having the player's inventory open, and the only solution was;

on inventory click:
	if click type is number key:
		index of event-slot is 40:
			slot hotbar button of event-inventory is a diamond:
				broadcast "yes"

This pull request simplifies this process with

on inventory click:
	if offhand item will be a diamond:
		broadcast "yes"

This will mean no matter how the click event happens, if the offhand is modified, it'll be tracked with this future state.

I also removed the default expression from the setTime as it makes no sense to restrict it here. This basically means that offhand item would have worked and offhand item of player would not, because it would only work if it was a default expression. It now works for both.


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

@TheLimeGlass TheLimeGlass added the feature Pull request adding a new feature. label Jun 28, 2023
@TheLimeGlass TheLimeGlass changed the title Simplify number key offhand swapping Simplify offhand swapping Jun 28, 2023
@TheLimeGlass TheLimeGlass added enhancement Feature request, an issue about something that could be improved, or a PR improving something. and removed feature Pull request adding a new feature. labels Jun 28, 2023
@TheLimeGlass TheLimeGlass marked this pull request as draft June 28, 2023 04:45
@TheLimeGlass TheLimeGlass marked this pull request as ready for review June 28, 2023 05:18
Copy link
Member

@Moderocky Moderocky left a comment

Choose a reason for hiding this comment

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

Good in general but all the casting is a mess.

src/main/java/ch/njol/skript/expressions/ExprTool.java Outdated Show resolved Hide resolved
@sovdeeth sovdeeth changed the base branch from master to dev/feature December 30, 2023 07:50
@sovdeeth
Copy link
Member

Closing due to inactivity.

@sovdeeth sovdeeth closed this Jul 16, 2024
@TheLimeGlass TheLimeGlass reopened this Jul 16, 2024
Copy link
Member

@sovdeeth sovdeeth left a comment

Choose a reason for hiding this comment

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

This PR needs a LOT more testing, it only works in a few specific cases right now.

it's also odd that offhand tool would properly support future values while tool still doesn't

src/main/java/ch/njol/skript/expressions/ExprTool.java Outdated Show resolved Hide resolved
} else if (inventoryClickEvent.getClick() == ClickType.SWAP_OFFHAND) {
PlayerInventory inventory = inventoryClickEvent.getWhoClicked().getInventory();
return new InventorySlot(inventory, inventoryClickEvent.getSlot());
} else if (inventoryClickEvent.getSlot() == EquipmentSlot.EquipSlot.OFF_HAND.slotNumber) {
Copy link
Member

Choose a reason for hiding this comment

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

What about the case where another slot is being double clicked to collect_to_cursor, which pulls from the offhand? Currently it just says null for the future offhand tool.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't believe Spigot/Minecraft has an API for determining the slots a double click is pulling from.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah you'd have to manually write checks for this (from my experience with skript-gui)

src/main/java/ch/njol/skript/expressions/ExprTool.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/expressions/ExprTool.java Outdated Show resolved Hide resolved
Copy link
Contributor

@Fusezion Fusezion left a comment

Choose a reason for hiding this comment

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

Just a small unnoticed detail

src/main/java/ch/njol/skript/expressions/ExprTool.java Outdated Show resolved Hide resolved
Copy link
Member

@Moderocky Moderocky left a comment

Choose a reason for hiding this comment

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

Looks good :)

@@ -18,18 +18,6 @@
*/
Copy link
Member

Choose a reason for hiding this comment

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

remove license

case PLACE_ONE:
case PLACE_SOME:
case SWAP_WITH_CURSOR:
return new CursorSlot((Player) inventoryClickEvent.getWhoClicked(), inventoryClickEvent.getCurrentItem());
Copy link
Member

Choose a reason for hiding this comment

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

This should be returning the value of the future offhand slot. Right now it's returning the future cursor slot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature request, an issue about something that could be improved, or a PR improving something.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants