New chunk based format for infinite maps #1696

ketanhwr · 2017-08-22T18:16:10Z

I've removed startx and starty attributes from Tile Layer. Now, for infinite maps, each 16x16 chunk will be stored along with its startx and starty coordinates.

Ablu

I added some code style comments. Generally I wonder whether it would maybe also make sense to store non-infinite maps in the chunk based format... In some cases this could help to speed up the processing of the file (and file size if no compression is used...). But I am not sure on this and would like to hear @bjorn's feedback first. Otherwise I think that in some places the code is reaching a deep nesting of ifs, loops, etc... which makes it hard to follow at times. Maybe some helper functions could be introduced to prevent too deep nesting and keep the individual parts easier to understand?

Ablu · 2017-08-22T20:49:04Z

src/libtiled/mapreader.cpp

                }
+            }
+        }


I think it would make sense to move this (and the following else branch) into a seperate function to make the code a bit easier to read.

That many braces closing in the same place is usually a good indicator that the code could be moved into a function...

Yes, the reading of the inner part of either data itself or each of its chunk elements should be done in a separate function rather than duplicating the whole code. Property this function again needs to take a target QRect so that it knows where to set the tiles.

Ablu · 2017-08-22T20:54:34Z

src/libtiled/mapreader.cpp

+                                          xml.text().toLatin1(),
+                                          layerDataFormat,
+                                          0,
+                                          0);


I guess the 0 does not need to be in an extra line now ;)

Ablu · 2017-08-22T20:56:44Z

src/libtiled/mapwriter.cpp

+                    w.writeAttribute(QLatin1String("starty"), QString::number(chunkStartY));
+
+                    if (mLayerDataFormat == Map::XML) {
+                        for (int y1 = 0; y1 < CHUNK_SIZE; y1++) {


Maybe use cellY here (or chunkY before)? I find adding numbers to variables to prevent name clashes a bit confusing often.

Yes, but actually this whole code should not be duplicated in the first place. It should be moved into a function taking the QXmlStreamWriter, the tile layer and a QRect specifying which part of the layer to store. That function can then be called either once for the whole tile layer, or repeatedly for each chunk, depending on whether the map is infinite.

ketanhwr · 2017-08-22T21:14:29Z

Generally I wonder whether it would maybe also make sense to store non-infinite maps in the chunk based format... In some cases this could help to speed up the processing of the file (and file size if no compression is used...).

Although this would be quite helpful, but this would break all the current maps 😕

Ablu · 2017-08-22T22:10:53Z

Although this would be quite helpful, but this would break all the current maps

Sure... It would have to be configurable somehow... But I am not sure how such a configuration could look like...

bjorn

Added many comments regarding the same theme: reducing code duplication.

Regarding the usage of chunk-based storage for finite maps, while I agree that this could be useful it's not a high priority to support this right now. Please do make sure that reading of chunks also works for finite maps, which should be pretty much automatically supported.

bjorn · 2017-08-25T14:20:22Z

src/libtiled/gidmapper.cpp

@@ -186,16 +179,96 @@ QByteArray GidMapper::encodeLayerData(const TileLayer &tileLayer,
    return tileData.toBase64();
 }

+QByteArray GidMapper::encodeChunkData(const TileLayer &tileLayer,


Rather than duplicating this whole function, please just add a QRect parameter. If the provided rect is empty it could probably default to QRect(0, 0, tileLayer.width(), tileLayer.height()).

bjorn · 2017-08-25T14:21:39Z

src/libtiled/gidmapper.cpp

 GidMapper::DecodeError GidMapper::decodeLayerData(TileLayer &tileLayer,
                                                  const QByteArray &layerData,
+                                                  Map::LayerDataFormat format) const


Similarly, this function could take a QRect to specify the target area of the decoded layer data.

bjorn · 2017-08-25T14:25:41Z

src/libtiled/mapreader.cpp

                }
+            }
+        }


Yes, the reading of the inner part of either data itself or each of its chunk elements should be done in a separate function rather than duplicating the whole code. Property this function again needs to take a target QRect so that it knows where to set the tiles.

bjorn · 2017-08-25T14:33:06Z

src/libtiled/mapreader.cpp

