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

Conversation

Projects
None yet
4 participants
@ketanhwr
Contributor

ketanhwr commented Jun 30, 2017

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.

@Ablu

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

Show outdated Hide outdated src/libtiled/tilelayer.cpp Outdated
Show outdated Hide outdated src/libtiled/tilelayer.cpp Outdated
Show outdated Hide outdated src/libtiled/tilelayer.cpp Outdated
Show outdated Hide outdated src/libtiled/tilelayer.cpp Outdated
Show outdated Hide outdated src/libtiled/tilelayer.cpp Outdated
Show outdated Hide outdated src/libtiled/tilelayer.h Outdated
Show outdated Hide outdated src/libtiled/tilelayer.h Outdated
Show outdated Hide outdated src/tiled/bucketfilltool.cpp Outdated
Show outdated Hide outdated src/tiled/bucketfilltool.cpp Outdated
Show outdated Hide outdated src/tiled/tilesetdock.cpp Outdated
@Ablu

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

This comment has been minimized.

Show comment
Hide comment
@ketanhwr

ketanhwr Jun 30, 2017

Contributor

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.

Contributor

ketanhwr commented Jun 30, 2017

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.

@bjorn

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!

Show outdated Hide outdated src/libtiled/tiled.h Outdated
Show outdated Hide outdated src/libtiled/tilelayer.cpp Outdated
Show outdated Hide outdated src/libtiled/tilelayer.cpp Outdated
Show outdated Hide outdated src/libtiled/tilelayer.cpp Outdated
Show outdated Hide outdated src/libtiled/tilelayer.cpp Outdated
Show outdated Hide outdated src/libtiled/tilelayer.cpp Outdated
Show outdated Hide outdated src/libtiled/tilelayer.cpp Outdated
Show outdated Hide outdated src/libtiled/tilelayer.h Outdated
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(); }

This comment has been minimized.

@bjorn

bjorn Jun 30, 2017

Owner

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.

@bjorn

bjorn Jun 30, 2017

Owner

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.

Show outdated Hide outdated src/libtiled/tilelayer.h Outdated
@ketanhwr

This comment has been minimized.

Show comment
Hide comment
@ketanhwr

ketanhwr Jul 1, 2017

Contributor

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.

Contributor

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.

@bjorn

Some more comments.

Show outdated Hide outdated src/libtiled/tilelayer.h Outdated
Show outdated Hide outdated src/libtiled/tilelayer.h Outdated
Show outdated Hide outdated src/libtiled/tilelayer.cpp Outdated
Show outdated Hide outdated src/libtiled/tilelayer.cpp Outdated
@bjorn

Two possible optimizations.

Show outdated Hide outdated src/libtiled/tilelayer.cpp Outdated
Show outdated Hide outdated src/libtiled/tilelayer.cpp Outdated

ketanhwr and others added some commits Jul 4, 2017

Don't use pointers to refer to chunks
Resolves leaking of chunks and makes some stuff easier.
@bjorn

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

Show outdated Hide outdated src/libtiled/tilelayer.cpp Outdated
Show outdated Hide outdated src/libtiled/tilelayer.cpp Outdated
Show outdated Hide outdated src/libtiled/tilelayer.cpp Outdated
Show outdated Hide outdated src/libtiled/tilelayer.cpp Outdated
Show outdated Hide outdated src/libtiled/tilelayer.cpp Outdated
Show outdated Hide outdated src/libtiled/tilelayer.cpp Outdated
@bjorn

This comment has been minimized.

Show comment
Hide comment
@bjorn

bjorn Jul 4, 2017

Owner

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.

Owner

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.

Chunk is no longer deallocated
The condition of deallocating a chunk is extremely rare
@bjorn

Small comments.

Show outdated Hide outdated src/libtiled/tilelayer.cpp Outdated
Show outdated Hide outdated src/libtiled/tilelayer.cpp Outdated

ketanhwr added some commits Jul 4, 2017

@bjorn bjorn merged commit fb8c380 into bjorn:master Jul 4, 2017

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@HeadClot

This comment has been minimized.

Show comment
Hide comment
@HeadClot

HeadClot Jul 4, 2017

Happy that this got merged in.

HeadClot commented Jul 4, 2017

Happy that this got merged in.

@ketanhwr

This comment has been minimized.

Show comment
Hide comment
@ketanhwr

ketanhwr Jul 4, 2017

Contributor

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

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@HeadClot

HeadClot Jul 4, 2017

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

HeadClot commented Jul 4, 2017

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

@ketanhwr ketanhwr deleted the ketanhwr:infinite-maps branch Jul 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment