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 variable type reference being incorrect when same literals #5504

Closed
wants to merge 6 commits into from

Conversation

TheLimeGlass
Copy link
Collaborator

@TheLimeGlass TheLimeGlass commented Mar 10, 2023

Description

Makes a fix for re-parsing variables that have the same literal name.

Example of this can be setting a variable to a snowball. Skript assumes it's an itemtype.
Now when using that variable in an entitydata expression, Skript attempts to convert but can't
as no converters exist for ItemType -> EntityData. That's where this fix steps in.

With the knowledge from my pull request at #5503 I knew exactly what Skript was doing and how to fix it.

I also cleaned up the EffShoot class to be up to date with standards while I was debugging.

Note that this pull request #5457 also helps this issue and solidifies that Skript will understand local variables


Target Minecraft Versions: any
Requirements: none
Related Issues: #3753, #5493 there are probably more, but nothing came up relating to index "variable"

@TheLimeGlass TheLimeGlass added the bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. label Mar 10, 2023
@TheLimeGlass TheLimeGlass mentioned this pull request Mar 19, 2023
1 task
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.

A useful fix.

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.

Putting a little pause on this for now, testers have noticed that this is introducing a lot of unexpected type coercion that needs to be addressed.

@Moderocky Moderocky force-pushed the master branch 2 times, most recently from bd134d0 to 3f08853 Compare September 16, 2023 16:59
@Moderocky Moderocky changed the base branch from master to dev/feature September 18, 2023 10:20
@sovdeeth
Copy link
Member

The type coercion that Kenzie mentioned earlier is still an issue:

on load:
    set {_a} to "2"
    set {_b} to "5 dirt of sharpness"
    set {_c} to 12 tnt entities
    broadcast "2 + {_a} - expected: 2, actual: %2 + {_a}%"
    broadcast "enchantments of {_b}: expected: <none>, actual: %enchantments of {_b}%"
    broadcast "item amount of {_c}: expected: <none>, actual: %item amount of {_c}%" 

2.7.1:
image

This PR's nightly:
image

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.

Conflicts need resolving

@sovdeeth
Copy link
Member

Closing for inactivity

@sovdeeth sovdeeth closed this Sep 13, 2024
@APickledWalrus APickledWalrus deleted the fix/shoot-variable branch October 13, 2024 18:49
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.

3 participants