Antique Atlas

Antique Atlas

32M Downloads

Fix the packet system

Hunternif opened this issue · 23 comments

commented

Right now atlas data is being sent in a single packet via compressed NBT tag. This is stupid and dangerous, because atlas size may grow indefinitely.

  • Don't send full NBT tag during synchronization.
  • Make sure large atlases can be sent via multiple packets
commented

I saw an image someone else had in their mod: java.lang.RuntimeException: Tried to read NBT tag that was too big; tried to allocate: 2097156bytes where max allowed: 2097152

So the map really will have to be split up into smaller nbt packets. apex2top's message says: java.lang.IllegalArgumentException: Payload may not be larger than 1048576 bytes, probably in reference to hunternif.mc.atlas.network.client.MapDataPacket, so splitting network packets really would help, but only until the map doubled again.

I must remember to have backwards compatible loading for seamless upgrading.

commented

AFAIR in the beginning I sent map data via fixed-size packets, by
manually serializing all the tile coordinates into the buffer. And the only reason I switched to NBT was simply that I discovered this pre-existing method for sending NBT data, and it nicely decoupled the serialization code from the mod codebase. The switch was NOT a very good idea. @Kenkron, you don't have to stick to NBT if you see better ways to organize the data.

commented

I think sticking to NBT, but just breaking it down is a good idea. One of the implications of that note above is that, unless the NBT tag is broken up too, the maps will stop working when too large (about twice as large as the networking allows).

I've got a packet rework branch. It feels slow going, but I seem to be picking up speed as I learn more.

commented

@Kenkron, I made an edit-typo in my comment: my switching to NBT was definitely NOT a good idea :)
Also, please check your inbox on Minecraft Forums.

commented

oh, okay... I'll have to reconsider then. I think the NBT may still need to be changed to avoid the the 2M limit, but I'll go check the minecraft-forum page.

commented

Sorry for the confusion. I mean, I definitely made a mistake when I switched to NBT without considering packet size limit, which allowed this issue to arise. Your idea about breaking up the NBT will definitely work. I think it's a good way to solve this issue.

commented

Oh, sweet. Then I can keep at it.

commented
commented

According to code - https://github.com/gamerforEA/Minecraft-PacketUnlimiter/blob/master/src/main/java/com/gamerforea/packetunlimiter/asm/PacketBufferPatch.java

  • it splits oversized packet to 2 smaller packets with additional integer (istead short) and byte array.

I'm not tested it yet with AA, but it resolve my problem with oversized packet from other mod.

commented

Sry, previous message accedenctially removed(

commented

Sry, previous message accedenctially removed(

Lol, I wondered where it went. If it weren't for email notifications, I wouldn't have seen it at all. I guess I'll give it a look when I have time to scan someone else's source code.

TBH, I also haven't gotten around to making a world with an over-sized map.

commented

Also, maybe implement world's whitelist?
I need atlas only in overworld, but have many variuous world from other mods.

commented
commented

I'd say this is one of the more important current issues

commented

Man, this is a tricky one. I'm guessing that, in order to fix this, the AtlasData object must no longer be a WorldSavedData object. Instead, there should be a set of DimensionSubData and MarkerSubData objects, each of which store data for a particular atlas, at particular coordinates, of a particular dimension. Then something (probably a re-purposed AtlasDataHandler class) needs to hold a cache of these smaller objects as they are requested.

Does that sound about right?

commented

I don't think it is strictly necessary to change the internal structure of AtlasData to do this. (Though i do think that it could and should be more sensibly organized somehow. Is the hash function for IntVec2 good enough? How about a Qtree or other geospatial data structures?)
I used to iterate through AtlasData tile by tile and send fixed-size packets as they fill up, so that after each packet a client's atlas could display the change (Areas would gradually fill in random places). But that was either slow or inelegant, I can't remember exactly why I abandoned that idea.
One way to do this could be to make a general-purpose large packet automatically split up into parts, which are sent one by one and assembled on the client.
But your idea sounds interesting too.

commented

(Having not worked on the mod for a long time, I may be forgetting something important here)
Please see issue #45, as it is connected to this one.
Edit: yep, I did forget to consider #45. Your idea seems like it would help both issues.

commented

Splitting up a large packet into smaller ones sounds easier, but if a map is supposed to be able to grow to a very large size, it might be best to use only relavent regions. I expect I'll make some mistakes implementing this, but I've got a new version of Ubuntu installed, and it's raining outside, so I'll give it a try.

commented

There is a fix for this for 1.7.10 on curse. I'll close it once 1.8.9 and 1.9.4 also have the fix.

commented

It's over. I can finally move on to something more exciting!

commented

Also fixed #103

commented

HOW?!!!!!!!

commented

Forgot to clear the list of tile groups to send between each sending, defeating the entire purpose of the packet rework. This may have been causing #128. Fixed as of 835eb51.