McJtyMods / TheOneProbe

This simple mod adds a probe and an API for other mods to show information with that probe
MIT License
137 stars 68 forks source link

The One Probe crashes world when looking at Refined Storage drive. #215

Open ExpensiveKoala opened 6 years ago

ExpensiveKoala commented 6 years ago

Refined Storage issue here: https://github.com/raoulvdberge/refinedstorage/issues/1550

I wasn't too sure which one to report it to, so I did it to both.

jcmnn commented 6 years ago

Having this issue as well when looking at a Draconium Chest.

josephcsible commented 6 years ago

Versions?

jcmnn commented 6 years ago

1.12-1.4.23

pascal-aman commented 6 years ago

Same bug, but with filled morbs (mob pokeballs) from Thermal Expansion in a chest/strongbox. Version is also 1.12-1.4.23. Picking up the strongbox and then placing it causes a full client freeze.

Tora-B commented 6 years ago

This is a fundamental design problem of TOP. It's shipping way too much unnecessary data between the client and the server. Either the client should already have all the information it needs, and process it locally, or it should make a small request to the server. Instead, the client is packing up and shipping a ton of information to the server that the server should already have (Block NBT), making the server process it, and ship the information back. This is grossly irresponsible and inefficient.

McJty commented 6 years ago

It is not a fundamental design problem. This is possible to solve. The problem here is that it is trying to show the itemstacks in the container and these are big. However I could add an option to strip NBT from itemstacks before they are sent.

What you can do right now to solve this is to add the offending blocks to this list:

# A list of blocks for which we don't send NBT over the network. This is mostly useful for blocks that have HUGE NBT in their pickblock (itemstack) [default: ]
S:dontSendNBT <
 >
Tora-B commented 6 years ago

I didn't mean that the problem was unsolvable, just that it was a design problem, not a typo or simple logic error.

Why are you sending NBT to the server? It should already have it.

Either process the NBT on the client, or if you need information from the server, just tell it what information you need. If you need the NBT of a block, tell the server which block. The server shouldn't accept arbitrary block NBT like that, which is why it kicks clients that attempt to send large packets.

McJty commented 6 years ago

Problem is that I need to call getPickBlock() from the client since many mods/blocks (wrongly or not) assume that it only has to be implemented correctly client-side. So I call this client-side, send it to the server which can then process it and possibly send it back if required.

Tora-B commented 6 years ago

Many? Well... I can see it either way. On one hand, PickBlock depends on a raytrace, so it's kind of a client function. On the other, there shouldn't be a reason to implement it differently between the client and the server, and I'm curious what mods are not implementing it "correctly", and in what way.

Regardless, it's not clear why you're depending on it at all. Its primary purpose is to get an item containing all the information about the block. I can only guess the issue is that you don't handle blocks directly, and turning them into items makes things simpler. Again, either the client or the server should have all the necessary information, and if not, send the minimal amount of information from one to the other. If the information you need is already available on the client, then why are you making the server do the processing, rather than just processing it on the client?

In short, why is the server involved in this process at all? What do you need that the client doesn't already have?

McJty commented 6 years ago

Blocks don't always have enough information to show the correct information. There are many blocks that depend on tile entity data (and also NBT data in the corresponding itemblock) to see what they are. A good example of this is Draconic Ore which is a TE that stores the type of ore in the TE data

Tora-B commented 6 years ago

Well, yeah, lots of blocks have tile entities. And information about that tile entity is already on either the client or server, and often synchronized between the two in order to make the block function. It's not like getPickBlock is the only way to get an NBT representation of a block. Nor is it necessarily a good way to do so, because the item version of a block isn't guaranteed to contain all the state information of the block. Machines may drop their contents or intentionally lose their internal state, such as processing progress, when turned into an item, even with getPickBlock. Multiblocks and transportation grids have little to do with the items they're constructed out of, and none of that information will be preserved.

But again, even if you think there's useful information to be gotten with getPickBlock, why are you sending it to the server? Why are you doing client-oriented processing on the server, and not the client?

McJty commented 6 years ago

The entire concept in TOP is that the API for adding information to the tooltip is handled server-side. That makes it a LOT easier to support TOP for mod developers compared to WAILA. For example, a TOP handler for a tank with WAILA needs to have a packet to request that info from the server so that it can be displayed by the client-side waila handler. For TOP that is not required as all info is already available server-side so the TOP info handler can just fetch the data directly

Tora-B commented 6 years ago

I agree. All the information you need is already available server-side. So why are you gathering that information on the client and shipping it to the server, when it's already there?

McJty commented 6 years ago

Because I need the getPickBlock from the server. It is the only correct way to display the corresponding item (like you would display it as an item in the inventory). Given that I cannot consistently call getPickBlock() on the server to get that info I actually don't have a good way to (for all blocks) get the correct ItemStack to display. You are correct that all info that I should need to get that ItemStack is already on the server I still have no good way to actually construct that ItemStack. The only code that does that correctly is getPickBlock()

McJty commented 6 years ago

BTW. I went to a lot of iterations before coming to the solution that is currently implemented in TOP. It is unfortunatelly not that easy