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 'any sword' represents multiple types error #6263

Closed

Conversation

Fusezion
Copy link
Contributor

Description

This PR aims to fix the if player's tool is any sword comparison no longer working as intended due to a change somewhere in #5815. I am targeting dev/feature if this is incorrect please tell me I'll spend the 10 hours to fix it.

From what I've been able to tell this should of been caused by ItemStack parser not properly checking for ItemType#isAll I've now add a check for it as well removed some finals within the method.

While Pickle wasn't able to replicate it my guess is a change that was needed happened and brought to light this comparison issue.

Screenshot of it working in action, ignore the broadcast, this was a build with my debug messages
image


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

@sovdeeth sovdeeth added bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. 2.8 Targeting a 2.8.X version release labels Dec 29, 2023
@TheLimeGlass
Copy link
Collaborator

I believe the assertion should still remain. Also a test should be added so that assertion can be caught.

@Fusezion
Copy link
Contributor Author

I believe the assertion should still remain. Also a test should be added so that assertion can be caught.

Assert shouldn't be needed as it should never be met, itemtype should be NonNull.

Added a test however every sword is every sword seems to be saying it's passing when it should be failing as it does on the server running same code.

@Fusezion
Copy link
Contributor Author

Fusezion commented Dec 29, 2023

Alright so after a dumb test, something else is going on here.
image

command /testing:
	trigger:
		if stone sword is any sword: # pass
			broadcast "passed - stone sword is any sword"
		if any sword is any sword: # pass
			broadcast "passed - any sword is any sword"
		if stone sword is every sword: # fail
			broadcast "failed - stone sword is every sword"
		if every sword is every sword: # fail
			broadcast "failed - every sword is every sword"

Note: I do believe every sword is every sword should pass, however if that's the case the error should be handled on it's own comparator not in a parser. Skript doesn't even handle this as an error and should be treated more as a warning if you ask me.

So I'm in favor of changing from Skript.error() to something like Skript.warning()

@Fusezion
Copy link
Contributor Author

Swapping over to a draft pr for a bit, issue is caused by a custom build for an addon causing itemstack parser to be called needlessly in cases like the issues above. This is also why pickle was not able to replicate. Internally this change is needed, however I want to first find how and why this error happened since the only left overs in my build are classes not finished but also no Skript.register methods being called. There's no startup issues either so it's a far fetch issue.

I work in under 3 hours, so this might not be marked as ready until tomorrow.

@Fusezion Fusezion marked this pull request as draft December 29, 2023 17:36
@APickledWalrus
Copy link
Member

Closing in favor of #6266

@Fusezion Fusezion deleted the fix/is_any_comparison branch January 2, 2024 04:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.8 Targeting a 2.8.X version release 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