FalsehoodMC / Fabrication

A huge collection of vanilla tweaks and small features for both Fabric and Forge.
https://www.curseforge.com/minecraft/mc-mods/fabrication
MIT License
112 stars 23 forks source link

[1.19.2] Turning on Weaponized Enderpearl in Forgery breaks Bumblezone Enderpearl dimension teleporting #574

Closed TelepathicGrunt closed 1 year ago

TelepathicGrunt commented 1 year ago

I am the developer of Bumblezone mod. In my mod, one way of entering the dimension is to throw an Enderpearl at a Beehive/Bee Nest, my mod detects this, disables the rest of the Enderpearl code, and teleports the player to the dimension.

However, a few different people reported issues where my mod's pearl teleportation was not working. One of them reported they had Forgery on and this setting set to true: image

After experimenting, Forgery is indeed killing my mod's teleportation entirely. This mixin that always cancels the rest of the behavior is the cause: https://github.com/unascribed/Fabrication/blob/ecc8f510fa431f88485037babd8ebf01a875de24/src/main/java/com/unascribed/fabrication/mixin/e_mechanics/weaponized_pearls/MixinEnderPearlEntity.java

Doing an inject mixin with a cancel that has is triggered almost always like this is a dangerous mixin. What is happening here is you're canceling the rest of the pearl's code including Forge's Enderpearl impact event that I rely upon in Forge. This is the event that does my behavior: https://github.com/TelepathicGrunt/Bumblezone/blob/latest-released-Forge/src/main/java/com/telepathicgrunt/the_bumblezone/entities/EnderpearlImpact.java

The patched vanilla code where Forge puts their event: image

My suggestion would be to remove this mixin entirely on Forge and use the Forge event with lowest priority so it runs last. That will make your mod most compatible with all other mods using this event and respect the other mod's canceling of the event if they wish. That or fire the Enderpearl impact event directly in your mixin and respect it's deny value if a mod sets the event result to deny the rest of the pearl's behavior

On Fabric, Fabrication does not break Bumblezone's pearl teleportation but that is because I use a pearl impact mixin at head which luckily, manages to avoid your mixin. https://github.com/TelepathicGrunt/Bumblezone/blob/1.19.2-Fabric/src/main/java/com/telepathicgrunt/the_bumblezone/mixin/items/EnderpearlImpactMixin.java

SFort commented 1 year ago

running the ender pearl event when the behaviour has been compleatly replaced is likely to cause other conflicts.

i'd suggest adding an alternative for entering the dimension maybe using an enderpearl on a beehive block?

this does bring up fabrication needing an update when it comes to feature failiure.

@unascribed might wanna chime in maybe an API, something along the lines of FabConfAPI.addFail("*.weaponized_pearls", "Weaponized pears is incomatible with bumblezone ")? it would help with fabrication specifying why a feature is off in the config screen at least (which i feel like it used todo more accuratley) or maybe addFail("*.weaponized_pears", "wp is incomaptibel...", "bumble1") to allow fabrication adding compat down the line using the bumble1 field as a sortof id for the issue? i don't know haven't put much thought into it

unascribed commented 1 year ago

Does this feature seriously need to be implemented as a complete hack override like this?

SFort commented 1 year ago

it compleatly replaces teleportation, don't see a reliable way to compleatly replace a mechanic, and keep support for things that depend on it

TelepathicGrunt commented 1 year ago

@SFort Respectfully, I disagree on firing the Forge event yourself causing issues. In fact, I would argue the current skipping of the Forge event is causing far more issues than any of us realizes due to how many mods relies on that event for various behaviors. I am not going to add a right-click workaround just for a single mod that is doing an unconditional inject cancel which is heavily discouraged even on Fabric due to silent mod conflicts like this.

Usages of the event that I can find https://sourcegraph.com/search?q=context:global+EntityTeleportEvent.EnderPearl&patternType=standard&sm=1

Notable conflicts that I can see with a light search:

Nomadic tents uses the event to prevent Enderpearl teleporting in their tents which they must have a good reason for doing so: https://sourcegraph.com/github.com/skyjay1/Nomadic-Tents/-/blob/src/main/java/nomadictents/event/NTEvents.java

Rftoolsutility uses the event to prevent pearl teleporting in certain arenas to prevent cheesing it I am guessing: https://sourcegraph.com/github.com/McJtyMods/RFToolsUtility/-/blob/src/main/java/mcjty/rftoolsutility/setup/ForgeEventHandlers.java

Dimensionalpocketsii mod uses event to prevent people from teleporting with the pearl in their dimensional pockets (probably to prevent spamming to go through the ceiling): https://sourcegraph.com/github.com/TheCosmicNebula/DimensionalPocketsII/-/blob/src/main/java/com/tcn/dimensionalpocketsii/pocket/core/management/PocketEventManager.java

Primalmagick has an effect that is suppose to stop all teleporting and that include the pearl's: https://sourcegraph.com/github.com/daedalus4096/PrimalMagick/-/blob/src/main/java/com/verdantartifice/primalmagick/common/events/EntityEvents.java

And that's just a small selection of the mods there. All of them are broken due to this mixin.

If you must keep the mixin, I would recommend MixinExtra mod (When shading it on Forge, be sure to 'relocate' the shadowed jar so it won't blow up with other people also shadowing MixinExtra) https://github.com/LlamaLad7/MixinExtras https://github.com/TelepathicGrunt/Bumblezone/blob/4b08bf22dfffa9a3d861fd5b13531cdb7d6fbbdb/build.gradle#L172

Then you can use WrapOperation annotation to disable the endermite spawning and other changes/disabling more easier and regular non-canceling injections where needed. And by targeting directly below the Forge event, other mod's canceling of the event will still work and your change is directly where it needs to be to work https://github.com/LlamaLad7/MixinExtras/wiki/WrapOperation

SFort commented 1 year ago

All of them are broken due to this mixin.

...? none of those are broken. the mods you just listed disable teleportation. nothing breaks if a mechanic that no longer exists is being disabled.

the problem is that fabrication removes a mechanic, that your mod is using. which is absolutley my problem and my fault, but it's a issue caused intentionally in case a mod ware to re-imagine ender pearl teleportation.

this is as if fabrication added a feature "*.disable_ender_pearls" and you complained that it broke your mod. which i mean is fair it would do that.

i suggested an alternative not to shift blame but as a easy workaround for something that doesn't have a simple solution.

thinking about it a failure API is a terrible idea and this is the perfect example a failiure api would be usefull for something like grindstone disenchanting where a mod replaces the functionality. but something like this where one or the other feature should work needs to be decided by the user. as a practical example lets say fabrication fails this feature when bumblezone is installed. if a user wants to use weaponized_pearls and add a alternative way of getting into the bumblezone dim they're sol

weaponized pears should just be turned off by the user in this case

TelepathicGrunt commented 1 year ago

I was throwing the Enderpearl too close and was being pulled in so I thought this was still doing teleporting but smoothed out (some mods try to smooth out teleporting so I thought that this was that). I didn't actually throw it far enough away to see teleporting was actually entirely skipped.

I looked around and found there is a Forge ProjectileImpactEvent that fires long before your mixin so I will switch to that to get ahead of your mod's behavior and still teleport into my dimension.