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

Fix Item Comparison Issues #3419

Merged
merged 5 commits into from
Oct 5, 2020
Merged

Fix Item Comparison Issues #3419

merged 5 commits into from
Oct 5, 2020

Conversation

APickledWalrus
Copy link
Member

@APickledWalrus APickledWalrus commented Sep 29, 2020

Description

This PR fixes an issue with item meta comparisons. Essentially, we were checking "our" values for nullness / emptyness but not "their" values. This resulted in the following comparison issue:

image
"null" and "Diamond" are definitely not the same. Instead, the following scenario now results in a SAME_MATERIAL quality result.

This actually used to work right before 2.5 alpha 2. At that point in time, we were just checking "their" values which essentially caused this issue but in reverse.

It also fixes issues with skull comparisons and default aliases.

This PR needs more testing before being merged. Input from others is also appreciated.

Target Minecraft Versions: Any
Requirements: Any
Related Issues: #3405 (maybe others too)

@APickledWalrus APickledWalrus added the bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. label Sep 29, 2020
Some of these changes may cause major issues. More extensive testing may be needed (other than the test script)
@APickledWalrus APickledWalrus changed the title Fix Meta Comparison Issues Fix Item Comparison Issues Sep 30, 2020
@APickledWalrus
Copy link
Member Author

Okay, so the latest commit tries to fix more comparison issues by changing the way ItemTypes are compared. I think this way of comparing should work well. I wrote a somewhat extensive test to make sure there aren't any issues, but it's always possible that there could be some.

@iperrealistico
Copy link

"null" and "Diamond" are definitely not the same. Instead, the following scenario now results in a SAME_MATERIAL quality result.

This actually used to work right before 2.5 alpha 2. At that point in time, we were just checking "their" values will essentially caused this issue but in reverse.

Interesting stuff! Infact this started happening after updating to 2.5, the issue was not there in 2.4

@TheLimeGlass
Copy link
Collaborator

TheLimeGlass commented Oct 2, 2020

@APickledWalrus I suggest posting a built jar in this conversation, as most Skript users don't know how to compile and it would help with the testing.

Does this pull essentially fix if event-item is light green stained glass named "&aLimeGlass": which has been a long time known bug where the condition is always false. That's probably the main cause.

@iperrealistico
Copy link

I just tried the last nightly build (74fd002) and nope, the issue is still there...

		if arg-player has arg-number of diamonds named "":
			remove arg-number of diamonds named "" from arg-player's inventory
			add {_worth} to arg-player's money
			play sound "block.metal.step" with volume 1 and pitch 2 for arg-player

this piece of code counts also renamed diamonds, and removes them from player's inventory... as usual, it used to work before 2.5 came out

@APickledWalrus
Copy link
Member Author

I just tried the last nightly build (74fd002) and nope, the issue is still there...

		if arg-player has arg-number of diamonds named "":
			remove arg-number of diamonds named "" from arg-player's inventory
			add {_worth} to arg-player's money
			play sound "block.metal.step" with volume 1 and pitch 2 for arg-player

this piece of code counts also renamed diamonds, and removes them from player's inventory... as usual, it used to work before 2.5 came out

You will need to try the latest build of this PR. You can try downloading it here: https://github.com/SkriptLang/Skript/suites/1272558230/artifacts/19615437

If that doesn’t work, let me know and I can upload a jar here

@APickledWalrus
Copy link
Member Author

@APickledWalrus I suggest posting a built jar in this conversation, as most Skript users don't know how to compile and it would help with the testing.

Does this pull essentially fix if event-item is light green stained glass named "&aLimeGlass": which has been a long time known bug where the condition is always false. That's probably the main cause.

I meant to reply to this earlier... whoops. In the case you give, if I use the right click event it does successfully compare, so yes I suppose. The main goal of this PR is to try and fix many, if not all, of the comparison issues with items.

@iperrealistico
Copy link

This. Is. Finally. Fixed. Thank you! Merge this damn fix 😆

@iperrealistico
Copy link

This isn't probably realted to your build but maybe you know what is causing this error in the console, it happens sometimes:

err.txt

@iperrealistico
Copy link

Also, is this fix included in the last release? The official skript 2.5

@APickledWalrus
Copy link
Member Author

Also, is this fix included in the last release? The official skript 2.5

It is not. However, the Skript jar I linked above is Skript 2.5 + this fix.
This fix should be included in Skript 2.5.1

@APickledWalrus
Copy link
Member Author

This isn't probably realted to your build but maybe you know what is causing this error in the console, it happens sometimes:

err.txt

You can open a separate issue report for this.

@APickledWalrus APickledWalrus changed the base branch from master to dev-2.5 October 5, 2020 11:18
Copy link
Contributor

@ShaneBeee ShaneBeee left a comment

Choose a reason for hiding this comment

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

As long as this all works, and doesn't cause issue, it looks good to me 👍

@APickledWalrus APickledWalrus merged commit 9149978 into SkriptLang:dev-2.5 Oct 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue that needs to be fixed. Alternatively, a PR fixing an issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants