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

Event Values Fix #7599

Open
wants to merge 5 commits into
base: dev/patch
Choose a base branch
from

Conversation

TheAbsolutionism
Copy link
Contributor

@TheAbsolutionism TheAbsolutionism 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, #delegateConverters

/*
In this method we can delegate/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
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?

@TheAbsolutionism
Copy link
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.

@sovdeeth sovdeeth added the bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. label Feb 16, 2025
# Conflicts:
#	src/main/java/ch/njol/skript/registrations/EventValues.java
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants