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.

Sign up for GitHub

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

Jump to bottom

Supporting Infinite Maps #1635

Merged
merged 11 commits into from
Jul 4, 2017
Merged

Supporting Infinite Maps #1635

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
2d1a3c8
Adds 16x16 block to store cells
ketanhwr Jun 30, 2017
44c2e2c
Requested changes
ketanhwr Jun 30, 2017
e4f1621
Rename block to chunk
ketanhwr Jul 1, 2017
3650afd
Shifts to QHash
ketanhwr Jul 2, 2017
309db5c
Bug fix
ketanhwr Jul 3, 2017
011043d
Optimisations
ketanhwr Jul 4, 2017
bc1c9c4
Changes implementation of flipping/rotating
ketanhwr Jul 4, 2017
e937a65
Don't use pointers to refer to chunks
bjorn Jul 4, 2017
ea2163c
Chunk is no longer deallocated
ketanhwr Jul 4, 2017
9046a38
Minor changes
ketanhwr Jul 4, 2017
42ec140
Changes implementation of resize
ketanhwr Jul 4, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/libtiled/tiled.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ enum Alignment {
BottomRight
};

static const int CHUNK_SIZE = 16;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either this should be BLOCK_SIZE or the Block class should be renamed to Chunk. I think I'd prefer the latter.

Btw, note that a const int at namespace level is implicitly static, so that keyword is redundant.


static const char TILES_MIMETYPE[] = "application/vnd.tile.list";
static const char FRAMES_MIMETYPE[] = "application/vnd.frame.list";
static const char LAYERS_MIMETYPE[] = "application/vnd.layer.list";
Expand Down
170 changes: 131 additions & 39 deletions src/libtiled/tilelayer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,71 @@

using namespace Tiled;

QRegion Block::region(std::function<bool (const Cell &)> condition) const
{
QRegion region;

for (int y = 0; y < CHUNK_SIZE; ++y) {
for (int x = 0; x < CHUNK_SIZE; ++x) {
if (condition(cellAt(x, y))) {
const int rangeStart = x;
for (++x; x <= CHUNK_SIZE; ++x) {
if (x == CHUNK_SIZE || !condition(cellAt(x, y))) {
const int rangeEnd = x;
region += QRect(rangeStart, y, rangeEnd - rangeStart, 1);
break;
}
}
}
}
}

return region;
}

void Block::setCell(int x, int y, const Cell &cell)
{
int index = x + y * CHUNK_SIZE;

if (mGrid[index].isEmpty() && !cell.isEmpty())
mCells++;
else if (!mGrid[index].isEmpty() && cell.isEmpty())
mCells--;
Copy link
Member

@bjorn bjorn Jun 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would effectively do the same thing with less checking:

    if (!mGrid[index].isEmpty())
        --mCells;
    else if (!cell.isEmpty())
        ++mCells;

If an empty cell gets overwritten with an empty cell, nothing happens. If a non-empty cell gets overwritten with a non-empty cell, it decrements and then increments, which is fine as well.

Or even without branching at all:

mCells -= !mGrid[index].isEmpty();
mCells += !cell.isEmpty();

This is using the fact that a boolean will be either 0 or 1 (not sure if the compiler will warn, though, then a static_cast<int> may be needed).

And we could even remove the negation:

mCells += mGrid[index].isEmpty();
mCells -= cell.isEmpty();

The first line increments mCells when the existing cell is empty, under the assumption that it will be overwritten with a non-empty cell. The second line decrements when the new cell is empty.

mCells += mGrid[index].isEmpty() - cell.isEmpty();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!


mGrid[index] = cell;
}

bool Block::hasCell(std::function<bool (const Cell &)> condition) const
{
for (const Cell &cell : mGrid)
if (condition(cell))
return true;

return false;
}

void Block::removeReferencesToTileset(Tileset *tileset)
{
for (int i = 0, i_end = mGrid.size(); i < i_end; ++i) {
if (mGrid.at(i).tileset() == tileset)
mGrid.replace(i, Cell());
}
}

void Block::replaceReferencesToTileset(Tileset *oldTileset,
Tileset *newTileset)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation is off.

{
for (Cell &cell : mGrid) {
if (cell.tileset() == oldTileset)
cell.setTile(newTileset, cell.tileId());
}
}

