Skript icon indicating copy to clipboard operation
Skript copied to clipboard

Fix player death event related syntaxes

Open TheLimeGlass opened this issue 2 years ago • 3 comments

Description

Fixes EffKeepInventory, EffRespawn force, ExprLevel not displaying a warning, and ExprMessage death message all not working.

Skript never had PlayerDeathEvent registered so Skript would never understand the event during parse time.

Existing syntaxes that rely on EntityDeathEvent still work as intended because PlayerDeathEvent extends EntityDeathEvent like attacker, victim etc.

This pull request also allows for

on death:
	if victim is a player:
		keep inventory

Target Minecraft Versions: any Requirements: none Related Issues: https://github.com/SkriptLang/Skript/issues/5657 and https://github.com/SkriptLang/Skript/issues/2285 which was closed as a Spigot bug when in reality it was Skript not working properly for the force respawn.

TheLimeGlass avatar May 01 '23 22:05 TheLimeGlass

I'm not really a fan of putting in a hardcoded check like this into CondCompare. I would prefer us to take less invasive options, and I'll note a few possibilities below:

1. Only allow the syntax in an explicit `on death of player` declaration (this is a breaking change and may limit users more)

2. Allow the syntax in any kind of death event and just depend on the existing event instanceof check (should not be a breaking change and gives the user the most freedom, but there is less error checking for the user)

The existing implementation certainly has a good balance between error checking and user freedom, but I think it's invasiveness weights it down too greatly.

The old implementation disallows usage of syntaxes as it cannot understand the condition if the expression type that goes inside the pattern is unknown. This pull request opens the ability to not silently hide errors that the user should know about and enables syntaxes to start working as Skript would then know the proper type.

There are more events that would benefit from this enhancement and not just the death event like the force respawn and others as mentioned in the first introduction message. So I don't agree with restricting to just death events.

TheLimeGlass avatar Aug 27 '23 12:08 TheLimeGlass

Is this even still useful since https://github.com/SkriptLang/Skript/pull/5658 has been merged?

sovdeeth avatar Apr 07 '24 14:04 sovdeeth

Is this even still useful since #5658 has been merged?

Yes, this is still valid.

TheLimeGlass avatar Apr 07 '24 15:04 TheLimeGlass

Thank you for this contribution. However, after discussing with the team, we do not think that the current approach is suitable for Skript. We may consider a more general system for having syntax like CondCompare affect the ParserInstance and its data, and you are welcome to provide ideas for that. For now, though, we will be closing this PR.

APickledWalrus avatar Jan 18 '25 21:01 APickledWalrus