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 keep inv/exp and other bugs related to death event #5658

Merged

Conversation

AyhamAl-Ali
Copy link
Member

Description

This bug was due to the change of how isCurrentEvent works (no super class support)
A simple change in init method to use EntityDeathEvent (super class) instead of PlayerDeathEvent while keeping the check for PlayerDeathEvent in execute method


Target Minecraft Versions: All
Requirements: None
Related Issues:

@AyhamAl-Ali AyhamAl-Ali added the bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. label Apr 30, 2023
@AyhamAl-Ali AyhamAl-Ali changed the title 🐛 Fix keep inv/exp in death event 🛠 Fix keep inv/exp in death event Apr 30, 2023
@AyhamAl-Ali AyhamAl-Ali added the 2.7 Targeting a 2.7.X version release label Apr 30, 2023
@TheLimeGlass
Copy link
Collaborator

Well it does this because the death event Skript registers is the entity death event. I don't know if this is the correct approach to this fix.

If no player is even involved in the event, it'll be misleading. Like on death of pig and attacker is not a player. This will not error at parse time, but will not work at runtime leading people to believe it may be broken.

@AyhamAl-Ali
Copy link
Member Author

Did it error at parse time before with the case you mentioned?
If you have another solution let me know.

@TheLimeGlass
Copy link
Collaborator

Did it error at parse time before with the case you mentioned? If you have another solution let me know.

It wasn't a default expression before, so it never had to grab from the event.

A solution could be reading from the event, it's all a literal so it'll be able to figure out if the entity type "should" be that literal.
Another could be actually getting Skript to use the PlayerDeathEvent rather than EntityDeathEvent when it comes to a player.

@TheLimeGlass TheLimeGlass self-requested a review May 1, 2023 04:15
@AyhamAl-Ali
Copy link
Member Author

AyhamAl-Ali commented May 1, 2023

default expression

How is it a default expression now? nothing has changed except the check in init method

As for the event check it didn't error at parse time before because the event is indeed registered as EntityDeathEvent and the code can't know if it's PlayerDeathEvent until runtime.

In addition to that, you can't know if the victim is player in init, even if you check for event params, it means nothing because users might use inner conditions to check for victim is player

The current simple check for PlayerDeathEvent in execute method is enough to not throw any errors.

@TheLimeGlass TheLimeGlass removed the 2.7 Targeting a 2.7.X version release label May 9, 2023
@TheLimeGlass
Copy link
Collaborator

I find #5663 to be a superior solution.

@AyhamAl-Ali
Copy link
Member Author

I find #5663 to be a superior solution.

I agree with Pickle's comment, therefore this solution (this PR) is better, is not a breaking change and more reliable that hardcoding exceptions

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.

Good

@AyhamAl-Ali AyhamAl-Ali changed the title 🛠 Fix keep inv/exp in death event 🛠 Fix keep inv/exp and other bugs related to death event Sep 13, 2023
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.

5 participants