TileLayer::TileLayer(const QString &name, int x, int y, int width, int height)
: Layer(TileLayerType, name, x, y)
, mWidth(width)
, mHeight(height)
, mGrid(width * height)
, mEmptyCell(Cell())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're calling the copy constructor here to initialize mEmptyCell with the instance constructed by Cell(). You could call just the constructor by doing mEmptyCell(). However, you should leave this line out entirely because the default constructor will be called anyway.

, mUsedTilesetsDirty(false)
{
Q_ASSERT(width >= 0);
Expand Down Expand Up @@ -88,20 +148,11 @@ QRegion TileLayer::region(std::function<bool (const Cell &)> condition) const
{
QRegion region;

for (int y = 0; y < mHeight; ++y) {
for (int x = 0; x < mWidth; ++x) {
if (condition(cellAt(x, y))) {
const int rangeStart = x;
for (++x; x <= mWidth; ++x) {
if (x == mWidth || !condition(cellAt(x, y))) {
const int rangeEnd = x;
region += QRect(rangeStart + mX, y + mY,
rangeEnd - rangeStart, 1);
break;
}
}
}
}
QMapIterator< QPair<int, int>, Block* > it(mMap);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for using QPair<int, int> instead of QPoint? In memory it is the same thing, but using QPoint will produce easier to read code since you can use x/y instead of first/second.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a map, the key should of such a data type such that it can be easily compared. QPair has operators for < and > while QPoint has not overloaded these operators. If no comparison between two values can be made, then what's the point of inserting a value in a red black tree? It gives compilation error anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point, however we don't need this container to be sorted and if you use a QHash then QPoint will work fine.

while (it.hasNext()) {
it.next();
region += it.value()->region(condition).translated(it.key().first * CHUNK_SIZE + mX,
it.key().second * CHUNK_SIZE + mY);
}

return region;
Expand All @@ -114,7 +165,12 @@ void Tiled::TileLayer::setCell(int x, int y, const Cell &cell)
{
Q_ASSERT(contains(x, y));

Cell &existingCell = mGrid[x + y * mWidth];
Cell existingCell = Cell();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just write:

Cell existingCell;

The default constructor will be called either way.


if (mMap.contains(block(x, y)))
existingCell = mMap[block(x, y)]->cellAt(x % CHUNK_SIZE, y % CHUNK_SIZE);
else
mMap[block(x, y)] = new Block();

if (!mUsedTilesetsDirty) {
Tileset *oldTileset = existingCell.isEmpty() ? nullptr : existingCell.tileset();
Expand All @@ -127,7 +183,12 @@ void Tiled::TileLayer::setCell(int x, int y, const Cell &cell)
}
}

existingCell = cell;
mMap[block(x, y)]->setCell(x % CHUNK_SIZE, y % CHUNK_SIZE, cell);

if (mMap[block(x, y)]->isEmpty()) {
delete mMap[block(x, y)];
mMap.remove(block(x, y));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be done in one line:

delete mMap.take(block(x, y));

}
}

TileLayer *TileLayer::copy(const QRegion &region) const
Expand Down Expand Up @@ -233,7 +294,7 @@ void TileLayer::flip(FlipDirection direction)
}
}

mGrid = newGrid;
copyGrid(newGrid);
}

void TileLayer::flipHexagonal(FlipDirection direction)
Expand Down Expand Up @@ -269,7 +330,7 @@ void TileLayer::flipHexagonal(FlipDirection direction)
}
}

mGrid = newGrid;
copyGrid(newGrid);
}

void TileLayer::rotate(RotateDirection direction)
Expand Down Expand Up @@ -309,7 +370,7 @@ void TileLayer::rotate(RotateDirection direction)

mWidth = newWidth;
mHeight = newHeight;
mGrid = newGrid;
copyGrid(newGrid);
}

void TileLayer::rotateHexagonal(RotateDirection direction, Map *map)
Expand Down Expand Up @@ -390,7 +451,7 @@ void TileLayer::rotateHexagonal(RotateDirection direction, Map *map)

mWidth = newWidth;
mHeight = newHeight;
mGrid = newGrid;
copyGrid(newGrid);

QRect filledRect = region().boundingRect();

