More Item Comparison Improvements

[APickledWalrus]

Description

Welcome to the third or fourth iteration of item comparison improvement PRs

This PR aims to fix a small issue @TheBentoBox reported to me with air comparisons. The slot ItemStack would have an amount of 0, when we should just handle it as 1 in this case. Essentially, Skript was comparing: 0 air is 1 air

It also fixes an issue with item comparisons where Skript would invert the arguments to try and find a comparator. While this is okay in most cases, it is not okay for item comparisons. EXAMPLE:

player's tool = diamond sword of sharpness 3 named "Sharp Sword"
if the player's tool is a diamond sword of sharpness 3

This is valid because the player's tool has all of the qualities of that second item. It is also allowed to have more. The condition correctly passes.

player's tool = diamond sword of sharpness 3 named "Sharp Sword"
if a diamond sword of sharpness 3 is the player's tool

In this case, the condition also passes. This is invalid and incorrect as the first item (a diamond sword of sharpness 3) does not have all of the qualities player's tool has. It is passing because Skript inverts the arguments.

To solve this, I have registered additional converters with opposite types (e.g. Slot - ItemType AND ItemType - Slot exist now). I also had to make some minor changes to the way Skript searches for comparators, but the behavior should remain the same.

---

Target Minecraft Versions: Any
Requirements: None
Related Issues:

• Bug Report for on right click event with named item #4163
• Condition "Name" of certain items in player's inventory is broken when item have lore #4054
• Wool comparing problem #3669
• Checking if a player has an item (item as variable) #999
• custom helmet example script fix #4053
• compare itemtype #4278
• Variable maybe broken #4292

[APickledWalrus]

Marking as a draft for now as I would like to move some comparison stuff out of the DefaultComparators class and into ItemType / ItemData

EDIT: Nevermind - that was a total disaster and I will focus on such changes for another PR lol - going to focus on fixing #4163

[AyhamAl-Ali]

Well done 🚀
Does this also fix #4163 ?

[APickledWalrus]

Well done 🚀

Does this also fix #4163 ?

Yes - I have amended the description to clarify so 😁

[iOshawott]

I'm using skript nightly version with this improvement but there's problem

My players just found error to duplicate money cuz comparison was blind to name and lore of item

I was using a checks system like that

command /czek [<integer>]:
	trigger:
		if player's balance is greater than arg-1:
			remove arg-1 from player's balance
			add 1 paper of unbreaking 1 named "&b&lCzek" with lore "&a%arg-1%$" to player's inventory
on rightclick with paper:
	if name of player's tool contains "&b&lCzek":
		set {_y} to number of player's tool in player's inventory
		send "Debug: %{_y}%"

and in skript 2.5 everything was working smoothly, on 2.6 even with these improvements it counts unnamed paper to number o checks!

so, welp, the problem is that it counts unnamed paper as player's tool, but when I'm using "remove all player's tool from player's inventory" it's not removing all paper
so removing items is working, but it counts number of these items wrong

[APickledWalrus]

Yep - I was just made aware of this issue the other day :) I will have an update coming later today to this PR.

EDIT: Okay, I have pushed some new commits. Please try them out and let me know how it's working.

[iOshawott]

Yeah, it solved that problem.

[iOshawott]

I'm not sure if it's this feature issue, but my skript

every 1 seconds:
	if {pt} is set:
		if {pt} is greater than 0:
			set {pt} to {pt}-1
		else:
			delete {pt}
on script load:
	set {pt} to 10
on server list ping:
	{pt} is set
	set motd to "&cPrzerwa techniczna trwa jeszcze %{pt}% sekund"

It got bugged and it's not deleting when {pt} is 0 at every 1 seconds
It's working in 2.5 and 2.6 and latest nightly I used but it's not working at https://github.com/SkriptLang/Skript/actions/runs/1052829795

