Skip to content

Event Values Fix#7599

Merged
sovdeeth merged 7 commits into
SkriptLang:dev/patchfrom
Absolutionism:dev/EventValueFix
Mar 19, 2025
Merged

Event Values Fix#7599
sovdeeth merged 7 commits into
SkriptLang:dev/patchfrom
Absolutionism:dev/EventValueFix

Conversation

@Absolutionism
Copy link
Copy Markdown
Contributor

@Absolutionism Absolutionism commented Feb 9, 2025

Description

This PR aims to fix and update Event Values. (dev/patch branch)

  • For the PlayerDropItemEvent and PlayerPickupItemEvent using event-entity was throwing a parse error of multiple entities which should not be the case. The easy solution to this was to register an Entity value directly to these classes. Though in their corresponding SkriptEvents they are paired with their Entity counterparts, inside EventValueExpression#init only the Player event was being accessed.

  • Adds the #setTime into ExprEntity which utilizes EventValueExpression#setTime allowing past and future event-%entitydata% resulting in the fix of not being able to grab the future event-entity in ItemMergeEvent

  • Adds a new method within EventValues, #stripConverters

/*
In this method we can strip converters that are able to be obtainable through their own 'event-classinfo'.
For example, PlayerTradeEvent has a player value (player who traded) and an AbstractVillager value (villager traded from).
Beforehand, since there is no Entity value, it was then grabbing both values as they both can be casted as an Entity
	resulting in a parse error of "multiple entities"
Now, we filter out the ones that can be obtained using their own classinfo, such as 'event-player'
	which leaves us only the AbstractVillager for 'event-entity'
 */

Though the PlayerTradeEvent could have easily been fixed by changing the valueClass of the registered event value from AbstractVillager to Entity (dont know why it's like that in the first place, as we dont register a class info for abstract villager)
However, I believe this addition can be beneficial.

  • Finally, adds an EventValues.sk test which sole purpose is to look for parse errors.
    Had to fix EffDetonate.sk because EffSecSpawn.lastSpawned was being overwritten by the dropped items that spawned.

  • Also changes variable names within EventValues to match what they are


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

@sovdeeth sovdeeth added the feature Pull request adding a new feature. label Feb 10, 2025
@sovdeeth
Copy link
Copy Markdown
Member

This sounds good and looks alright from a skim. I'm worried about the delegate function though. If a Player value is registered (assume it's the only event-value for this event), does it still allow event-entity to point to the Player?

@Absolutionism
Copy link
Copy Markdown
Contributor Author

If a Player value is registered (assume it's the only event-value for this event), does it still allow event-entity to point to the Player?

Yes, the #delegateConverters method will only attempt if there were two or more converters found.
So in the instance that only a Player value is registered but we're looking for an Entity then it will successfully return the Player.
More than likely being because only one converter was found.

I also have it to where the method is only used after the first and second loops, as those loops are what covers the #hasMultipleConverters and relies on using the converters provided when registering an event value and not the loops that check for registered Converters.

Comment thread src/main/java/ch/njol/skript/registrations/EventValues.java Outdated
@sovdeeth sovdeeth added the bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. label Feb 16, 2025
Comment thread src/main/java/ch/njol/skript/registrations/EventValues.java Outdated
Comment thread src/test/skript/tests/misc/EventValues.sk
# Conflicts:
#	src/main/java/ch/njol/skript/registrations/EventValues.java
@Absolutionism Absolutionism requested a review from sovdeeth March 2, 2025 19:29
Copy link
Copy Markdown
Member

@sovdeeth sovdeeth left a comment

Choose a reason for hiding this comment

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

wonderful work

@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 Mar 15, 2025
@sovdeeth sovdeeth requested a review from APickledWalrus March 15, 2025 23:49
@sovdeeth sovdeeth merged commit e26ea9a into SkriptLang:dev/patch Mar 19, 2025
erenkarakal pushed a commit to erenkarakal/Skript that referenced this pull request Nov 26, 2025
* Initial Commit

* Add 'event-entity' with 'event-player' in parse test

* Revert javadocs

* Requested Changes

* Improve Method Javadocs
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. feature Pull request adding a new feature. 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.

3 participants