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

Supporting Infinite Maps #1635

Merged
merged 11 commits into from
Jul 4, 2017
Merged

Supporting Infinite Maps #1635

merged 11 commits into from
Jul 4, 2017

Conversation

ketanhwr
Copy link
Contributor

In progress PR for #260.

Currently I've added a class Block which is a 16x16 grid of cells. This is used in TileLayer and now, TileLayer no longes needs mGrid, and only stores instances of Block. The blocks are stored in a QMap and these blocks get deleted when they're empty.

Copy link
Contributor

@Ablu Ablu left a comment

Choose a reason for hiding this comment

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

Can you move the 16 to some constant at some place. Currently it is a magic number which occurs in many many places.

Cell existingCell = Cell();

if (mMap.contains(block(x, y)))
existingCell = mMap[block(x, y)]->cellAt(x%16, y%16);
Copy link
Contributor

Choose a reason for hiding this comment

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

x % 16 <<--- there should be spaces between the operators

for (int x = 0; x < mWidth; ++x) {
setCell(x, y, newGrid[x + y * mWidth]);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

4th copy of this code snipped now... Maybe time for a helper function?

@@ -127,7 +183,12 @@ void Tiled::TileLayer::setCell(int x, int y, const Cell &cell)
}
}

existingCell = cell;
mMap[block(x, y)]->setCell(x%16, y%16, cell);
Copy link
Contributor

Choose a reason for hiding this comment

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

spaces around operators again

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

@@ -594,7 +694,9 @@ TileLayer *TileLayer::clone() const
TileLayer *TileLayer::initializeClone(TileLayer *clone) const
{
Layer::initializeClone(clone);
clone->mGrid = mGrid;
for (int x = 0; x < mWidth; ++x)
for (int y = 0; y < mHeight; ++y)
Copy link
Contributor

Choose a reason for hiding this comment

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

In other cases you first iterate over the y, here you first iterate over x. Generally first iterating over y should be faster due to the access being x + y * 16 regarding cache hits.

@@ -371,6 +421,11 @@ inline bool TileLayer::contains(const QPoint &point) const
return contains(point.x(), point.y());
}

inline QPair<int, int> TileLayer::block(int x, int y) const
Copy link
Contributor

Choose a reason for hiding this comment

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

I find the name of this function not really describing... it sounds like it blocks something... But it actually calculates the block coordinates...

protected:
TileLayer *initializeClone(TileLayer *clone) const;

private:
int mWidth;
int mHeight;
QVector<Cell> mGrid;
Cell mEmptyCell;
QMap< QPair<int, int>, Block* > mMap;
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not really like this QPair here... Why not QPoint? Or introduce an own struct like BlockCoordinate?

