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

Open
wants to merge 29 commits into
base: dev/feature
Choose a base branch
from
Open

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

wants to merge 29 commits into from

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

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
@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
Copy link
Contributor Author

0XPYEX0 commented Sep 10, 2024

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

@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
Efnilite
Efnilite approved these changes Sep 10, 2024
View reviewed changes
Copy link
Member

@Efnilite Efnilite left a comment

Choose a reason for hiding this comment

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

nice

src/main/java/ch/njol/skript/effects/EffHealth.java Outdated Show resolved Hide resolved
Fusezion
Fusezion approved these changes Sep 10, 2024
View reviewed changes
Copy link
Contributor

@Fusezion Fusezion left a comment

Choose a reason for hiding this comment

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

Just awaiting test, but for now I can approve. If you're unsure how to make test let someone know and we can help explain it.

sovdeeth
sovdeeth requested changes Sep 10, 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.

needs some tests but looks good

@0XPYEX0
Copy link
Contributor Author

0XPYEX0 commented Sep 11, 2024

What should I do for test？😂I don know it actually

@Efnilite
Copy link
Member

What should I do for test？😂I don know it actually

you should spawn an entity and assert that a bunch of normal and edge cases work as you expect. you can check out examples

cheeezburga
cheeezburga reviewed Sep 11, 2024
View reviewed changes
src/test/skript/tests/syntaxes/effects/EffHealth.sk Outdated
Comment on lines 23 to 25
wait 1 tick
damage {_m} by 2 hearts with damage cause fall
assert health of {_m} is 3 with "damage cow failed"
Copy link
Member

Choose a reason for hiding this comment

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

You can't wait in tests. Also, this will throw an error on MC servers running an older version than 1.20.4.

Copy link
Contributor Author

@0XPYEX0 0XPYEX0 Sep 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We must wait at least 1 tick, or assert exception, because:
image
image
It just marked the entity to be damaged, the damage will be delayed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And there is something wrong with parse if section:
image
image

I can only upload the parse if version and wait for it to be fixed

Copy link
Member

Choose a reason for hiding this comment

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

You cannot wait, because anything after the wait will run after the test runner is finished, essentially making it useless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You cannot wait, because anything after the wait will run after the test runner is finished, essentially making it useless.

Yes, so I removed it. But idk how to fix the mark problem, bcs the damage is delayed

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fusezion Fusezion Fusezion approved these changes

@cheeezburga cheeezburga cheeezburga approved these changes

@Efnilite Efnilite Efnilite approved these changes

@sovdeeth sovdeeth sovdeeth approved these changes

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
enhancement Feature request, an issue about something that could be improved, or a PR improving something.
Projects
None yet
Milestone
No milestone
Development

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