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

Minimap in a resizing dialog #1516

Merged
merged 20 commits into from Apr 14, 2017

Conversation

Projects
None yet
2 participants
@Acuion
Copy link
Contributor

commented Mar 30, 2017

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

This comment has been minimized.

Copy link
Owner

commented Mar 30, 2017

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.

Acuion added some commits Mar 30, 2017

@Acuion

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2017

  • 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

This comment has been minimized.

Copy link
Owner

commented Mar 30, 2017

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

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2017

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
Copy link
Owner

left a comment

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.

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

This comment has been minimized.

Copy link
@bjorn

bjorn Apr 4, 2017

Owner

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


Q_DECLARE_FLAGS(MiniMapRenderFlags, MiniMapRenderFlag)

static MiniMapRenderer* instance();

This comment has been minimized.

Copy link
@bjorn

bjorn Apr 4, 2017

Owner

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);

This comment has been minimized.

Copy link
@Acuion

Acuion Apr 6, 2017

Author Contributor

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.

class MiniMapRenderer
{
public:
enum MiniMapRenderFlag {

This comment has been minimized.

Copy link
@bjorn

bjorn Apr 4, 2017

Owner

Please call it just RenderFlags.

Q_DECLARE_FLAGS(MiniMapRenderFlags, MiniMapRenderFlag)

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

This comment has been minimized.

Copy link
@bjorn

bjorn Apr 4, 2017

Owner

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.

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

This comment has been minimized.

Copy link
@bjorn

bjorn Apr 4, 2017

Owner

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

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

This comment has been minimized.

Copy link
@bjorn

bjorn Apr 4, 2017

Owner

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 added some commits Apr 6, 2017

@Acuion

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2017

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
Copy link
Owner

left a comment

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.

@@ -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);

This comment has been minimized.

Copy link
@bjorn

bjorn Apr 6, 2017

Owner

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 added some commits Apr 6, 2017

@Acuion

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2017

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.

Acuion added some commits Apr 6, 2017

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

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bjorn

This comment has been minimized.

Copy link
Owner

commented Apr 14, 2017

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.

@Acuion Acuion deleted the Acuion:minimap-in-resize branch Apr 21, 2017

@bjorn

This comment has been minimized.

Copy link
Owner

commented Apr 24, 2017

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.