if (const Tile *tile = cell.tile())
mRandomCellPicker.add(cell, tile->probability());
const TileLayer &tileLayer = *variation.tileLayer();
for (int x = 0; x < tileLayer.width(); ++x) {
Copy link
Contributor

Choose a reason for hiding this comment

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

iteration order is wrong here again.

const TileLayer &tileLayer = *variation.tileLayer();
for (int x = 0; x < tileLayer.width(); ++x) {
for (int y = 0; y < tileLayer.height(); ++y) {
const Cell cell = tileLayer.cellAt(x, y);
Copy link
Contributor

Choose a reason for hiding this comment

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

&cell =, even if it probably does not matter

const QModelIndex modelIndex = model->tileIndex(tile);
QItemSelectionModel *selectionModel = view->selectionModel();
selections[selectionModel].select(modelIndex, modelIndex);
for (int x = 0; x < tileLayer.width(); ++x) {
Copy link
Contributor

Choose a reason for hiding this comment

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

iteration order again.

Copy link
Contributor

@Ablu Ablu left a comment

Choose a reason for hiding this comment

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

given the many iterations over the whole map it might make sense to provide an iterator for that... But I am not sure about that...

@ketanhwr
Copy link
Contributor Author

Can you suggest some other name other than block(x, y)? I couldn't think of one. And regarding QPair, I personally think that there won't be any point of adding a different data type there since it already completes all the task easily.

Copy link
Member

@bjorn bjorn left a comment

Choose a reason for hiding this comment

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

I really like where this is going and already in its current form it can mean a huge reduction in memory usage for people using many tile layers with little content.

At the moment this patch leaves many operations that iterate the entire area and that will even allocate the entire layer as huge array again, like the resize and offset operations. In each case we should consider ways to perform those operations more efficiently.

In any case, good start and let's see where we can take this!

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

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

@bjorn bjorn Jun 30, 2017

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!

}

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.

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.

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

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.

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

@@ -191,6 +244,8 @@ class TILEDSHARED_EXPORT TileLayer : public Layer
bool contains(int x, int y) const;
bool contains(const QPoint &point) const;

QPair<int, int> block(int x, int y) const;
Copy link
Member

@bjorn bjorn Jun 30, 2017

Choose a reason for hiding this comment

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

Since this doesn't return the actual block, the function should really be called blockCoordinates (or chunkCoordinates). However, I think you don't need such a function at all. Rather, I would define two functions:

  • Chunk *chunk(x, y);
    Returns a chunk at the given tile coordinates, allocating if needed.

  • Chunk *findChunk(x, y) const;
    Returns a chunk at the given tile coordinates, if it exists.

It seems to me like that is really all you need in the rest of the code.

Edit: Ok, and you need to be able to delete the chunks, so that would be three functions that need to calculate block coordinates. However, since this depends only on the const CHUNK_SIZE, it needs not be a member and you can put that in the cpp file:

static QPoint chunkCoordinates(int x, int)
{
    return QPoint(x / CHUNK_SIZE, y / CHUNK_SIZE);
}

However, note that this won't work correctly for negative values of x and y.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess, floor() will solve the problem for negative values?

Copy link
Member

@bjorn bjorn Jul 1, 2017

Choose a reason for hiding this comment

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

Yes, though that would require doing the division with doubles and converting back to integer. Alternatively you could special-case negative values:

x < 0 ? (x + 1) / CHUNK_SIZE - 1 : x / CHUNK_SIZE

I'm not sure what's best performance-wise, but I would guess this one to be faster.

Btw, I noticed x >> 4 would also do exactly what we want, but unfortunately right-shifting negative values is implementation-defined, which means I could just be lucky that my compiler does what we want in this case, so I wouldn't rely on it.

QVector<Cell>::iterator begin() { return mGrid.begin(); }
QVector<Cell>::iterator end() { return mGrid.end(); }
QVector<Cell>::const_iterator begin() const { return mGrid.begin(); }
QVector<Cell>::const_iterator end() const { return mGrid.end(); }
Copy link
Member

Choose a reason for hiding this comment

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

We should really still provide a way to iterate over the cells of a tile layer, especially now that it is allocated in chunks. Iterating only the chunks can mean a significant performance boost when iterating layers with large empty areas.

Of course, it will get a bit more complicated then. We'll need an iterator class that includes an iterator over mMap as a member in addition to a QVector<Cell>::iterator, such that when it reaches the end of a chunk, it can look for the next chunk to iterate. And unfortunately, we'd need a const-version of this iterator as well.


protected:
TileLayer *initializeClone(TileLayer *clone) const;

private:
int mWidth;
int mHeight;
QVector<Cell> mGrid;
Cell mEmptyCell;
QMap< QPair<int, int>, Block* > mMap;
Copy link
Member

Choose a reason for hiding this comment

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

I think mMap is not descriptive enough. Could be mChunks (with Block renamed to Chunk). Also, since we don't need the chunks to be sorted by their chunk coordinates, you should use a QHash for better lookup performance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should really still provide a way to iterate over the cells of a tile layer, especially now that it is allocated in chunks. Iterating only the chunks can mean a significant performance boost when iterating layers with large empty areas.

A QMap would help in this case I think.

Copy link
Member

Choose a reason for hiding this comment

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

I don't see how it matters whether QMap or QHash is used. Both can be iterated just fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No I mean, when we'll iterate over the chunks, I think we'll prefer to iterate over the chunks in sorted order. Using QHash, we cannot iterate in a sorted order.

Copy link
Member

Choose a reason for hiding this comment

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

I can't think of any use-case where we would care about the iteration order over the chunks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, QPoint doesn't have qHash defined. So should I go with QHash< QPair<int, int>, Chunk* > only?

Copy link
Member

Choose a reason for hiding this comment

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

Based on the qHash implementation for QPair, we can derive the following implementation for QPoint:

inline uint qHash(const QPoint &key, uint seed = 0) Q_DECL_NOTHROW
{
    uint h1 = qHash(key.x(), seed);
    uint h2 = qHash(key.y(), seed);
    return ((h1 << 16) | (h1 >> 16)) ^ h2 ^ seed;
}

@ketanhwr
Copy link
Contributor Author

ketanhwr commented Jul 1, 2017

At the moment this patch leaves many operations that iterate the entire area and that will even allocate the entire layer as huge array again, like the resize and offset operations. In each case we should consider ways to perform those operations more efficiently.

The simplest solution I can think of is just like the previous one. Creating a newChunks instead of newGrid and then iterating over the chunks and performing the operations, and in the end copy and newChunks to mChunks.

Copy link
Member

@bjorn bjorn left a comment

Choose a reason for hiding this comment

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

Some more comments.

if (mChunks.contains(chunkCoordinates))
return mChunks[chunkCoordinates];
else
return nullptr;
Copy link
Member

Choose a reason for hiding this comment

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

The above four lines can be written as:

return mChunks.value(chunkCoordinates);

{
QPair<int, int> chunkCoordinates(x / CHUNK_SIZE, y / CHUNK_SIZE);
if (mChunks.contains(chunkCoordinates))
return mChunks[chunkCoordinates];
Copy link
Member

Choose a reason for hiding this comment

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

This does a look-up twice, which we can avoid:

if (Chunk *chunk = mChunks.value(chunkCoordinates))
    return chunk;
return mChunks[chunkCoordinates] = new Chunk;

Cell &existingCell = mGrid[x + y * mWidth];
Cell existingCell;

existingCell = chunk(x, y)->cellAt(x % CHUNK_SIZE, y % CHUNK_SIZE);
Copy link
Member

Choose a reason for hiding this comment

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

This x % CHUNK_SIZE won't work for negative coordinates. Instead, with a chunk size of 16, we could do x & 0xF. We could call this x & CHUNK_MASK and define:

const int CHUNK_MASK = CHUNK_SIZE - 1;

Of course, this relies on the chunk size always being a power of 2.

while (it.hasNext()) {
it.next();
for (int x = 0; x < CHUNK_SIZE; ++x) {
for (int y = 0;y < CHUNK_SIZE; ++y) {
Copy link
Contributor

Choose a reason for hiding this comment

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

here a space is missing in front of y

Copy link
Member

@bjorn bjorn left a comment

Choose a reason for hiding this comment

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

Two possible optimizations.


clone->setCell(it.key().x() * CHUNK_SIZE + x,
it.key().y() * CHUNK_SIZE + y,
it.value()->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.

Actually this is a needlessly slow way of cloning. You should just copy over each chunk:

    QHashIterator<QPoint, Chunk* > it(mChunks);
    while (it.hasNext()) {
        it.next();
        clone->mChunks.insert(it.key(), new Chunk(*it.value()));
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I totally forgot that we can access the private members of another object in the same class. Also, forgot about the implicit copy constructor. Nice..!

existingCell = cell;
chunk(x, y)->setCell(x & CHUNK_MASK, y & CHUNK_MASK, cell);

if (chunk(x, y)->isEmpty())
Copy link
Member

Choose a reason for hiding this comment

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

This is the third call to chunk(x, y) in this function. You should really call it just once and remember the return value.

Copy link
Member

@bjorn bjorn left a comment

Choose a reason for hiding this comment

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

I think this was good approach! Noticed a few issues.

@@ -271,31 +271,42 @@ void TileLayer::erase(const QRegion &area)

void TileLayer::flip(FlipDirection direction)
{
QVector<Cell> newGrid(mWidth * mHeight);
TileLayer *newLayer = new TileLayer(QLatin1String(""), 0, 0, mWidth, mHeight);
Copy link
Member

Choose a reason for hiding this comment

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

The way to make an empty string is QString().

newLayer->setCell(mWidth - _x - 1, _y, dest);
} else if (direction == FlipVertically) {
dest.setFlippedVertically(!source.flippedVertically());
newLayer->setCell(_x, mHeight -y - 1, dest);
Copy link
Member

Choose a reason for hiding this comment

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

Missing space after -

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And it should be _y too.

dest.setRotatedHexagonal120((mask & 1) != 0);

(direction == FlipHorizontally) ? newLayer->setCell(mWidth - _x - 1, _y, dest)
: newLayer->setCell(_x, mHeight - _y - 1, dest);
Copy link
Member

Choose a reason for hiding this comment

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

Please write this as an if, else

int _y = it.key().y() * CHUNK_SIZE + y;

if (!contains(_x, _y))
continue;
Copy link
Member

Choose a reason for hiding this comment

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

I think instead of doing this contains check each time, it would be better to check dest.isEmpty() below and continue then. This applies to the other functions as well.

}
}

copyGrid(newGrid);
mChunks = newLayer->mChunks;
delete newLayer;
Copy link
Member

Choose a reason for hiding this comment

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

I realized this only worked because we were never deleting the chunks. I've fixed that leak by pushing a change where the chunk instances are managed by the QHash.

Btw, instead of manual deletion, please declare the newLayer as:

QScopedPointer<TileLayer> newLayer


// Skip out of bounds tiles
if (!bounds.contains(_x, _y)) {
newLayer->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.

You don't need to do this, since the newLayer is already a clone of the current layer. Actually, this means that you should only iterate the area indicated by bounds. Iterating just the chunks is not enough, since it could fail to include the needed area.

Copy link
Contributor Author

@ketanhwr ketanhwr Jul 4, 2017

Choose a reason for hiding this comment

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

That would mean iterating like this, right?

    for (int x = bounds.x(); x < bounds.x() + bounds.width(); ++x) {
        for (int y = bound.y(); y < bounds.y() + bounds.height(); ++y) {
            ...
        }
    }

Copy link
Member

@bjorn bjorn Jul 4, 2017

Choose a reason for hiding this comment

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

Yes, though you can use top, left, right and bottom of QRect instead (noting the <=):

    for (int y = bounds.top(); y <= bounds.bottom(); ++y) {
        for (int x = bounds.left(); x <= bounds.right(); ++x) {
            ...
        }
    }

Edit: and reversed the iteration order.

@bjorn
Copy link
Member

bjorn commented Jul 4, 2017

Btw, I noticed the main reason for the slow load times is due to the following lines in setCell:

    if (_chunk.isEmpty())
        mChunks.remove(chunkCoordinates(x, y));

In particular, it is slow because setting an empty cell at a location that is already entirely empty will cause the chunk to get allocated and then immediately de-allocated again, and this will happen for each cell of an empty tile layer.

While this confirms that your idea to avoid setting empty cells in the map reader would resolve that particular slowness, I think instead it may be good to avoid the allocation and de-allocation in general by inserting the following check at the top:

    if (cell.isEmpty() && !findChunk(x, y))
        return;

Additionally, we could not bother with de-allocating chunks at all. I think it's a bit dubious whether we really need to free up memory there, because the main point of the chunks is to avoid massive memory allocation. In contrast, the user is probably not going to erase large areas and then care about whether that frees up some memory. If we don't bother with de-allocation, then there is also no need for keeping track of mUsedCells in the chunks.

The condition of deallocating a chunk is extremely rare
Copy link
Member

@bjorn bjorn left a comment

Choose a reason for hiding this comment

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

Small comments.

@@ -165,6 +170,9 @@ void Tiled::TileLayer::setCell(int x, int y, const Cell &cell)
{
Q_ASSERT(contains(x, y));

if (cell.isEmpty() && !findChunk(x, y))
return;
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.

const Cell &source = it.value().cellAt(x, y);
Cell dest(source);

if (dest.isEmpty())
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 do this check a little earlier based on source, but in fact I wonder why you have two variables at all. I'd just do:

Cell cell = it.value().cellAt(x, y);
if (cell.isEmpty())
    continue;

@bjorn bjorn merged commit fb8c380 into mapeditor:master Jul 4, 2017
@HeadClot
Copy link

HeadClot commented Jul 4, 2017

Happy that this got merged in.

@ketanhwr
Copy link
Contributor Author

ketanhwr commented Jul 4, 2017

@HeadClot, the feature is not complete yet. Currently the only thing completed is storing the Tile Layer in 16*16 blocks. I'll now proceed to work on automatically resizing maps.

@HeadClot
Copy link

HeadClot commented Jul 4, 2017

@ketanhwr Thanks for the info. I am still excited to see this feature be worked on :)

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

Successfully merging this pull request may close these issues.

4 participants