Crystal-Nest / harvest-with-ease

Don't break crops, right click them!
https://modrinth.com/mod/harvest-with-ease
GNU General Public License v3.0
5 stars 1 forks source link

[Bug]May not compat with TFC in 1.18.2 #31

Closed Tollainmear closed 1 month ago

Tollainmear commented 7 months ago

Describe the bug right click on a full grown wheat plant with hoe, the wheat indeed be harvest, but the plant block was not be fully reseted, so you can see the info 'Mature' tag was still in HUD, and in the next random tick, the wheat just fully grown again.

To Reproduce Make sure the test env is TerraFirmaCraft 1.18.2-2.2.32 plant a seed and wait it be grown right click it with hoe you will see the info 'Mature' in HUD was not reset and just wait for the next random tick to make it update, the wheat was fully grown immediately

Expected behavior see my screen shot below.

Screenshots image

Stacktrace and logs If applicable, copy and paste relevant logs.

System information:

Additional context Terra firma craft

Tollainmear commented 7 months ago

Yep, here's additional info about this problem: I've removed every mod and just left these three mod:

And I found that you don't need to wait for a next random tick to prove this problem, just quit the world after harvesting then rejoin it, the plant which you just harvested was fully grown and the wheat you just obtained is also in your inventory.

Crystal-Spider commented 7 months ago

Most likely you need to open an issue with TFC and ask of them to add compatibility with HWE by using the provided API (I think they need to be using the AfterHarvest event and reset other properties other than the age).

Anyway, as soon as I can I will let you know more, thank you in the meantime!

Crystal-Spider commented 7 months ago

I confirm that to fix the bug it's needed for TFC to use HWE API.
If you open an issue with TFC, please link it here.

Tollainmear commented 7 months ago

I confirm that to fix the bug it's needed for TFC to use HWE API. If you open an issue with TFC, please link it here.

okay, thanks for your work, I'll make a report to tfc and paste link here later~

alcatrazEscapee commented 6 months ago

Frankly, given the description "works with all modded crops", the solution "the mod has to implement an API to handle checks notes a single right click" is not a solution. This mod runs into all sorts of issues trying to harvest TFC crops, including:

  1. When breaking a tall crop from the bottom, with a stick, it duplicates the stick, leaves it in place, and breaks the crop

2023-12-10_10 35 40

  1. When breaking a tall crop from the top with a stick, it drops nothing and just nukes the block:

2023-12-10_10 38 49

  1. It breaks the behavior of our crops that already have right click functionality (namely, picking without completely resetting the growth).
  2. All crops broken by this mod instantly regrow, because it just assumes that "set any property named age" is a sufficient replacement for "break the block and replant it". Which is is NOT.

Given all these issues, there's no way in hell I am introducing a soft dependency on this mod, subscribing to three separate bouncer events, preventing all manner of broken behavior. At best, I would like to disable this mod from working at all on TFC crops because it would be drastically easier to just implement right-click harvest within TFC. But this mod also seems to lack:

If harvest with ease is duplicating crops, sticks, in TFC, harvest with ease should be fixing it, or providing a way for the user to fix it. "Fix it yourself in your mod with our API" is not that.

Crystal-Spider commented 6 months ago

@alcatrazEscapee thank you for sharing your point of view. First of all you're right, so I will change "all modded crops" in "almost all modded crops".

Generally all these points can be easily solved by implementing HWE API: 1 & 2 can be solved by either preventing the harvest of your tall crops (RightClickHarvestCheck event) (and optionally implement your logic to handle right click that correctly harvests the crop) or changing the drops (HarvestDrops event) and the aftermath of the harvest (AfterHarvest event).
3 can be solved by preventing the harvest (RightClickHarvestCheck event).
4 can be solved by resetting the other properties added by TFC (AfterHarvest event).

As I see it, there should be no need for a blacklist option as no crop should have a reason not to work. If a mod needs to add/tweak specific behaviors, the API exists for this purpose. For a better user experience and consistency, I also think it's best if other mods are required to add a proper right-click harvest behavior rather than just saying "ignore these crops".

Crystal-Spider commented 1 month ago

@alcatrazEscapee I'm coming back to this since I'm refractoring the mod to employ a multiloader environment.
What do you suggest to make our mods compatible?
I am adding a configuration option to blacklist crops, so at least that's something that the end user can do.

What about ditching the events and instead exposing an interface (automatically applied to the base CropBlock class) with methods one can optionally override to tweak the harvest behavior?
This way it could be very easy to both override default behavior without duplicating logic, and also disable harvest for certain crops (for example overriding a method canHarvest and always return false).
It would still require an optional explicit dependency, but maybe it's better than subscribing to a few different events.

Let me know if you have any suggestion, thank you.

alcatrazEscapee commented 1 month ago

Exactly like I said above,

  • Any way for the user to blacklist certain crops (seriously this becomes a "implement the API in the target mod" issue?) No config?
  • Any soft dependency way, i.e. via a tag, to blacklist certain crops.

Add a config to blacklist crops; and a block tag to blacklist crops. Both are trivially implemented.

Crystal-Spider commented 1 month ago

Yeah, I am going to do both.

I was wondering, however, if there was a way that could make it easier for you, and thus other modders too, to add "proper" compatibility rather than just disabling the feature.

Crystal-Spider commented 1 month ago

Since v9.0.0.3-beta it's possible to add either a configuration option or a datapack to blacklist certain crops from interacting with this mod.
This can "solve" any incompatibility by blacklisting the crops that do not work.
Still, to add proper compatibility, the other mod would need to implement HWE events.

Closing this for now.