Expand All @@ -411,22 +472,30 @@ QSet<SharedTileset> TileLayer::usedTilesets() const
if (mUsedTilesetsDirty) {
QSet<SharedTileset> tilesets;

for (const Cell &cell : mGrid)
if (const Tile *tile = cell.tile())
tilesets.insert(tile->sharedTileset());
QMapIterator< QPair<int, int>, Block* > it(mMap);
while (it.hasNext()) {
it.next();

mUsedTilesets.swap(tilesets);
mUsedTilesetsDirty = false;
for (const Cell &cell : *it.value())
if (const Tile *tile = cell.tile())
tilesets.insert(tile->sharedTileset());

mUsedTilesets.swap(tilesets);
mUsedTilesetsDirty = false;
}
}

return mUsedTilesets;
}

bool TileLayer::hasCell(std::function<bool (const Cell &)> condition) const
{
for (const Cell &cell : mGrid)
if (condition(cell))
QMapIterator< QPair<int, int>, Block* > it(mMap);
while (it.hasNext()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you don't care about the coordinates of the chunk, please use a range-based for:

for (Block *block : mMap)

Same for removeReferencesToTileset, replaceReferencesToTileset and isEmpty.

it.next();
if (it.value()->hasCell(condition))
return true;
}

return false;
}
Expand All @@ -438,9 +507,10 @@ bool TileLayer::referencesTileset(const Tileset *tileset) const

void TileLayer::removeReferencesToTileset(Tileset *tileset)
{
for (int i = 0, i_end = mGrid.size(); i < i_end; ++i) {
if (mGrid.at(i).tileset() == tileset)
mGrid.replace(i, Cell());
QMapIterator< QPair<int, int>, Block* > it(mMap);
while (it.hasNext()) {
it.next();
it.value()->removeReferencesToTileset(tileset);
}

mUsedTilesets.remove(tileset->sharedPointer());
Expand All @@ -449,9 +519,10 @@ void TileLayer::removeReferencesToTileset(Tileset *tileset)
void TileLayer::replaceReferencesToTileset(Tileset *oldTileset,
Tileset *newTileset)
{
for (Cell &cell : mGrid) {
if (cell.tileset() == oldTileset)
cell.setTile(newTileset, cell.tileId());
QMapIterator< QPair<int, int>, Block* > it(mMap);
while (it.hasNext()) {
it.next();
it.value()->replaceReferencesToTileset(oldTileset, newTileset);
}

if (mUsedTilesets.remove(oldTileset->sharedPointer()))
Expand All @@ -478,7 +549,14 @@ void TileLayer::resize(const QSize &size, const QPoint &offset)
}
}

mGrid = newGrid;
for (int y = 0; y < mHeight; ++y) {
for (int x = 0; x < mWidth; ++x) {
if (y < size.height() && x < size.width())
setCell(x, y, newGrid[x + y * size.width()]);
else
setCell(x, y, mEmptyCell);
}
}
setSize(size);
}

Expand Down Expand Up @@ -524,7 +602,7 @@ void TileLayer::offsetTiles(const QPoint &offset,
}
}

mGrid = newGrid;
copyGrid(newGrid);
}

bool TileLayer::canMergeWith(Layer *other) const
Expand Down Expand Up @@ -574,9 +652,12 @@ QRegion TileLayer::computeDiffRegion(const TileLayer *other) const

bool TileLayer::isEmpty() const
{
for (const Cell &cell : mGrid)
if (!cell.isEmpty())
QMapIterator< QPair<int, int>, Block* > it(mMap);
while (it.hasNext()) {
it.next();
if (!it.value()->isEmpty())
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there seems to be missing a call to it.next() here


return true;
}
Expand All @@ -594,8 +675,19 @@ TileLayer *TileLayer::clone() const
TileLayer *TileLayer::initializeClone(TileLayer *clone) const
{
Layer::initializeClone(clone);
clone->mGrid = mGrid;
for (int y = 0; y < mHeight; ++y)
for (int x = 0; x < mWidth; ++x)
clone->setCell(x, y, cellAt(x, y));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of copying each cell over individually, it will be much faster to iterate mMap and clone all chunks instead. That will even allow the QVector of each chunk to do its implicit sharing as long as neither side is modified.

clone->mUsedTilesets = mUsedTilesets;
clone->mUsedTilesetsDirty = mUsedTilesetsDirty;
return clone;
}

void TileLayer::copyGrid(const QVector<Cell> &newGrid)
{
for (int y = 0; y < mHeight; ++y) {
for (int x = 0; x < mWidth; ++x) {
setCell(x, y, newGrid[x + y * mWidth]);
}
}
}
Loading