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

Automatically Resize Maps - #260 #1651

Merged
merged 16 commits into from Aug 15, 2017

Conversation

Projects
None yet
3 participants
@ketanhwr
Contributor

ketanhwr commented Jul 14, 2017

mRegion keeps track of the non-empty region of layer. The bounds function is modified so that it calculates the bounds using mRegion itself.

@bjorn

This comment has been minimized.

Show comment
Hide comment
@bjorn

bjorn Jul 14, 2017

Owner

I think this will not be good for performance. Hence I was suggesting, to keep track of a bounding QRect and only update it based on which chunks have been allocated. There is no need to keep track of the exact region nor do we need it to be accurate down to the tile.

Owner

bjorn commented Jul 14, 2017

I think this will not be good for performance. Hence I was suggesting, to keep track of a bounding QRect and only update it based on which chunks have been allocated. There is no need to keep track of the exact region nor do we need it to be accurate down to the tile.

@ketanhwr

This comment has been minimized.

Show comment
Hide comment
@ketanhwr

ketanhwr Jul 14, 2017

Contributor

Alright, so the region() function should be changed back to what it was prevously?

Contributor

ketanhwr commented Jul 14, 2017

Alright, so the region() function should be changed back to what it was prevously?

@ketanhwr

This comment has been minimized.

Show comment
Hide comment
@ketanhwr

ketanhwr Jul 14, 2017

Contributor

But why do you think that this won't be good for performance? In fact I think it's better now. Because earlier whenever the region() function was called, it traversed through all the cells but now it directly returns a QRegion and updates it when setCell is called.

Contributor

ketanhwr commented Jul 14, 2017

But why do you think that this won't be good for performance? In fact I think it's better now. Because earlier whenever the region() function was called, it traversed through all the cells but now it directly returns a QRegion and updates it when setCell is called.

@bjorn

This comment has been minimized.

Show comment
Hide comment
@bjorn

bjorn Jul 14, 2017

Owner

But why do you think that this won't be good for performance? In fact I think it's better now. Because earlier whenever the region() function was called, it traversed through all the cells but now it directly returns a QRegion and updates it when setCell is called.

Yes, but when does the region function get called? I think this only happens for the tile layer used for the active brush. Maintaining the region for all tile layers of a map is largely a wasted effort, and it really isn't cheap to add/remove many 1x1 QRects to a QRegion (which is why the TileLayer::region(...) (now Chunk::region) function uses a special optimization to avoid too many operations on the QRegion).

Owner

bjorn commented Jul 14, 2017

But why do you think that this won't be good for performance? In fact I think it's better now. Because earlier whenever the region() function was called, it traversed through all the cells but now it directly returns a QRegion and updates it when setCell is called.

Yes, but when does the region function get called? I think this only happens for the tile layer used for the active brush. Maintaining the region for all tile layers of a map is largely a wasted effort, and it really isn't cheap to add/remove many 1x1 QRects to a QRegion (which is why the TileLayer::region(...) (now Chunk::region) function uses a special optimization to avoid too many operations on the QRegion).

@ketanhwr

This comment has been minimized.

Show comment
Hide comment
@ketanhwr

ketanhwr Jul 14, 2017

Contributor

So instead of maintaining a mRegion, you're suggesting that I should maintain a mBounds, right?

Contributor

ketanhwr commented Jul 14, 2017

So instead of maintaining a mRegion, you're suggesting that I should maintain a mBounds, right?

@bjorn

This comment has been minimized.

Show comment
Hide comment
@bjorn

bjorn Jul 14, 2017

Owner

So instead of maintaining a mRegion, you're suggesting that I should maintain a mBounds, right?

Yes. In addition, I was suggesting that you update mBounds with chunk-granularity instead of tile-granularity.

Owner

bjorn commented Jul 14, 2017

So instead of maintaining a mRegion, you're suggesting that I should maintain a mBounds, right?

Yes. In addition, I was suggesting that you update mBounds with chunk-granularity instead of tile-granularity.

@bjorn

I'm looking forward to test this. :-)

Show outdated Hide outdated src/libtiled/tilelayer.h
Show outdated Hide outdated src/libtiled/tilelayer.h
Show outdated Hide outdated src/libtiled/tilelayer.h
Show outdated Hide outdated src/libtiled/tilelayer.cpp
Show outdated Hide outdated src/libtiled/tilelayer.h
@Ablu