@@ -897,23 +952,30 @@ void MapReaderPrivate::decodeCSVLayerData(TileLayer &tileLayer,
    QString trimText = text.trimmed().toString();
    QStringList tiles = trimText.split(QLatin1Char(','));

-    if (tiles.length() != tileLayer.width() * tileLayer.height()) {
+    int lengthCheck = (mMap->infinite()) ? CHUNK_SIZE * CHUNK_SIZE :


decodeCSVLayerData should take a QRect parameter specifying the size and position of the tiles it is decoding.

bjorn · 2017-08-25T14:40:11Z

src/libtiled/mapwriter.cpp

+                    w.writeAttribute(QLatin1String("starty"), QString::number(chunkStartY));
+
+                    if (mLayerDataFormat == Map::XML) {
+                        for (int y1 = 0; y1 < CHUNK_SIZE; y1++) {


Yes, but actually this whole code should not be duplicated in the first place. It should be moved into a function taking the QXmlStreamWriter, the tile layer and a QRect specifying which part of the layer to store. That function can then be called either once for the whole tile layer, or repeatedly for each chunk, depending on whether the map is infinite.

bjorn

Some more comments.

bjorn · 2017-08-25T20:36:58Z

src/libtiled/gidmapper.cpp

+    const int startX = bounds.x();
+    const int startY = bounds.y();
+    const int endX = startX + bounds.width() - 1;
+    const int endY = startY + bounds.height() - 1;


There isn't much point in having these local variables here anymore, since you already have the bounds. Please use it directly. You can use bounds.left() and bounds.right() instead of startX and startY for example, and reserve bounds.width() * bounds.height() * 4 space in tileData.

bjorn · 2017-08-25T20:45:16Z

src/libtiled/gidmapper.cpp

@@ -293,10 +220,10 @@ GidMapper::DecodeError GidMapper::decodeChunkData(TileLayer &tileLayer,
            return isEmpty() ? TileButNoTilesets : InvalidTile;
        }

-        tileLayer.setCell(x + startX, y + startY, result);
+        tileLayer.setCell(x + bounds.x(), y + bounds.y(), result);


I think the code would be more readable if you would initialize x and y with bounds.x() and bounds.y() and adjust the check before y++ to x == bounds.right() + 1 (and adjust reset of x), instead of doing this addition here.

Same goes for the code in readTileLayerRect and writeTileLayerData.

bjorn · 2017-08-25T20:52:55Z

src/libtiled/mapreader.cpp

-    int width = (mMap->infinite()) ? CHUNK_SIZE : tileLayer.width();
-    int height = (mMap->infinite()) ? CHUNK_SIZE : tileLayer.height();
+    int width = bounds.width();
+    int height = bounds.height();


Please iterate the area described by bounds directly, rather than from 0 to width and then adding bounds.x(). I just think it's easier to understand.

bjorn · 2017-08-25T21:07:10Z

src/libtiled/mapwriter.cpp

+                                                          QRect(bounds.x(),
+                                                                bounds.y(),
+                                                                bounds.width(),
+                                                                bounds.height()));


Just pass bounds?

🤦‍♂️

bjorn · 2017-08-25T21:14:09Z

src/libtiled/mapreader.cpp

+                if (xml.name() == QLatin1String("chunk")) {
+                    const QXmlStreamAttributes atts = xml.attributes();
+                    int startX = atts.value(QLatin1String("startx")).toInt();
+                    int startY = atts.value(QLatin1String("starty")).toInt();


Please have attributes x, y, width and height for chunk elements. The file format should not rely on a hard-coded internal chunk size. If you write it in the file it is much more flexible.

bjorn · 2017-08-26T20:32:39Z

src/libtiled/mapwriter.cpp

+            chunkStartY += CHUNK_SIZE;
+        }
+    } else {
+        writeTileLayerData(w, tileLayer, QRect(0, 0, endX + 1, endY + 1));


Judging by the code above, endX + 1 and endY + 1 can't possibly be the right values here, and should rather be tileLayer.width() and tileLayer.height().

int startX = 0; int startY = 0; int endX = tileLayer.width() - 1; int endY = tileLayer.height() - 1;

endX + 1 and endY + 1 are same as tileLayer.width() and tileLayer.height().

Ah, that's a little confusing. Please just use tileLayer.width() and tileLayer.height() here and move those variables local to the other branch.

bjorn · 2017-08-26T20:34:30Z

src/libtiled/mapreader.cpp

@@ -825,26 +823,52 @@ void MapReaderPrivate::readTileLayerData(TileLayer &tileLayer,

    mMap->setLayerDataFormat(layerDataFormat);

-    int x = 0;
-    int y = 0;
+    if (mMap->infinite()) {


It's a pity the reading code relies on checking whether the map it's reading is infinite or not. I believe this really shouldn't be necessary, and not doing so would help with forward-compatibility when eventually we may support chunked storage for finite maps. Can you think of a way to handle the chunks without this check?

I'm not able to think up of a solution. I don't see how we could remove this check, atleast as of now.

bjorn · 2017-08-28T07:59:54Z

Alright, I'll have a look later at trying to remove the infinite check on reading, but otherwise it looks good, thanks!

New chunk based format for infinite maps

d0c8fea

Ablu suggested changes Aug 22, 2017

View reviewed changes

bjorn requested changes Aug 25, 2017

View reviewed changes

Review changes

62cc1a3

bjorn requested changes Aug 25, 2017

View reviewed changes

Improved code and modified format a bit

4cadde3

bjorn requested changes Aug 26, 2017

View reviewed changes

Minor fix

eeeb5a3

bjorn merged commit 3f907f0 into mapeditor:master Aug 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New chunk based format for infinite maps #1696

New chunk based format for infinite maps #1696

ketanhwr commented Aug 22, 2017

Ablu left a comment

Ablu Aug 22, 2017

bjorn Aug 25, 2017

Ablu Aug 22, 2017

Ablu Aug 22, 2017

bjorn Aug 25, 2017

ketanhwr commented Aug 22, 2017

Ablu commented Aug 22, 2017

bjorn left a comment

bjorn Aug 25, 2017

bjorn Aug 25, 2017

bjorn Aug 25, 2017

bjorn Aug 25, 2017

bjorn Aug 25, 2017

bjorn left a comment

bjorn Aug 25, 2017

bjorn Aug 25, 2017

bjorn Aug 25, 2017

bjorn Aug 25, 2017

ketanhwr Aug 26, 2017

bjorn Aug 25, 2017

bjorn Aug 26, 2017

ketanhwr Aug 27, 2017

bjorn Aug 27, 2017

bjorn Aug 26, 2017

ketanhwr Aug 27, 2017

bjorn commented Aug 28, 2017

New chunk based format for infinite maps #1696

New chunk based format for infinite maps #1696

Conversation

ketanhwr commented Aug 22, 2017

Ablu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ketanhwr commented Aug 22, 2017

Ablu commented Aug 22, 2017

bjorn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjorn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjorn commented Aug 28, 2017