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 Particle Definition Conflicts #6760

Merged

Conversation

APickledWalrus
Copy link
Member

@APickledWalrus APickledWalrus commented Jun 3, 2024

Description

This PR aims to fix some particle-itemtype conflicts that were introduced in #6716

For example, some inputs such as totem of undying would parse as a visual effect rather than an itemtype. This caused errors with variables as visual effect serialization is broken. To combat this, I've made it so that visual effects are parsed after item types. Additionally, I've added some additional optional keywords to the affected particle definitions.

We might also prefer a more aggressive route of requiring the usage of keywords like particle. I wanted to avoid this if possible. We may choose to pursue this if the parse-order changes reveal any unexpected issues.

There is one breaking change that affects the elder guardian option that was added in 2.8.5. It now requires particle. I attempted to force visual effect parsing after entity types/data, but that caused unexpected test failures.


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

@APickledWalrus APickledWalrus added the bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. label Jun 3, 2024
@APickledWalrus APickledWalrus changed the base branch from master to dev/patch June 3, 2024 04:59
@sovdeeth
Copy link
Member

sovdeeth commented Jun 3, 2024

Can we get a specific regression test for this?
also, re: requiring particle, that's likely what I'm going to do with particle rework (each particle/effect would have "particle", "effect", or "sound" as part of its pattern to help avoid ambiguity, depending on what it is)

@sovdeeth sovdeeth mentioned this pull request Jun 3, 2024
1 task
This is needed as firework may also represent an entitydata.
@APickledWalrus APickledWalrus added the needs testing Needs testing to determine current status or issue validity, or for WIP feature pulls. label Jun 3, 2024
@sovdeeth sovdeeth added the patch-ready A PR/issue that has been approved and is ready to be merged/closed for the next patch version. label Jun 8, 2024
@sovdeeth sovdeeth mentioned this pull request Jun 8, 2024
@APickledWalrus APickledWalrus merged commit aa7955c into SkriptLang:dev/patch Jun 15, 2024
5 checks passed
@APickledWalrus APickledWalrus deleted the patch/particle-conflicts branch June 15, 2024 16:59
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. needs testing Needs testing to determine current status or issue validity, or for WIP feature pulls. patch-ready A PR/issue that has been approved and is ready to be merged/closed for the next patch version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants