Skip to content
This repository was archived by the owner on Jun 3, 2024. It is now read-only.
This repository was archived by the owner on Jun 3, 2024. It is now read-only.

Forge distinguishes between "onLivingAttack" and "onPlayerAttack" while patchwork doesn't. #114

Open
Open
Forge distinguishes between "onLivingAttack" and "onPlayerAttack" while patchwork doesn't.#114

Description

@kitlith
kitlith
opened on Jul 9, 2020

YarnForge's ForgeHooks:

        public static boolean onLivingAttack(LivingEntity entity, DamageSource src, float amount) {
                return entity instanceof PlayerEntity || !MinecraftForge.EVENT_BUS.post(new LivingAttackEvent(entity, src, amount));
        }

        public static boolean onPlayerAttack(LivingEntity entity, DamageSource src, float amount) {
                return !MinecraftForge.EVENT_BUS.post(new LivingAttackEvent(entity, src, amount));
        }

Patchwork's EntityEvents:

	public static boolean onLivingAttack(LivingEntity entity, DamageSource src, float damage) {
		return MinecraftForge.EVENT_BUS.post(new LivingAttackEvent(entity, src, damage));
	}

The LivingEntity mixin ends up not checking if the entity is instanceof PlayerEntity while forge does. For all I know there's a reason for this, but it's not documented if there is a reason.

Activity

  1. obj-obj commented on Jul 27, 2020

    @obj-obj
    obj-obj
    on Jul 27, 2020

    Do you have an example of a mod that requires this event?

    I think I fixed it, but I need to test somehow.

  2. kitlith commented on Jul 28, 2020

    @kitlith
    kitlith
    on Jul 28, 2020
    ContributorAuthor

    To be clear: The reason I opened this issue was because I saw it while implementing the god-classes module, which raised some questions that were not on scope for those PRs. It's my fault for not laying out those questions:

    1. Why does Forge do this?
    2. Is there a reason why Patchwork doesn't do this? (thus the issue)

    If the first question has a reasonable answer, and the answer to the second question is "no", then fixing it certainly makes sense.

    The god-classes module is unaffected either way as I simply added the instanceof check to the relevant method in ForgeHooks.

    (this comment brought to you by: discussion of whether a fix for this issue is actually needed or not)

  3. obj-obj commented on Jul 30, 2020

    @obj-obj
    obj-obj
    on Jul 30, 2020

    After learning a bit about forge's event system and looking at it again, I don't actually think this issue actually does need to be fixed, as entity attacks just break when you try to, and it's already implemented in ForgeHooks anyway.

  4. kitlith commented on Jul 30, 2020

    @kitlith
    kitlith
    on Jul 30, 2020
    ContributorAuthor

    @obj-obj I know exactly why your attempt is breaking, I've even told you exactly why it's breaking on the PR. That is irrelevant to whether this issue should or should not be fixed.

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions