Project MMO

Project MMO

10M Downloads

Frost Walking grants XP when it should not

harlanw opened this issue ยท 6 comments

commented

Describe the bug
On pmmo-1.19.2-3.1.0, wearing boots enchanted with Frost Walker I, II, or gaining this effect from another source gives building experience with the default config. Destroying the ice also gives excavation experience but maybe that is working as intended.

Expected behavior
This should not reward experience since you are not building anything.

Additional Comment
I also noticed that it appears experience might be applied even when the experience-earning event is cancelled. Not sure if this is a priority issue, race condition, or an issue with the other plugin but I thought I'd bring it up since I am here. While using YAWP with the walker-freeze flag enabled, moving near the edge of water, but still on solid ground grants experience, even though the event is cancelled.

commented

The building experience is intended. You can override this configuration though.

As for the event, how are you certain the event is cancelled? PMMO works exclusively on events and all of our events work on the lowest priority. This means pmmo doesn't execute any behavior until all other mods have done what they want with the event. This means, if you are getting building xp, the event is not in fact cancelled. This might be an edge case for YAWP.

commented

I'll have to lean on you a bit for the second part since I am not a modder.

It looks like the frost walker flag is loaded through a mixin and, as far as I can tell, the default priority[0][1] for mixins is 1000 which I am assuming is the lowest[2]? Not sure how that maps back to Forge's event priority system. YAWP regions also have a concept of priority[3] but as far as I can tell that looks unrelated.

I assumed the plugins might be fighting over being the last plugin to run. I hope you can fill me in. Stranger yet, I tested digging and building in a protected region and I found it does not grant experience (desired). This event is handled through the normal event system[4]. Maybe its an issue around mixins?

Is there a substantial difference between CallbackInfo::cancel() and event.setCancelled(true)[5]?

[0] org/spongepowered/asm/mixin/extensibility/IEnvironmentTokenProvider.java#L37
[1] yawp.mixins.json
[2] net/minecraftforge/eventbus/api/EventPriority.java
[3] de/z0rdak/yawp/commands/RegionCommands.java#L615

[4] de/z0rdak/yawp/handler/flags/PlayerFlagHandler.java#L324
[5] de/z0rdak/yawp/mixin/FrostWalkerEnchantmentMixin.java

commented

[0][1][2] Mixin priority is unrelated to event priority. The former relates to the order in which mixins are applied (which all happens during mod loading, before you get to the main menu). the latter is the sequence in which event listeners execute (which occurs each time an event triggers during gameplay).
[3] i'll take your word on it
[4] my guess is there is a use case with the wrapper that is not being handled as expected.
[5] CallbackInfo::cancel() is mixin speak for return;. event.setCancelled(true) changes the event flag so that when all listeners have finished executing the event system knows whether to proceed with the code it was created from in vanilla or not.

commented

Ok. I see now that YAWP isn't hooking directly into "Frost Walker happening," and instead has some kind of wrapper. I tried to decipher how this works but no luck. I'm going to open an issue there to figure out the rest. Thanks!

commented

That makes a lot of sense. thanks for letting me know.

commented

Hey there @Caltinor and @harlanw
Z0rdak here, the dev of YAWP.

  • [3] Agreed.
  • [5] Agreed.

The mixin is hooking after the forge event. It does prevent the block from being placed (given that the flag is set), but does not mark the event as canceled, thus having not the desired effect in your scenario.

I implemented this particular flag first in the Fabric port. Since there is no event bus system in Fabric I used a mixin for this. And I use the same mixin in Forge and only NOW have I realized that forge has put a hook for place block there as well.

Long story short:
I'll remove the mixin for this flag in the Forge port and replace it with the Forge event. This way it should be compatible.

Best regards
Z0rdak