search

Darkhax-Minecraft / BotanyPots

Adds some flower pots that can be used to grow various crops.
GNU Lesser General Public License v2.1
102 stars 77 forks source link

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

Open MUYUTwilighter opened 3 months ago

MUYUTwilighter commented 3 months ago

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

Darkhax commented 1 month ago

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.

MUYUTwilighter commented 1 month ago

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).