Allow compatibility with custom fishing loot tables

[sisby-folk]

Right now, the mod runs a copied variant of the bobber retreive method to convert fish.

This PR replaces that with a mixin that just swaps the ItemEntity right before it would be spawned.

This means the mod doesn't have to reimplement loot table calculations, velocity, experience drops, etc.

By overriding less code, compatibility is improved. Previously, Sandwichable was incompatible. It has a feature where you can fish in a special liquid (brine) for a special loot table. Because the mod was calculating the loot table from scratch, this was lost - but with this PR:

https://github.com/ExpensiveKoala/Fishing-Real/assets/55819817/e78900e2-b963-4c4b-b4be-d92278de64b9

The mixin is used on both platforms, replacing the forge event bus call and generally making this part of the mod more brief.

[ExpensiveKoala]

Thank you for the PR! This seems good for fabric, but on forge, it's a bit different. Other mods, like Aquaculture implement their own FishingHook and for compatibility with those such mods I rely on them firing the forge item fished event.

Can you update this PR with this in mind? If not, I'll switch the mixing that fabric is using to this version once I can get to it.

[sisby-folk]

We had a good look into this!

Aquaculture 2 is visible source so we had a poke around and think we have a good understanding of the situation.

The compat offered by the forge event bus isn't perfect - the event cancel prevents things like bait consumption and double fish chance in aquaculture 2 from ever actually occurring, but it's serviceable for having the top level functionality of the two mods work together. We've made adjustments to the PR to bring back that previous level of compat.

We refactored out the mixin into two methods, which we use for both the common mixin and for the forge event bus method. If the event bus method runs, the mixin wont (no hazards there) - but we also skip the event bus method if we're confident the mixin will apply (i.e. the hook is a regular fishing hook), just so we get the compatibility benefit of the mixin in forge where it's relevant, too.

[ExpensiveKoala]

I've been looking at this and I don't see the benefit of having the mixin run on forge side as well. The mixin solves the issue of custom loot tables, but on forge side such issue was never present due to the fact that the event being passed the contents of that are being fished in the first place. Especially with how hacky the "useMixinInstead" part of the event handler looks.

boolean useMixinInstead = event.getHookEntity().getClass() == FishingHook.class;
try {
    useMixinInstead = event.getHookEntity().getClass().getMethod("m_37156_").getDeclaringClass() == FishingHook.class;
} catch (Exception ignored) {}

Is there a benefit to having the mixin apply on both fabric and forge instead of just on fabric?

The bait consumption and double fish chance were things I didn't even think of. I created an issue #19 to track this and I want to add more compat later down the line.

[sisby-folk]

The benefit on the forge side is identical to the benefit on the fabric side, which is better compatibility with other retrieve mixins.

A good example would be if Sandwichable was also an Architectury mod, then it's mixins would be running on forge - and this method would provide compatibility in this case.

The try/catch reflection to see if the retrieve method is overridden is maybe a bit overkill and can be cut to make it so the mixin only works on the vanilla fishing bobber (it doesn't make the mixin-level compatibility apply to aquaculture anyway) if desired.

[ExpensiveKoala]

If Sandwichable added forge compatibility, they should be compatible automatically because they modify the loottable, but not the actual retrieving code which on forge will fire the event.

Based on the design philosophy of forge, if a mod is modifying this code with mixins and breaks the event, or fails to fire the event when they should be, it would be a bug/issue on their code to fix. This reasoning is why I am not convinced on why the mixin needs to run on forge as well.

[sisby-folk]

Hmm, yeah that makes sense. I think there are definitely other cases where mixins to the retrieve method would fail without this guarded mixin method, but considering they're purely theoretical, how about we pull the mixin from the new forge implementation and we can always bring it back if we discover a reason to have it.

[ExpensiveKoala]

Sounds good. Thanks for working with me on this.

[sisby-folk]

done! no problem at all honestly - happy to fix up mods we use.

[sisby-folk]

any update on getting this out?

[ExpensiveKoala]

any update on getting this out?

Yes. Sorry. I've been busy with work. I will probably be able to get to this during this weekend or next weekend. I want to also work on updating it to 1.20+ at the same time. (I'll release a new version for 1.19 first)