This comment has been minimized.

Show comment
Hide comment
@Ablu

Ablu Jul 17, 2017

Contributor

you broke a test apparently... Due to this the travis test failed.

Contributor

Ablu commented Jul 17, 2017

you broke a test apparently... Due to this the travis test failed.

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

Starting to look pretty good.

Show outdated Hide outdated src/libtiled/hexagonalrenderer.cpp
Show outdated Hide outdated src/libtiled/isometricrenderer.cpp
Show outdated Hide outdated src/libtiled/tilelayer.cpp
Show outdated Hide outdated src/tiled/stampbrush.cpp
@@ -349,7 +349,9 @@ class TILEDSHARED_EXPORT TileLayer : public Layer
/**
* Returns the bounds of this layer.
*/
QRect bounds() const { return QRect(mX, mY, mWidth, mHeight); }
QRect bounds() const { return mBounds.translated(mX, mY); }

This comment has been minimized.

@bjorn

bjorn Jul 18, 2017

Owner

I suggested to use bounds in the map renderers, but due to an assert this will currently only work if this function is implemented as:

QRect bounds() const { return mBounds.translated(mX, mY).intersected(rect()); }

That is because mBounds may extend beyond the limits imposed by the layer size due to growing it chunk-by-chunk. I think that is a fine implementation for now, and the intersection can be taken out again when we actually allow layers to grow (and take out the assert in cellAt).

@bjorn

bjorn Jul 18, 2017

Owner

I suggested to use bounds in the map renderers, but due to an assert this will currently only work if this function is implemented as:

QRect bounds() const { return mBounds.translated(mX, mY).intersected(rect()); }

That is because mBounds may extend beyond the limits imposed by the layer size due to growing it chunk-by-chunk. I think that is a fine implementation for now, and the intersection can be taken out again when we actually allow layers to grow (and take out the assert in cellAt).

@ketanhwr ketanhwr changed the title from Modifies bounds function to Automatically Resize Maps - #260 Jul 18, 2017

@ketanhwr

This comment has been minimized.

Show comment
Hide comment
@ketanhwr

ketanhwr Jul 18, 2017

Contributor

I'm not able to understand why the build fails 😕

Contributor

ketanhwr commented Jul 18, 2017

I'm not able to understand why the build fails 😕

@bjorn

A few comments. I'm sorry it's not exhaustive.

Show outdated Hide outdated src/plugins/csv/csvplugin.cpp
Show outdated Hide outdated src/tiled/tilepainter.cpp
Show outdated Hide outdated src/tiled/tilepainter.cpp
Show outdated Hide outdated src/libtiled/hexagonalrenderer.h
int startX = bounds.left();
int startY = bounds.top();
int endX = bounds.right();
int endY = bounds.bottom();

This comment has been minimized.

@bjorn

bjorn Jul 25, 2017

Owner

I believe this should still have the -1:

int endX = bounds.right() - 1;
int endY = bounds.bottom() - 1;

Because the right and bottom of a QRect are inclusive.

@bjorn

bjorn Jul 25, 2017

Owner

I believe this should still have the -1:

int endX = bounds.right() - 1;
int endY = bounds.bottom() - 1;

Because the right and bottom of a QRect are inclusive.

This comment has been minimized.

@ketanhwr

ketanhwr Jul 25, 2017

Contributor

It was previously iterating upto layer->width() - 1. So if - 1 is added, then it doesn't render the last row/column.

@ketanhwr

ketanhwr Jul 25, 2017

Contributor

It was previously iterating upto layer->width() - 1. So if - 1 is added, then it doesn't render the last row/column.

This comment has been minimized.

@bjorn

bjorn Jul 25, 2017

Owner

Ah, right, precisely because QRect is inclusive, we should indeed not do -1.

@bjorn

bjorn Jul 25, 2017

Owner

Ah, right, precisely because QRect is inclusive, we should indeed not do -1.

mDarkRectangle->setRect(sceneRect);
if (mMapDocument->map()->infinite())
mDarkRectangle->setRect(QRect());

This comment has been minimized.

@bjorn

bjorn Jul 25, 2017

Owner

We'll need to find some way to still darken the view, for the "Highlight Current Layer" feature to work on infinite maps.

@bjorn

bjorn Jul 25, 2017

Owner