(when I changed it to "if {pt} is greater than 1:" it's working)

======

okay that nightly build is broken XD "else ifs" doesn't work too

=====
I'll try to download it again, maybe something broken at my point...

[APickledWalrus]

using the latest nightly build of this PR I don't seem to have that issue. the following counts down and eventually prints out before terminating:

every 1 seconds:
	if {pt} is set:
		broadcast "BEFORE: %{pt}%"
		if {pt} is greater than 0:
			set {pt} to {pt}-1
		else:
			delete {pt}
		broadcast "AFTER: %{pt}%"
on script load:
	set {pt} to 10

EDIT: I have updated the PR to the latest commit as there were actually issues with conditions and such on the Skript build this PR was based on. Please download the latest nightly build from that commit.

[iOshawott]

Oh, yes counting it's okay, but it seems not to delete the variable. I asked my friend to test it with https://github.com/SkriptLang/Skript/actions/runs/1052985157 and this code:

every 1 seconds:
    if {pt} is set:
        if {pt} is greater than 0:
            set {pt} to {pt}-1
            broadcast "%{pt}%"
            wait 1 seconds
        else:
            delete {pt}
            broadcast "The End"
on script load:
    set {pt} to 10

Output on skript 2.5.3 on 1.16.5 looks like that:

Output on this version on 1.17.1 looks like that:

So - on 1.17.1 I'm not getting "The End" message even on empty server where I have only skript

[APickledWalrus]

So it's not working, even with the latest commit to bring this PR up to date with the master branch?

[iOshawott]

So it's not working, even with the latest commit to bring this PR up to date with the master branch?

yes, I compared results with https://github.com/SkriptLang/Skript/actions/runs/1049093891 (which is latest nightly master) and the problem exists here too so it's not this commit issue. I see master have problems with executing "else" (I don't know where to report it since it's nightly). There are a lot of different skripts where else aren't working (but in some cases else is working properly)

command /settest [<integer>]:
	trigger:
		set {test} to arg-1
command /whatstest:
	trigger:
		if {test} is 1:
			send "1"
		else if {test} is 2:
			send "2"
		else if {test} is 3:
			send "3"
		else if {test} is 4:
			send "4"
		else if {test} is 5:
			send "5"
		else:
			send "anything"

When you reload skript and type /settest 1 and then /whatstest - its working. Now without reloading skript type /settest 2 and /whatstest - and it will not work (sends nothing). But when you reload skript - /whatstest command will work again as intended.

I'm not sure if my English is good enough so I prepared a video with example https://www.youtube.com/watch?v=k6XhLVKmUU8

[TPGamesNL]

This is fallout from #4175, and unrelated to this PR.

(I don't know where to report it since it's nightly)

You can report issues with nightly builds the same as you would report normal issues, they're just as valid, if not more. We appreciate people testing nightly builds, since it allows us to fix bugs before releasing a new version.
If the nightly build is from an unmerged PR, then you should report it in a comment on that PR, instead of with an issue.

You can open an issue for this, but I'll probably make a PR fixing this soon

[iOshawott]

I just merged it with "Fix combined conditional running on non-first runs" in master 2 days ago. I'm using it on my server for 2 days and I don't see any issues. Works cool! <3 Good job APickledWalrus :)

[iOshawott]

I got one

I created a written book and named it to "&b&lPrace"
When I'm checking if player's tool is written book named &b&lPrace I'm getting yes it is, but when I'm checking if player HAVE this book... I'm getting that player doesn't have one. I see inconsistency here

command /compare:
	trigger:
		send "%player's tool%"
		if player's tool is written book named "&b&lPrace":
			send "yes it is"
		else:
			send "no it doesn't"
		if player have 1 written book named "&b&lPrace":
			send "yes have"
		else:
			send "no doesn't have"

[AyhamAl-Ali]

Great improvements so far, well done. Btw, is this related? #999

[APickledWalrus]

Sorry for the lack of a response - I have been away recently. I'll take a look into the mentioned issues and update here :)

[APickledWalrus]

@iOshawott I think I got this working as intended now. See results:

[TPGamesNL]

Approving, because the only comment is formatting

[APickledWalrus]

The latest commit makes some changes to how comparisons work for blocks. As of now, it is using the new comparison method. I have also reworked that method to make it clearer on what MatchQuality is required. I have also added a new check for blocks/itemtypes that sets the minimum required quality to SAME_MATERIAL. This quality will apply if the "first" itemtype is not a block but the "second" itemtype is.

EDIT: has issues as expected, will try to patch asap

EDIT 2: Issue should be fixed. To those testing this PR, I would appreciate it if you could test out whether there are any new issues with block comparisons.