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.

Sign up for GitHub

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

Jump to bottom

Support damage entity with damage-causes in MC 1.20.4+ #7044

Merged
merged 31 commits into from
Dec 24, 2024
Merged

Support damage entity with damage-causes in MC 1.20.4+ #7044

merged 31 commits into from
Dec 24, 2024

Conversation

0XPYEX0
Copy link
Contributor

@0XPYEX0 0XPYEX0 commented Sep 6, 2024

Description

As title

Target Minecraft Versions: 1.20.4+
Requirements: none
Related Issues: none

sovdeeth
sovdeeth requested changes Sep 7, 2024
View reviewed changes
Copy link
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.

tests failing

@0XPYEX0 0XPYEX0 requested a review from sovdeeth September 8, 2024 07:04
cheeezburga
cheeezburga reviewed Sep 9, 2024
View reviewed changes
Copy link
Member

@cheeezburga cheeezburga left a comment

Choose a reason for hiding this comment

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

Looks pretty good, just some things here and there. Also, would love to see the description and examples updated.

src/main/java/ch/njol/skript/effects/EffHealth.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffHealth.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffHealth.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffHealth.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffHealth.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffHealth.java Outdated Show resolved Hide resolved
@0XPYEX0 0XPYEX0 requested a review from cheeezburga September 9, 2024 09:12
@cheeezburga cheeezburga added enhancement Feature request, an issue about something that could be improved, or a PR improving something. 2.10 Targeting a 2.10.X version release and removed 2.10 Targeting a 2.10.X version release labels Sep 9, 2024
@0XPYEX0 0XPYEX0 requested a review from cheeezburga September 10, 2024 03:39
@0XPYEX0
Copy link
Contributor Author

0XPYEX0 commented Sep 10, 2024

Maybe I should move HealthUtils.damage() to DamageUtils.damage() ? @cheeezburga

@0XPYEX0 0XPYEX0 requested a review from cheeezburga September 10, 2024 03:56
@cheeezburga
Copy link
Member

Maybe I should move HealthUtils.damage() to DamageUtils.damage() ? @cheeezburga

I don't think so. All the methods that modify the health of a damageable should be left in HealthUtils.

Fusezion
Fusezion suggested changes Sep 10, 2024
View reviewed changes
Copy link
Contributor

@Fusezion Fusezion left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Just some mild issues with this, outside of missing any addition test for with fake damage cause so we should look into that as well.

src/main/java/ch/njol/skript/effects/EffHealth.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffHealth.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffHealth.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffHealth.java Outdated Show resolved Hide resolved
@0XPYEX0 0XPYEX0 requested a review from cheeezburga September 10, 2024 04:55
@0XPYEX0
Copy link
Contributor Author

0XPYEX0 commented Sep 21, 2024

idk how to fix the mark problem

0XPYEX0

This comment was marked as spam.

Sign in to view

@0XPYEX0 0XPYEX0 requested a review from sovdeeth October 1, 2024 13:15
@cheeezburga cheeezburga added feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. and removed feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. labels Oct 5, 2024
@0XPYEX0
Copy link
Contributor Author

0XPYEX0 commented Oct 5, 2024

Actually, i don't think the test codes are necessary
I cannot solve the mark problem, causing the failure of test
The method just marked the entity to be damaged, so assert error

@Moderocky Moderocky added help wanted Contributions are highly welcomed priority: medium Issues that are detrimental to user experience (prohibitive bugs or lack of useful implementation). labels Nov 30, 2024
@Moderocky
Copy link
Member

It looks like the creator needs help with fixing the tests for this. If somebody would be willing to assist, that would be good.

@Moderocky
Copy link
Member

So I've investigated this and solved the test issue: these new experimental methods process more 'natural' damage, which goes through the damage logic, triggering damage mitigation, invincibility frames, etc.

I've fixed the test by creating a new entity for the damage source tests, but this might become a problem in future since the behaviour is quite unpredictable.

@Moderocky Moderocky merged commit 96fb660 into SkriptLang:dev/feature Dec 24, 2024
3 of 5 checks passed
@Moderocky Moderocky mentioned this pull request Dec 24, 2024
Merged
Moderocky added a commit that referenced this pull request Dec 24, 2024
@Moderocky @0XPYEX0
@sovdeeth @Efnilite
Support damage entity with damage-causes in MC 1.20.4+ (#7044) (#7309)
486158c 
* feat: damage entity with damage-causes

* style: import

* fix: ArrayIndexOutOfBoundsException

* perf: resolved pr mentioned

* style: Add example

* style: sort codes

* perf: resolved PR mentioned

* style: depart to DamageUtils

* style: remove licence

* perf: depart to HealthUtils

* style: description

* style: to JetBrains annotation

* perf: resolved PR mentioned

* style: Suppress exactly; Add description

* perf: Deny using damage cause in old version

* perf: resolved PR mentioned

* perf: resolved PR mentioned

* perf: 更新 EffHealth.java

* style: sort code

* feat: test codes

* perf: test codes

* perf: check version

* fix: wrong health

* fix: test codes

* Update src/main/java/ch/njol/skript/bukkitutil/HealthUtils.java

* Fix tests (WIP on dev/feature)

---------

Co-authored-by: XPYEX <XPYEX0@163.com>
Co-authored-by: XPYEX <50171612+0XPYEX0@users.noreply.github.com>
Co-authored-by: sovdee <10354869+sovdeeth@users.noreply.github.com>
Co-authored-by: Efnilite <35348263+Efnilite@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Efnilite Efnilite Efnilite approved these changes

@Fusezion Fusezion Fusezion approved these changes

@cheeezburga cheeezburga cheeezburga approved these changes

@sovdeeth sovdeeth sovdeeth approved these changes

Assignees

@sovdeeth sovdeeth

@Efnilite Efnilite

Labels
enhancement Feature request, an issue about something that could be improved, or a PR improving something. help wanted Contributions are highly welcomed priority: medium Issues that are detrimental to user experience (prohibitive bugs or lack of useful implementation).
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@0XPYEX0 @cheeezburga @Efnilite @Moderocky @sovdeeth @Fusezion