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

Damage effect fixes #4003

Merged
merged 6 commits into from
Jun 9, 2021
Merged

Conversation

TPGamesNL
Copy link
Member

Description


Target Minecraft Versions: any
Requirements: none
Related Issues: #3997, #3715 (also #3719, #2755, #2836)

@TPGamesNL TPGamesNL added bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. 2.6 labels May 14, 2021
@TPGamesNL TPGamesNL marked this pull request as draft May 14, 2021 18:27
@Jjman739
Copy link

Jjman739 commented May 14, 2021

Removes the fake damage cause part of the damage effect, because there's no way to implement it (it probably previously worked because of the aforementioned bug). If you want to retain the functionality, set the last damage cause after the damage effect.

In my testing, I was able to get it to work, with the caveat that the damage effect processes twice (first for the designated cause and then again for "unknown" if the first was not cancelled.) Are you sure there's no way?

I was going to do more research to see if I could get it to only call the damage once.

@TPGamesNL
Copy link
Member Author

The first event call you had, was from Skript calling the event manually (which I removed, as it shouldn't do this). The second is the actual event being called from your server, the damage cause of which cannot be set without reflection (and probably NMS).

@Jjman739
Copy link

Jjman739 commented May 14, 2021

Yeah... upon further analysis, I noticed that the event call doesn't deal the damage directly, instead only being used to check if the event is being cancelled.

Why shouldn't it call the event manually, though? If the damage event can be made to apply the damage, that should solve the issue, unless I'm misunderstanding the way Bukkit events work.

@TPGamesNL
Copy link
Member Author

TPGamesNL commented May 14, 2021

Bukkit events are only called to let plugins know something happened, no internal server stuff relies on events being called. The player will therefore not be damaged if an event is called, all it will do is confuse other plugins (and scripts), because it seems like the player is taking damage twice, even though it's only once.

EDIT: well done on the debugging and testing, by the way 👍

@Jjman739
Copy link

(Thanks)

It's probably too hacky to have it call the event and then play the hit animation and reduce the HP directly in accordance with the final damage amount, right?

@TPGamesNL TPGamesNL requested a review from FranKusmiruk May 15, 2021 09:54
@TPGamesNL
Copy link
Member Author

It's probably too hacky to have it call the event and then play the hit animation and reduce the HP directly in accordance with the final damage amount, right?

Yep

@TPGamesNL TPGamesNL marked this pull request as ready for review May 15, 2021 10:29
@TPGamesNL TPGamesNL requested a review from FranKusmiruk May 16, 2021 08:05
Copy link
Member

@APickledWalrus APickledWalrus left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@FranKusmiruk FranKusmiruk merged commit 9b39c45 into SkriptLang:master Jun 9, 2021
@TPGamesNL TPGamesNL deleted the fix/damage-effect branch June 10, 2021 06:23
@Fusezion Fusezion mentioned this pull request Feb 18, 2023
1 task
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.

4 participants