Skip to content

Wrap properly damage source and deprecate Bukkit's lazy enum#8058

Closed
Lulu13022002 wants to merge 1 commit into
PaperMC:feature/registry-modificationfrom
Lulu13022002:fix/damagesource
Closed

Wrap properly damage source and deprecate Bukkit's lazy enum#8058
Lulu13022002 wants to merge 1 commit into
PaperMC:feature/registry-modificationfrom
Lulu13022002:fix/damagesource

Conversation

@Lulu13022002
Copy link
Copy Markdown
Contributor

@Lulu13022002 Lulu13022002 commented Jun 27, 2022

Closes #7415
Supersedes #7980

Bukkit has for years stored damage history into a single enum which loses 90% of available damage history and cause a bunch of issue about the future (when Mojang decide to add new damage source for example)

This PR wrap the whole nms DamageSource.

The main reasons of this changes

  • Parity with vanilla (Bukkit lost some cause and create cause that doesn't exists in vanilla) (see supersedes PR javadoc)
  • Maintainability (Bukkit has a single method with over 100 lines just to determine the right cause)
  • API (bukkit has no API to create custom damage source (different from vanilla) or just to apply a certain cause to an entity)

New API
With this PR you can create your own damage source using a builder
Example:

DamageOrigin origin = DamageOrigin.of(VanillaDamageType.SONIC_BOOM).scalesWithDifficulty(true).build();

entity.damage(50.5D, origin);
entity.damage(23.1D, DamageOrigins.lava());
entity.damage(12.1D, DamageOrigins.sweetBerryBush(), EventContext.fromBlock(blockPos));
// some damage origin would normally be linked to a block in order to be compatible with bukkit events

DamageOrigin originComplex = DamageOrigin.of(type).deathMessage(killed -> Component.translatable("translatableKey", killed.getName())).build();

All damage origin must be categorized under a damage type and you can
create them in the methods called before the registries freeze in paper plugin
bootstrapper. Example:

final LifecycleEventManager<BootstrapContext> lifecycleManager = context.getLifecycleManager();

lifecycleManager.registerEventHandler(RegistryEvents.DAMAGE_TYPE.preFreeze(), event -> {
    event.registry().register(NEW_DAMAGE_TYPE, builder -> {
        builder.name("my_damage").foodExhaustion(5.0F).deathMessageFormat(DeathMessageFormat.FALL_VARIANTS).effects(DamageEffect.FREEZING).scale(DamageScale.ALWAYS);
    });
});

The builder is already prefilled with the default values.
The name will be used as part of the translatable key when an entity died due to this damage cause
If you want to change the death message without using the default behaviour (using the damage type name as key) you needs to use a callback function in the damage origin builder.

All the damage types (including plugin/datapack made one) will be under the DAMAGE_TYPE registry
available with RegistryAccess.registryAccess().getRegistry(RegistryKey.DAMAGE_TYPE)
Note this registry will not be available during bootstrap so don't try to get the value found in the VanillaDamageTypes class to get their keys! Instead you can get them through the DamageTypeKeys class generated automatically. For now the damage type tags aren't supported yet by this task and are left untyped.

You can also get the hurting sound or if an entity is invulnerable to a specific damage origin and to create explosion with custom origin. The damage type tags are also available

boolean invulnerable = entity.isInvulnerableTo(origin);
Sound sound = entity.getHurtSound(origin);
world.createExplosion(null, position.offset(0, 2, 0), 2, false, true, origin);
boolean isFireDamage = origin.isTagged(DamageTypeTags.IS_FIRE)

Additionally the damage origin is now available into the EntityDeathEvent, PlayerDeathEvent, VehicleDamageEvent, VehicleDestroyEvent, PlayerAttackEntityCooldownResetEvent, EntityResurrectEvent and TameableDeathMessageEvent

However i have decided to not change how the events works basically only the cause changes but the same events will be called for the same entity/block/action
In the future we could probably get rid of the bukkit logic that enforce such damage to be a block damage or not and
that has issue with concurrency.

Credits:
Javadoc for bukkit api has been taken from linked PR

Copy link
Copy Markdown
Member

@Owen1212055 Owen1212055 left a comment

Choose a reason for hiding this comment

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

I have been working on a very similar api, and i'm fine if you would like to take any inspiration for how I handled this api here.
Owen1212055@9d78b6e#diff-d0659f5a1063f530efebfa22bb1dbbcd7918630c558ae382821988a5fbb8a38f

I'd really recommend expanding this api to allow you to "build" your own damage sources. Currently you allow this but it is locked behind an nms class, you need to expose it via the api.
See: https://canary.discord.com/channels/289587909051416579/925530366192779286/987522312674897940

Basically, you want api objects to be backed by an NMS damage source which can be used in things like damage methods. My current iteration allows you to simply implement the interface, but that should no longer be the case and it should only be possible to create sources via builders.
You currently also remove the damage cause api, you cannot do that and have to instead have backward compatibility for that old system. It should be noted that many damage cause do not have a corrisponding NMS damage source.

Comment thread patches/api/0387-Add-damage-source-wrapper.patch Outdated
Comment thread patches/api/0387-Add-damage-source-wrapper.patch Outdated
Comment thread patches/api/0387-Add-damage-source-wrapper.patch Outdated
Comment thread patches/server/0918-Add-damage-source-wrapper.patch Outdated
Comment thread patches/api/0387-Add-damage-source-wrapper.patch Outdated
Comment thread patches/api/0387-Add-damage-source-wrapper.patch Outdated
Comment thread patches/server/0918-Add-damage-source-wrapper.patch Outdated
Comment thread patches/server/0918-Add-damage-source-wrapper.patch Outdated
@Lulu13022002
Copy link
Copy Markdown
Contributor Author

Lulu13022002 commented Jun 28, 2022

I have updated the PR with the following things:

  • Keep the old damage cause behavior
  • Move the builders and the static helpers in the API
  • Fallback to a default translatable component in case of a null function for death message
  • Add some utilities methods to check if a damage origin is similiar to another and some string representation
  • Removed the damage attribute cause even if vanilla doesn't use multiple things (except for fireball: a projectile and fire damage),
    Mojang can adds a new damage at any time so i have just decided for now to lock certain damage type excluding fire and projectile cause
  • Change the internal api logic with how you handle stuff in your branch using unsafe bukkit and abstract class to cast the builders
    - Avoid a backed nms builder and override the interface instead like you did

Comment thread patches/api/0387-Add-damage-source-wrapper.patch Outdated
Comment thread patches/api/0387-Add-damage-source-wrapper.patch Outdated
Comment thread patches/server/0918-Add-damage-source-wrapper.patch Outdated
Comment thread patches/server/0918-Add-damage-source-wrapper.patch Outdated
Comment thread patches/server/0918-Add-damage-source-wrapper.patch Outdated
Comment thread patches/api/0387-Add-damage-source-wrapper.patch Outdated
Comment thread patches/api/0387-Add-damage-source-wrapper.patch Outdated
Comment thread patches/api/0387-Add-damage-source-wrapper.patch Outdated
Comment thread patches/api/0387-Add-damage-source-wrapper.patch Outdated
Comment thread patches/api/0387-Add-damage-source-wrapper.patch Outdated
Copy link
Copy Markdown
Member

@Owen1212055 Owen1212055 left a comment

Choose a reason for hiding this comment

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

Getting closer here! Good work.

Comment thread patches/api/0387-Add-damage-source-wrapper.patch Outdated
Comment thread patches/api/0387-Add-damage-source-wrapper.patch Outdated
Comment thread patches/api/0387-Add-damage-source-wrapper.patch Outdated
Comment thread patches/server/0918-Add-damage-source-wrapper.patch Outdated
Comment thread patches/server/0918-Add-damage-source-wrapper.patch Outdated
Comment thread patches/server/0918-Add-damage-source-wrapper.patch Outdated
Comment thread patches/server/0918-Add-damage-source-wrapper.patch Outdated
Comment thread patches/server/0918-Add-damage-source-wrapper.patch Outdated
Comment thread patches/server/0918-Add-damage-source-wrapper.patch Outdated
Comment thread patches/api/0387-Add-damage-source-wrapper.patch Outdated
@Owen1212055
Copy link
Copy Markdown
Member

image
Also, please resolve the conversations to help clear some of the old requests.

@NeumimTo
Copy link
Copy Markdown

NeumimTo commented Aug 6, 2022

Once/if merged what will be its target version?

@Lulu13022002
Copy link
Copy Markdown
Contributor Author

I think i will target the latest version

@Lulu13022002 Lulu13022002 force-pushed the fix/damagesource branch 3 times, most recently from 024b857 to 87e26d3 Compare August 12, 2022 10:13
@Lulu13022002 Lulu13022002 force-pushed the fix/damagesource branch 3 times, most recently from 9166429 to 0361fee Compare October 20, 2022 15:43
@Lulu13022002
Copy link
Copy Markdown
Contributor Author

Rebased and added more example in the initial comment for anyone interested. Also now the plugin API can't trigger issue like in the 8340, the incompatible association will now throw a sweet error in a precondition (only for the vanilla damage, the custom one made by builder can already break this limit). Only the plugin that abuse of the nms hurt method could or if paper/bukkit hasn't been updated properly when mc is updated. The test is now more simple and will not fail to static init the vanilla damage origin class like before (i don't even know how the build was green??).

Copy link
Copy Markdown
Contributor

@lynxplay lynxplay left a comment

Choose a reason for hiding this comment

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

Initial review of the API.
Mostly focused on the javadocs, the type layout looks fine to me 👍

I'll have some more time next week to also review the impl.

Comment thread patches/api/0404-Add-damage-source-wrapper.patch Outdated
Comment thread patches/api/0404-Add-damage-source-wrapper.patch Outdated
Comment thread patches/api/0404-Add-damage-source-wrapper.patch Outdated
Comment thread patches/api/0404-Add-damage-source-wrapper.patch Outdated
Comment thread patches/api/0404-Add-damage-source-wrapper.patch Outdated
Comment thread patches/api/0404-Add-damage-source-wrapper.patch Outdated
Comment thread patches/api/0404-Add-damage-source-wrapper.patch Outdated
Comment thread patches/api/0404-Add-damage-source-wrapper.patch Outdated
Comment thread patches/api/0404-Add-damage-source-wrapper.patch Outdated
Comment thread patches/api/0404-Add-damage-source-wrapper.patch Outdated
@Lulu13022002 Lulu13022002 force-pushed the fix/damagesource branch 7 times, most recently from bee13f4 to a7f563c Compare October 25, 2022 12:35
@Lulu13022002
Copy link
Copy Markdown
Contributor Author

Lulu13022002 commented Dec 14, 2022

Rebased for 1.19.3
Mojang while fixing of https://bugs.mojang.com/browse/MC-200006 has introduced a new type of damage source, the PointDamageSource. This is a generic damage source with the source position of the damage in order to check if the target is well covered holding a shield or not. I have added the api to support this, you could for example now specify from which position a damage has been dealt to support shield covering for all non entities things (so blocks, tile entity, particle, fake entity etc...). In vanilla, this is used for bed/respawn anchor explosion. This also allow you to change the source position of the entity damage origin as well by default it use the direct attacker position. But you could change this if your custom entity shot particle as projectile for example... The main advantage using this over a rewrite of the logic in the plugin is a better compatibility for others plugins (i.e: the damage modifier api would catch this change). For the entity damage origin, the source position is nullable to be able to ignore the shield at all but still hurt the armor of the entity. The bypass armor flag has the priority over this new feature.
For example to make more realistic cactus that will not hit you if you're covered you could do in the EntityDamageByBlockEvent somethings like:

if (e.getOrigin().getType() == VanillaDamageType.CACTUS) {
    if (e.getEntity() instanceof LivingEntity lv) {
        e.setCancelled(true);
        lv.damage(3, DamageOrigin.of(VanillaDamageType.CACTUS, e.getDamager().getLocation()).build());
        // cactus damage are 1 point but this is just an example to show you that you need at least 3 point of damage
        // if you want to hurt the shield otherwise the damage will always be blocked if well covered
        lv.setNoDamageTicks(2 + (lv.getMaximumNoDamageTicks() / 2));
    }
}

There's also some changes for all the falling block derivated damage origin. They're now a EntityDamageOrigin (the entity will be the falling block before the fall). You should use the VanillaDamageOrigin preset if you don't know what to put in the builder.

@Lulu13022002 Lulu13022002 changed the base branch from master to feature/registry-modification May 28, 2023 17:04
@Machine-Maker Machine-Maker force-pushed the feature/registry-modification branch 2 times, most recently from 6baf31a to ab7f541 Compare June 9, 2023 20:03
@Machine-Maker Machine-Maker force-pushed the feature/registry-modification branch from ab7f541 to 6bee6ce Compare July 16, 2023 21:36
@Machine-Maker Machine-Maker force-pushed the feature/registry-modification branch from 6bee6ce to ee1c66e Compare July 17, 2023 20:21
@Lulu13022002 Lulu13022002 changed the base branch from feature/registry-modification to master October 31, 2023 18:11
@Lulu13022002 Lulu13022002 changed the base branch from master to feature/registry-modification November 16, 2023 19:39
@Machine-Maker Machine-Maker force-pushed the feature/registry-modification branch 2 times, most recently from e5fca8b to 27dd551 Compare November 23, 2023 05:47
@Machine-Maker Machine-Maker force-pushed the feature/registry-modification branch from 27dd551 to 2c627bc Compare December 18, 2023 04:21
@Lulu13022002 Lulu13022002 force-pushed the fix/damagesource branch 2 times, most recently from 85e9944 to fba3767 Compare December 18, 2023 18:41
@marcbal
Copy link
Copy Markdown
Contributor

marcbal commented Jan 2, 2024

It looks like there is nothing more to do for this PR. Most of the recent commits are rebases for newer MC version. What prevents it from being ready for review / unmarked as draft?

Really hope to see this soon added into paper :)

@Owen1212055
Copy link
Copy Markdown
Member

Owen1212055 commented Jan 2, 2024

spigot has an upstream PR, but also this PR is waiting for some other PRs currently in review.

@Machine-Maker Machine-Maker force-pushed the feature/registry-modification branch from 2c627bc to 26a19a5 Compare January 9, 2024 01:56
@Machine-Maker Machine-Maker force-pushed the feature/registry-modification branch 2 times, most recently from 1fe80ff to 956b5ef Compare February 9, 2024 20:19
Closes issue 7415
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Closed

Development

Successfully merging this pull request may close these issues.

Provide cleaner way to damage/kill Player

5 participants