We'll need to find some way to still darken the view, for the "Highlight Current Layer" feature to work on infinite maps.

@bjorn

While there is indeed the problem with the scene rect, and I have no good solution for this now, I've found a lot of small issues with the existing patch in the meantime which can be addressed.

I suggest we work towards merging this patch without trying to solve the "jumping" problem for now. It will probably take some trial and error to find a workable solution to that, but it's more important to get all the other parts of the code adjusted in the right way.

Show outdated Hide outdated src/libtiled/mapreader.cpp
Show outdated Hide outdated src/libtiled/maprenderer.h
Show outdated Hide outdated src/libtiled/maptovariantconverter.cpp
Show outdated Hide outdated src/libtiled/mapwriter.cpp
Show outdated Hide outdated src/libtiled/mapwriter.cpp
Show outdated Hide outdated src/tiled/thumbnailrenderer.cpp
Show outdated Hide outdated src/tiled/tilepainter.cpp
Show outdated Hide outdated src/tiled/tilepainter.cpp
Show outdated Hide outdated src/tmxrasterizer/tmxrasterizer.cpp
Show outdated Hide outdated tests/staggeredrenderer/test_staggeredrenderer.cpp
@Ablu

This comment has been minimized.

Show comment
Hide comment
@Ablu

Ablu Aug 2, 2017

Contributor

Travis seems to have a compiling issue:

test_staggeredrenderer.cpp:56:11: error: no viable conversion from 'QRect' to
      'QSize'
    QSize mapBoundingRect = renderer.mapBoundingRect();
          ^                 ~~~~~~~~~~~~~~~~~~~~~~~~~~
Contributor

Ablu commented Aug 2, 2017

Travis seems to have a compiling issue:

test_staggeredrenderer.cpp:56:11: error: no viable conversion from 'QRect' to
      'QSize'
    QSize mapBoundingRect = renderer.mapBoundingRect();
          ^                 ~~~~~~~~~~~~~~~~~~~~~~~~~~
@bjorn

bjorn requested changes Aug 6, 2017 edited

Some review feedback.

I guess the main things still open for this change would be:

  • Checking the behavior of automapping. The main thing to fix is AutomappingManager::autoMap taking the map size rather than calculating the combined bounds, but there may be other parts to modify.

  • Implementing the alternative chunk-based storage format.

Show outdated Hide outdated src/tiled/terrainbrush.cpp
Show outdated Hide outdated src/tiled/terrainbrush.cpp
Show outdated Hide outdated src/tiled/terrainbrush.cpp
@@ -68,7 +70,7 @@ Cell TilePainter::cellAt(int x, int y) const
const int layerX = x - mTileLayer->x();
const int layerY = y - mTileLayer->y();
if (!mTileLayer->contains(layerX, layerY))
if (!mTileLayer->contains(layerX, layerY) && !mMapDocument->map()->infinite())

This comment has been minimized.

@bjorn

bjorn Aug 6, 2017

Owner

Check is not necessary at all anymore. If the layer does not contain this position, it will anyway return an empty cell.

Edit: GitHub seems to match this comment to a different line now. In this function it is of course still needed..

@bjorn

bjorn Aug 6, 2017

Owner

Check is not necessary at all anymore. If the layer does not contain this position, it will anyway return an empty cell.

Edit: GitHub seems to match this comment to a different line now. In this function it is of course still needed..

Show outdated Hide outdated src/tiled/tilepainter.cpp
Show outdated Hide outdated src/tmxrasterizer/tmxrasterizer.cpp

ketanhwr added some commits Aug 7, 2017

@bjorn

You'll still need to adjust HexagonalRenderer::mapBoundingRect.

Show outdated Hide outdated src/libtiled/hexagonalrenderer.cpp
Show outdated Hide outdated src/libtiled/isometricrenderer.cpp
Show outdated Hide outdated src/libtiled/hexagonalrenderer.cpp
Show outdated Hide outdated src/libtiled/hexagonalrenderer.cpp
Show outdated Hide outdated src/libtiled/hexagonalrenderer.cpp
Show outdated Hide outdated src/libtiled/hexagonalrenderer.cpp
Show outdated Hide outdated src/libtiled/hexagonalrenderer.cpp

ketanhwr added some commits Aug 15, 2017

@bjorn bjorn merged commit 59b3747 into bjorn:master Aug 15, 2017

1 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment