Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New chunk based format for infinite maps #1696

Merged
merged 4 commits into from Aug 28, 2017
Merged

Conversation

@ketanhwr
Copy link
Contributor

@ketanhwr ketanhwr commented Aug 22, 2017

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.

Copy link
Contributor

@Ablu Ablu left a comment

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?

}
}
}

This comment has been minimized.

@Ablu

Ablu Aug 22, 2017
Contributor

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

This comment has been minimized.

@bjorn

bjorn Aug 25, 2017
Member

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.

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

This comment has been minimized.

@Ablu

Ablu Aug 22, 2017
Contributor

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

w.writeAttribute(QLatin1String("starty"), QString::number(chunkStartY));

if (mLayerDataFormat == Map::XML) {
for (int y1 = 0; y1 < CHUNK_SIZE; y1++) {

This comment has been minimized.

@Ablu

Ablu Aug 22, 2017
Contributor

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

This comment has been minimized.

@bjorn

bjorn Aug 25, 2017
Member

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
Copy link
Contributor Author

@ketanhwr ketanhwr commented Aug 22, 2017

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
Copy link
Contributor

@Ablu Ablu commented Aug 22, 2017

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

Copy link
Member

@bjorn bjorn left a comment

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.

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

QByteArray GidMapper::encodeChunkData(const TileLayer &tileLayer,

This comment has been minimized.

@bjorn

bjorn Aug 25, 2017
Member

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

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

This comment has been minimized.

@bjorn

bjorn Aug 25, 2017
Member

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

}
}
}

This comment has been minimized.

@bjorn

bjorn Aug 25, 2017
Member

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.

@@ -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 :

This comment has been minimized.

@bjorn

bjorn Aug 25, 2017
Member

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

w.writeAttribute(QLatin1String("starty"), QString::number(chunkStartY));

if (mLayerDataFormat == Map::XML) {
for (int y1 = 0; y1 < CHUNK_SIZE; y1++) {

This comment has been minimized.

@bjorn

bjorn Aug 25, 2017
Member

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.

Copy link
Member

@bjorn bjorn left a comment

Some more comments.

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

This comment has been minimized.

@bjorn

bjorn Aug 25, 2017
Member

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.

@@ -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);

This comment has been minimized.

@bjorn

bjorn Aug 25, 2017
Member

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.

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

This comment has been minimized.

@bjorn

bjorn Aug 25, 2017
Member

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.

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

This comment has been minimized.

@bjorn

bjorn Aug 25, 2017
Member

Just pass bounds?

This comment has been minimized.

@ketanhwr

ketanhwr Aug 26, 2017
Author Contributor

🤦‍♂️

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

This comment has been minimized.

@bjorn

bjorn Aug 25, 2017
Member

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.

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

This comment has been minimized.

@bjorn

bjorn Aug 26, 2017
Member

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

This comment has been minimized.

@ketanhwr

ketanhwr Aug 27, 2017
Author Contributor

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

This comment has been minimized.

@bjorn

bjorn Aug 27, 2017
Member

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

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

mMap->setLayerDataFormat(layerDataFormat);

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

This comment has been minimized.

@bjorn

bjorn Aug 26, 2017
Member

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?

This comment has been minimized.

@ketanhwr

ketanhwr Aug 27, 2017
Author Contributor

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 bjorn merged commit 3f907f0 into mapeditor:master Aug 28, 2017
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bjorn
Copy link
Member

@bjorn bjorn commented Aug 28, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.