Skip to content

Damage with any cause#7423

Closed
TetraTheta wants to merge 0 commit into
PaperMC:masterfrom
TetraTheta:master
Closed

Damage with any cause#7423
TetraTheta wants to merge 0 commit into
PaperMC:masterfrom
TetraTheta:master

Conversation

@TetraTheta
Copy link
Copy Markdown
Contributor

Continuation of #5191. This will resolve #7415.

Even though I tried to mimic (and enhance?) the original PR, since this is my first trial of contributing to Paper, there should be some error present.

If this patch needs patch note, tell me and I will try to add information it might need as lot as possible I can think of.

Any criticism is welcomed.

@TetraTheta TetraTheta requested a review from a team as a code owner January 30, 2022 10:52
Comment thread patches/api/0364-Damage-with-any-cause.patch Outdated
Comment thread patches/server/0860-Damage-with-any-cause.patch Outdated
Comment thread patches/server/0860-Damage-with-any-cause.patch Outdated
Comment thread patches/server/0860-Damage-with-any-cause.patch Outdated
Comment thread patches/server/0860-Damage-with-any-cause.patch Outdated
@NeumimTo
Copy link
Copy Markdown

nice to see that somebody picked it up after me.

https://github.com/NeumimTo/paperprtests
here i made simple test plugin for my former pr.

Should be still useful.

@NeumimTo
Copy link
Copy Markdown

Maybe it would be also worth to discuss whenever it would be better instead of

void damage(double amount, @NotNull DamageCause damageCause, @Nullable Entity source);

create a damage builder

void damage(DamageBuilder damageBuilder);
class DamageBuilder {
   double damage;
   Entity source;
   DamageCause cause;
   ...
}

In future, if somebody would need to add an additional arguments to damage the method they would not need to create another overloaded damage method in damageable interface, but only add a new field/method to damagebuilder.

@TetraTheta
Copy link
Copy Markdown
Contributor Author

Anyone insterested in this PR, please reply your opinion of this opinion.
Because, I think both current and suggested way are good, so I cannot decide what to choose.

}

public boolean hurt(DamageSource source, float amount) {
+ // Paper start
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why does this change exist? You never seem to add a call to your new three argument version of hurt.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is used at CraftEnderDragonPart which implements EnderDragonPart which extends Damagable which extends Entity.

// Paper start
@Override
public void damage(double amount, DamageCause cause, Entity source) {
    getParent().damage(amount, cause, source);
}
// Paper end

It is used to make EnderDragonPart to be damaged with specific source(entity).

That method is also used at CraftLivingEntity too.

I hope I understood the concept of the original PR(#5191)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

That's CraftEntity.damage(), not Entity.hurt(). The former calls the two argument version of the latter. I think either the three argument version isn't supposed to exist or that was where it was supposed to get called although the third argument doesn't appear to do anything so I'm leaning toward shouldn't exist.

Copy link
Copy Markdown
Contributor Author

@TetraTheta TetraTheta Mar 1, 2022

Choose a reason for hiding this comment

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

If I understood the definition of the abstract class correctly, it is somewhat hybrid of interface and actual class, thus making actual method (not structure only) can be inherited via implementation.
CraftEntity doesn't override damage() from Entity, and since Entity is abstract class, full Entity#damage() will be inherited to CraftEntity.

Please correct me if I am wrong.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The hurt method hurt(DamageSource, float) was referenced from some non spigot code, which i simply did not want to refactor in my original pr - it was not even needed.

Therefore i only overloaded the method and made the old method call new overloaded method with null argument, that way the pr do not have to do that many changes all over the place

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

pr do not have to do that many changes all over the place

Well, that makes me kind of confused. When I first look at the original PR, I thought the codes were nice and clean, no clutterness present, no need to refactor the whole logic of the PR... In this case, I don't have any idea of 'how to make these codes cleaner'.

Any advise is welcomed. I must be misunderstood something wrong.

@TetraTheta TetraTheta closed this Mar 1, 2022
@TetraTheta
Copy link
Copy Markdown
Contributor Author

Oops, I closed PR by mistake. I will open new PR with some... improvement later.
Next time, I should really open new PR in my own branch rather than master...

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Provide cleaner way to damage/kill Player

4 participants