Botany Pots

Botany Pots

Mods
33M Downloads

[BUG] Severe intermittent lag on client tick when run with BotanyPotsTiers

MUYUTwilighter opened this issue · 2 comments

commented

Minecraft Version

1.20.1

Mod Version

13.0.38

Mod Loader

Fabric

What environment are you running the mod in?

Client

Issue Description

I know it sounds like an issue of BotanyPotsTiers (abbr. BPT), but I managed to fix it with mixin on BotanyPots (abbr. BP).

Bookshelf keeps sending the ClientboundBlockEntityDataPacket for BlockEntityBotanyPot at intervals, and every time when client receive it, BlockEntityBotanyPot will always trying to execute findSoil and findCrop. These causes lag when more than 128 pots are placed in world and would exacerbate especially with pots from BPT.

This is a profiler report by Spark mod, recording only the interval lagging ticks.
https://spark.lucko.me/cp9xGcK8TG

However, these 2 methods is not always necessary to be executed, as the packet is regularly update without any modification. So I wrote a mod (not published on CurseForge nor other platform yet) to do some judgement before the load method is trying to invoke finding methods. And it seems to work fine on my modpack.

You can merge the code of my mod at your will.
https://github.com/MUYUTwilighter/BotanyPotsFix

commented

Hello, sorry for not replying sooner. I haven't been very active the last few weeks because I have been dealing with some personal stuff. This is a very interesting issue and I am surprised that I haven't noticed this in my own testing and profiling.

The fix you created looks okay for the most part, but I would want to validate the fix a bit more and look into why Bookshelf is sending so many update packets.

  • 👍1
commented

It would be much better if you can stop bookshelf from sending unnecessary packets, as it is actually the root cause.

But Bookshelf is kinda' lib mod, the interval BE data syncing might be necessary for some features (like syncing growth time in your mod). Additionally, it would cause too much effort to predict whether a BE should or shouldn't get updated for each client (That's why I do the check on client in my code).

Thus, my amature thought is that we'd better prevent syncing unnecessary data (like inventory in this case), instead of preventing bookshelf from sending packets.

What's more, I guess you didn't test out the problem because there aren't many soil & crop recipes in vanilla game, and your pot is not very quick to grow a crop (The later is just a guess, as I was using BotanyPotsTiers when I found this problem).

Lastly, I have posted the PR and I'm concerning is this issue no longer needed? as we could already talk about the problem is PR page. (It is totally fine if you leave this issue open until fixed, I am just having a notice).