Minimap in a resizing dialog #1516

Acuion · 2017-03-30T00:03:09Z

I think it is useful to allow a user to see what exactly is going to be removed while resizing and visually estimate the proportions.

Rendering of a minimap is now located in a singleton, which can be accessed from both an editor and a resizing window. Maybe it is not a perfect architecture solution, but at lest it works. Waiting for a feedback regarding it.
I think now there is no reason to scale the preview when the old rect goes outside the view, so a user can see a more detailed picture of a map.

bjorn · 2017-03-30T12:54:19Z

This is something I have wanted to do for years, but somehow never prioritized it over other stuff. I shortly tried it out and it's really cool! I'll try to get around to a detailed code review in the evening, or at least in the coming days.

I think now there is no reason to scale the preview when the old rect goes outside the view, so a user can see a more detailed picture of a map.

I'm not sure about this. It depends on whether it's more important for the user to see the remaining piece of the map, or what part he is cutting away. If the remaining piece is what he wants to focus on, it should be generally easier to select that area and use the "Crop to Selection" action. So in general I'm inclined to think it's better to make sure the whole map is visible, even if it will be smaller as a result.

…s' checkbox

Acuion · 2017-03-30T14:54:41Z

A preview behavior was strange if a new size was smaller than an old one. Now in that case it doesn't scale and a user can select a new rect by moving a frame.
Objects were not offseted if the "Remove objects outside of the map" was not set

bjorn · 2017-03-30T15:23:57Z

Objects were not offseted if the "Remove objects outside of the map" was not set

Nice catch! Can you open a separate pull request for the this bug, to the 0.18 branch? It's a nice catch and if we do a 0.18.3 release I'd like to include it there.

A preview behavior was strange if a new size was smaller than an old one. Now in that case it doesn't scale and a user can select a new rect by moving a frame.

I don't notice strange behavior currently in Tiled, so I assume this was about this pull request?

Acuion · 2017-03-30T15:32:23Z

I don't notice strange behavior currently in Tiled, so I assume this was about this pull request?

Oh, yes, it becomes strange to me only when a minimap is inserted.

Nice catch! Can you open a separate pull request for the this bug, to the 0.18 branch? It's a nice catch and if we do a 0.18.3 release I'd like to include it there.

Ok, is it still needed here?

bjorn

A preview behavior was strange if a new size was smaller than an old one. Now in that case it doesn't scale and a user can select a new rect by moving a frame.

I think the behavior is still unexpected and would prefer you reverted the changes made to the mouse handling and scale calculations. The original behavior was intentional, and you can see this also in the same kind of widget in GIMP when going to Image -> Canvas Size.

I've added some inline comments regarding the implementation.

bjorn · 2017-04-04T10:48:05Z

src/tiled/minimaprenderer.cpp

@@ -0,0 +1,126 @@
+#include "minimaprenderer.h"


Please don't forget to copy the license header from minimap.cpp.

bjorn · 2017-04-04T11:28:14Z

src/tiled/minimaprenderer.h

+
+    Q_DECLARE_FLAGS(MiniMapRenderFlags, MiniMapRenderFlag)
+
+    static MiniMapRenderer* instance();


I doesn't makes sense to have the MiniMapRenderer as a singleton, since it doesn't store any state that would be useful to keep around. Instead, please just allocate it on the stack where you need it, passing the MapDocument* to its constructor instead of having a setMapDocument method:

MiniMapRenderer renderer(mMapDocument); renderer.renderToImage(image, flags);

The problem is that ResizeHelper has no mMapDocument. That's why I've implemented the renderer as a singleton.
Or is it better to somehow pass it to ResizeHelper? I think it is more complicated than the singleton approach.

bjorn · 2017-04-04T11:30:07Z

src/tiled/minimaprenderer.h

