Tiled Quick: rendering only visible tiles in isometric maps

[mitchcurtis]

Support for isometric maps in tiledquickplugin was added in #1535, but all tiles in the map are rendered, rather than only those tiles within the MapItem's visibleArea.

[mitchcurtis]

@bjorn, can you please give some guidance about which parts of the code would need to be modified to fix this? I'd like to have a go at fixing it, but am unsure where to start.

From what I can see, this:

https://github.com/bjorn/tiled/blob/9cc4bcd6322cd2d7b73a2de1f8a594749bd29837/src/tiledquickplugin/tilelayeritem.cpp#L333-L334

should become:

const QRect rect = mapItem->visibleTileArea(mLayer);

and then this fixed to work with isometric maps:

https://github.com/bjorn/tiled/blob/9cc4bcd6322cd2d7b73a2de1f8a594749bd29837/src/tiledquickplugin/mapitem.cpp#L58-L84

There's also a comment here so I assume this needs to be changed too:

https://github.com/bjorn/tiled/blob/9cc4bcd6322cd2d7b73a2de1f8a594749bd29837/src/tiledquickplugin/tilelayeritem.cpp#L254-L288

[mitchcurtis]

With this patch, I see that visibleTileArea() needs to account for the different "shape" of an isometric map:

diff --git a/src/tiledquick/qml/main.qml b/src/tiledquick/qml/main.qml
index d67909ea..67de1c5d 100644
--- a/src/tiledquick/qml/main.qml
+++ b/src/tiledquick/qml/main.qml
@@ -14,7 +14,7 @@ ApplicationWindow {
     height: 720
     minimumWidth: 480
     minimumHeight: 320
-    title: qsTr("Tiled Quick")
+    title: qsTr("Tiled Quick | %1").arg(mapItem.visibleArea)
     visible: true
 
     FontLoader {
diff --git a/src/tiledquickplugin/tilelayeritem.cpp b/src/tiledquickplugin/tilelayeritem.cpp
index 2aebc089..b128f3bb 100644
--- a/src/tiledquickplugin/tilelayeritem.cpp
+++ b/src/tiledquickplugin/tilelayeritem.cpp
@@ -330,8 +330,8 @@ QSGNode *TileLayerItem::updatePaintNode(QSGNode *node,
 void TileLayerItem::updateVisibleTiles()
 {
     const MapItem *mapItem = static_cast<MapItem*>(parentItem());
-    const QRect rect = mLayer->map()->orientation() == Map::Orthogonal
-            ? mapItem->visibleTileArea(mLayer) : mLayer->bounds();
+    const QRect rect = mapItem->visibleTileArea(mLayer);
+    qDebug() << mapItem->visibleArea() << rect;
 
     if (mVisibleTiles != rect) {
         mVisibleTiles = rect;

but at worst this should just mean that too little of the map is rendered. A more important issue is the glitch in rendering:

I wonder where this issue comes from. I'm looking at IsometricRenderHelper::addTilesToNode(), and it just works with the given rect, so I don't see how it could be causing this issue. Perhaps that TODO comment is wrong.

TilesNode::processTileData() seems to just be responsible for drawing individual tiles, so I don't see it causing this...

[bjorn]

Well, one problem is that visibleTileArea expects to return a rectangular area in tile coordinates. However, on an isometric map, the visible tiles do not form a rectangular area. We can still use a QRect, but we'd need to interpret it differently in the renderer.

Alternatively, we'd pass the visible area in pixels to the renderer and then the renderer can only draw the visible tiles (this is how it works with the current MapRenderer implementations). Then you can check the code in the IsometricMapRenderer to see how it draws just the tiles in the given rectangle.

I can't currently explain that glitch either though. I'd also expect a different result. Maybe I can play with it a bit tomorrow.

[bjorn]

@mitchcurtis I've fixed that rendering issue in your screenshot in change 0712c20. I need to stop for today. Feel free to see if you can improve the rendering by expanding that rectangle until it fills the viewport (the rectangle would have to be based on which tiles are visible in the corners).

[mitchcurtis]

Well, one problem is that visibleTileArea expects to return a rectangular area in tile coordinates. However, on an isometric map, the visible tiles do not form a rectangular area. We can still use a QRect, but we'd need to interpret it differently in the renderer.

Alternatively, we'd pass the visible area in pixels to the renderer and then the renderer can only draw the visible tiles (this is how it works with the current MapRenderer implementations). Then you can check the code in the IsometricMapRenderer to see how it draws just the tiles in the given rectangle.

It sounds like you'd prefer this approach. Do I understand correctly that this work would be done in IsometricRenderHelper::addTilesToNode() in that case?

I can't currently explain that glitch either though. I'd also expect a different result. Maybe I can play with it a bit tomorrow.

Thanks for fixing that!

[mitchcurtis]

Well that was easier than I expected... copying the code from IsometricRenderer::drawTileLayer() worked well. With mitchcurtis@bd119e6 it's almost perfect.

There is still some red showing through when moving the map from left to right and vice versa. Do you know where I'd fix that? Or why it's not a problem for the widget-based rendering path?

[bjorn]

Well that was easier than I expected... copying the code from IsometricRenderer::drawTileLayer() worked well. With mitchcurtis@bd119e6 it's almost perfect.

Ah, nice work!

There is still some red showing through when moving the map from left to right and vice versa. Do you know where I'd fix that? Or why it's not a problem for the widget-based rendering path?

Could it just be an updating issue? The update() call only happens when mVisibleTiles changes, but this member is not used for the isometric rendering case.

Of course, this was also an optimization, which will not work when we work with the visible area in pixels.

[bjorn]

Btw, I wonder if it would make sense to adjust the MapRenderer::drawTileLayer signature so that it would take an abstracting interface or callback through which tiles are rendered, which would have scene graph and QPainter implementations. This could even help enabling export of isometric maps to GameMaker 1.4 format, which needs to write out XML elements representing each tile.

[mitchcurtis]

Btw, I wonder if it would make sense to adjust the MapRenderer::drawTileLayer signature so that it would take an abstracting interface or callback through which tiles are rendered, which would have scene graph and QPainter implementations. This could even help enabling export of isometric maps to GameMaker 1.4 format, which needs to write out XML elements representing each tile.

Yeah it seems a bit silly to duplicate the code. I can try, but no guarantees. :p

[mitchcurtis]

Before I spend any more time on this, is that what you had in mind?

196ae9d

[bjorn]

Before I spend any more time on this, is that what you had in mind?

Yeah, exactly! Two things which come to mind:

• The QPainter argument should be removed.
• To avoid having to update a lot of existing code like tmxviewer.cpp and various places in Tiled, you should keep a version with the current signature that routes the calls to the CellRenderer.

In addition, there is a bunch of logic in CellRenderer that handles the various flipping flags, which would be nice to share as well somehow. But let's do this one step at a time. The CellRenderer also handles rendering of missing tiles and tile collision shapes. We'll have to see how to best handle those things later. :-)

[mitchcurtis]

Is it OK to call CellRenderer::flush() if the overload of OrthogonalRenderer::drawTileLayer() that takes the RenderTileCallback returns early (i.e. nothing was drawn)?

void OrthogonalRenderer::drawTileLayer(QPainter *painter, const TileLayer *layer, const QRectF &exposed) const
{
    CellRenderer renderer(painter, this, layer->effectiveTintColor(), CellRenderer::HexagonalCells);
    auto tileRenderFunction = [&renderer](const Cell &cell, const QPointF &pos, const QSizeF &size) {
        renderer.render(cell, pos, size, CellRenderer::BottomLeft);
    };
    drawTileLayer(layer, tileRenderFunction, exposed);
    renderer.flush();
}

[mitchcurtis]

Almost there, but I have an issue:

55572e6#diff-549a459c3d8ce7cb85758e9418548567R262

Do you know how I can get the tile pos? Converting there feels inefficient and the visual result is also wrong... the map is rendered in an incorrect position...

[bjorn]

Is it OK to call CellRenderer::flush() if the overload of OrthogonalRenderer::drawTileLayer() that takes the RenderTileCallback returns early (i.e. nothing was drawn)?

Sure, then it should exit early:

    if (!mTile)
        return;

However, you can remove that call entirely, since ~CellRenderer() already calls flush().

Do you know how I can get the tile pos? Converting there feels inefficient and the visual result is also wrong... the map is rendered in an incorrect position...

You should not need the tile pos. Just change appendTileData to take the const Cell &cell and const QPointF &screenPos, replacing the local variables it has for this.

[mitchcurtis]

Hmm... I've made those changes you suggested but it still renders weirdly...

[bjorn]

@mitchcurtis Can you please open a pull request? It would make collaboration a bit easier. :)