+class MiniMapRenderer
+{
+public:
+    enum MiniMapRenderFlag {


Please call it just RenderFlags.

bjorn · 2017-04-04T11:30:32Z

src/tiled/minimaprenderer.h

+    Q_DECLARE_FLAGS(MiniMapRenderFlags, MiniMapRenderFlag)
+
+    static MiniMapRenderer* instance();
+    void renderMinimapToImage(QImage& image, const MiniMapRenderFlags minimapRenderFlags) const;


You can call this renderToImage since it's clear that it renders a mini-map.

You can remove the const on the flags parameter (please also shorten its name). It has no meaning since the parameter is passed by value.

bjorn · 2017-04-04T11:35:48Z

src/tiled/minimaprenderer.h

+/*
+ * minimaprenderer.h
+ * Copyright 2017, Yuriy Natarov <natarur@gmail.com>
+ * Based on a minimap by Christoph Schnackenberg and Thorbjørn Lindeijer


Please just copy the copyright lines from minimap.h instead of writing a custom message.

bjorn · 2017-04-04T11:39:55Z

src/tiled/resizehelper.cpp

+    mMiniMapRenderer->renderMinimapToImage(minimap, MiniMapRenderer::DrawObjects
+                                           | MiniMapRenderer::DrawImages
+                                           | MiniMapRenderer::DrawTiles
+                                           | MiniMapRenderer::IgnoreInvisibleLayer);


Rendering the mini-map on each paint event is too slow for large maps. Instead, please keep the image around and only repaint it when the scale changes.

Ideally, we'd do the rendering in a thread. I think we can easily do that here, because the resize helper dialog is modal so there can't be any changes to the underlying data in the meantime. Up to you whether you want to look into that or leave it for later.

Acuion · 2017-04-06T13:47:20Z

Done, but I personally think that my approach was better because currently, in case of a map shrinking, a lot of the space is unused and the minimap become smaller, so as a profit from its usage here.

bjorn

The problem is that ResizeHelper has no mMapDocument. That's why I've implemented the renderer as a singleton.
Or is it better to somehow pass it to ResizeHelper? I think it is more complicated than the singleton approach.

Hmm, right, I hadn't thought of that. I'm not entirely happy with the singleton solution though. I'll need to find some time to consider other solutions.

Done, but I personally think that my approach was better because currently, in case of a map shrinking, a lot of the space is unused and the minimap become smaller, so as a profit from its usage here.

While you're right that the image is smaller that it'd need to be, I think it is confusing when you for example scale a 100x100 map to 50x200 map, that in one direction the mouse drags the existing map and in the other direction it drags the preview of the new area.

bjorn · 2017-04-06T20:49:03Z

src/tiled/resizehelper.cpp

@@ -179,7 +177,13 @@ void ResizeHelper::recalculateScale()
    // Pick the smallest scale
    const double scaleW = _size.width() / (double) width;
    const double scaleH = _size.height() / (double) height;
-    mScale = (scaleW < scaleH) ? scaleW : scaleH;
+    mScale = qMin(scaleW, scaleH);


That the scale is recalculated doesn't mean it'll change, so please still compare with the previous scale and don't recreate the image when it isn't necessary.

Acuion · 2017-04-06T22:31:37Z

While you're right that the image is smaller that it'd need to be, I think it is confusing when you for example scale a 100x100 map to 50x200 map, that in one direction the mouse drags the existing map and in the other direction it drags the preview of the new area.

How about zooming as a compromise solution? It allows a user to switch between a general and a detailed picture.

Waiting for your ideas regarding the singleton.

bjorn · 2017-04-14T14:19:29Z

I like the zooming! It's rather hidden, but indeed this way it can help people who want to see more detail.

About the singleton, I struggled a bit to resolve it myself, and then rather than explaining how I would like it to be solved, I decided to just push that in a follow-up change myself. See commit 62a89e1. It may make the code a little more convoluted, especially since I needed to forward the renderer callback from the ResizeDialog, but it keeps things more cleanly separated.

Thanks for the nice improvement! I do hope that before release either me or somebody else will get around to making it render the image in a thread so that the UI stays more responsive when resizing larger maps.

bjorn · 2017-04-24T12:02:24Z

Not sure why I didn't think if this before, but I noticed today that the implementation as such only works for orthogonal maps using square tiles. I've opened issue #1554 about extending the functionality to other map configurations.

Acuion and others added 7 commits March 30, 2017 01:34

minimap in map resizing

a5de90a

.pro

101a9c8

includes

3d33024

extra line rem

d70c13d

disable map scaling when the old rect goes outside the view

471532f

removing of useless vars

4766a32

nullptr check

16b3634

Acuion and others added 4 commits March 30, 2017 16:12

singleton fixes

9341110

rects behaviour

b22844e

bugfix: objects were not offseted without checking the 'remove object…

59e5d9e

…s' checkbox

min/max in a Qt way

042521a

codestyle and a destructor

de36810

A comment poision

c069d9a

bjorn requested changes Apr 4, 2017

View reviewed changes

Acuion added 3 commits April 6, 2017 10:51

refactoring

2bde26f

Minimap renders only on scale change

afe33d0

Revert of modified rects behaviour

bf1cc4c

bjorn requested changes Apr 6, 2017

View reviewed changes

Acuion added 2 commits April 7, 2017 00:34

Scale check

a1997dd

zooming

8a3a231

Acuion added 2 commits April 7, 2017 01:33

Init fix

5848972

Singleton as a reference

156125f

bjorn merged commit 245c013 into mapeditor:master Apr 14, 2017

Acuion deleted the minimap-in-resize branch April 21, 2017 06:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minimap in a resizing dialog #1516

Minimap in a resizing dialog #1516

Acuion commented Mar 30, 2017 •

edited

Loading

bjorn commented Mar 30, 2017

Acuion commented Mar 30, 2017

bjorn commented Mar 30, 2017

Acuion commented Mar 30, 2017 •

edited

Loading

bjorn left a comment •

edited

Loading

bjorn Apr 4, 2017

bjorn Apr 4, 2017

Acuion Apr 6, 2017

bjorn Apr 4, 2017

bjorn Apr 4, 2017

bjorn Apr 4, 2017

bjorn Apr 4, 2017

Acuion commented Apr 6, 2017

bjorn left a comment

bjorn Apr 6, 2017

Acuion commented Apr 6, 2017

bjorn commented Apr 14, 2017

bjorn commented Apr 24, 2017


		Q_DECLARE_FLAGS(MiniMapRenderFlags, MiniMapRenderFlag)

		static MiniMapRenderer* instance();

Minimap in a resizing dialog #1516

Minimap in a resizing dialog #1516

Conversation

Acuion commented Mar 30, 2017 • edited Loading

bjorn commented Mar 30, 2017

Acuion commented Mar 30, 2017

bjorn commented Mar 30, 2017

Acuion commented Mar 30, 2017 • edited Loading

bjorn left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Acuion commented Apr 6, 2017

bjorn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Acuion commented Apr 6, 2017

bjorn commented Apr 14, 2017

bjorn commented Apr 24, 2017

Acuion commented Mar 30, 2017 •

edited

Loading

Acuion commented Mar 30, 2017 •

edited

Loading

bjorn left a comment •

edited

Loading