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

Added Move selection Feature #1607

Closed
wants to merge 17 commits into from
Closed

Conversation

@ketanhwr
Copy link
Contributor

@ketanhwr ketanhwr commented Jun 10, 2017

This addresses #650.

It works fine as of now, but I need feedback regarding the flow of this tool. You need to select an area with the tile selection tool, and then select this tool and then move the selection to someplace else. I have a few doubts in mind regarding the flow but I think a feedback would be great!

@Ablu
Copy link
Contributor

@Ablu Ablu commented Jun 10, 2017

A few points I realized while testing:

  • When starting to drag, but still staying on the same tile (so only drag like a pixel to the left or right), the whole map is overlayed with the light blue/gray
  • Sometimes I get a situation like this:
    screenshot from 2017-06-10 17-22-20
    The lower row is nothing what I selected... seems to be a small graphical glitch... It seems to preview one tile to much in width and height. Also the display of those lines seems to lag behind while dragging...

Generally I think the flow of the tool is fine... The only thing which could be annoying is that there is no thing like a floating layer like in Gimp... So if you move tiles on crowded layers you might accidently delete / override tiles...

Copy link
Contributor

@Ablu Ablu left a comment

Did a very undetailed review. I guess @bjorn needs to check whether the approach is correct.

void MoveSelectionTool::tilePositionChanged(const QPoint &pos)
{
if (mDragging) {

This comment has been minimized.

@Ablu

Ablu Jun 10, 2017
Contributor

unneeded empty line

TileLayer *tileLayer = dynamic_cast<TileLayer*>(currentLayer);
const QRegion &selectedArea = mapDocument()->selectedArea();

ClipboardManager::instance()->copySelection(mapDocument());

This comment has been minimized.

@Ablu

Ablu Jun 10, 2017
Contributor

I still think that there should be no ClipboardManager involved here... One would not expect to loose the current clipboard content when using the tool. It should be possible to store the moved selection in the tool.

This comment has been minimized.

@Ablu

Ablu Jun 10, 2017
Contributor

Can't you simply use the mPreviewLayer variable for this?

This comment has been minimized.

@ketanhwr

ketanhwr Jun 10, 2017
Author Contributor

Oh yeah. I didn't realise that, I'll look into this!

if (map)
tilesetManager->removeReferences(map->tilesets());

brushItem()->clear();

This comment has been minimized.

@Ablu

Ablu Jun 10, 2017
Contributor

I think the preview layer should be cleared here too.

@ketanhwr
Copy link
Contributor Author

@ketanhwr ketanhwr commented Jun 12, 2017

@Ablu: The clipboard is no longer needed now. I've used the preview layer to paint only. There are still a few bugs.

So if you move tiles on crowded layers you might accidently delete / override tiles...

This can be solved by: Whenever the tool is selected, the selected area remains same, as in, even if it overrides the tiles when you move the selection, it won't actually delete them unless you choose some other tool. I will have to save the preview layer when the tool is first activated and paint when the tool is deactivated.

@ketanhwr
Copy link
Contributor Author

@ketanhwr ketanhwr commented Jun 12, 2017

@Ablu, I've probably fixed all the issues that you've mentioned.

Sometimes I get a situation like this:

I'm not able to reproduce it, it might as well be a graphical error only.

Copy link
Member

@bjorn bjorn left a comment

Yep, this tool is definitely full of challenges. I've tried it out and inspected the code, and here's my feedback. :-)

void MoveSelectionTool::activate(MapScene *scene)
{
AbstractTileTool::activate(scene);
cut();

This comment has been minimized.

@bjorn

bjorn Jun 13, 2017
Member

I think you should wait with the cutting until the user starts to drag the selection.

}

mLastUpdate = pos;
}

This comment has been minimized.

@bjorn

bjorn Jun 13, 2017
Member

I think you should refresh the cursor also in this function, and change it to a hand when the user will start a drag operation (if the position is in the selected area). This makes it easier to anticipate the action done when clicking. See also the change of cursor when hovering an object.

QRegion selectedArea = mapDocument()->selectedArea();
selectedArea.translate(offset);

mapDocument()->undoStack()->push(new ChangeSelectedArea(mapDocument(), selectedArea));

This comment has been minimized.

@bjorn

bjorn Jun 13, 2017
Member

This is not a good idea since it is part of the undo history. I think the selection should be cleared when you start to drag (as part of the cut, essentially), at which point the brush item will take over the highlighting of the floating tiles. Once the move operation is done, the selection can stay cleared.


if (mPreviewLayer) {
mPreviewLayer->setX(mPreviewLayer->x()+offset.x());
mPreviewLayer->setY(mPreviewLayer->y()+offset.y());

This comment has been minimized.

@bjorn

bjorn Jun 13, 2017
Member

Coding style: please put spaces around the + operator.


void MoveSelectionTool::mouseReleased(QGraphicsSceneMouseEvent *)
{
if (mDragging) {

This comment has been minimized.

@bjorn

bjorn Jun 13, 2017
Member

You'll probably want to ignore all but the left mouse button here.


void activate(MapScene *scene) override;
void deactivate(MapScene *scene) override;

This comment has been minimized.

@bjorn

bjorn Jun 13, 2017
Member

You'll want to override the default implementations of mouseEntered and mouseLeft, since they're showing/hiding the preview item, which is not what you want for this tool.

TileLayer *tileLayer = dynamic_cast<TileLayer*>(currentLayer);
const QRegion &selectedArea = mapDocument()->selectedArea();

TileLayer *brushLayer = tileLayer->copy(tileLayer->bounds());

This comment has been minimized.

@bjorn

bjorn Jun 13, 2017
Member

You should not copy the entirely layer (and Layer::bounds is technically not in the right coordinate space to use as a parameter to Layer::copy, though it won't run into issues with layers at 0,0 which is all layers at the moment). Essentially, I think you want:

mPreviewLayer = SharedTileLayer(tileLayer->copy(selectedArea.translated(-tileLayer->position())));

And since this only copied the relevant region, you'll need to set the initial position:

mPreviewLayer->setPosition(selectedArea.boundingRect().topLeft());
mPreviewLayer = SharedTileLayer(brushLayer);

brushItem()->setTileLayer(mPreviewLayer);
brushItem()->setTileRegion(mapDocument()->selectedArea());

This comment has been minimized.

@bjorn

bjorn Jun 13, 2017
Member

Please use the version of setTileLayer that also takes a region, otherwise setTileLayer will first compute the region from the tile layer, which is a waste of time:

brushItem()->setTileLayer(mPreviewLayer, selectedArea);
brushItem()->setTileRegion(mapDocument()->selectedArea());

QUndoStack *stack = mapDocument()->undoStack();
stack->beginMacro(tr("Move Selection"));

This comment has been minimized.

@bjorn

bjorn Jun 13, 2017
Member

I think it's a bad idea to leave this macro open, since it will be hard to ensure that it is eventually ended, and even harder to make sure that no other changes are pushed to the stack in between. Maybe it's better just to end this macro here and deal with the fact that the user may undo after starting a move operation.

mPreviewLayer->setX(mPreviewLayer->x()+offset.x());
mPreviewLayer->setY(mPreviewLayer->y()+offset.y());
brushItem()->setTileLayer(mPreviewLayer);
brushItem()->setTileRegion(mapDocument()->selectedArea());

This comment has been minimized.

@bjorn

bjorn Jun 13, 2017
Member

Please use the version of setTileLayer that takes the region as second parameter. Also, since we shouldn't rely on the selected area, you could instead translate the current tile region by the offset as well: brushItem()->tileRegion().translated(offset).

ketanhwr added 5 commits Jun 13, 2017
@ketanhwr
Copy link
Contributor Author

@ketanhwr ketanhwr commented Jun 13, 2017

@bjorn I've made the changes, have a look now 🙂

Copy link
Member

@bjorn bjorn left a comment

I've provided some minor inline comments, but here's the big suggestion:

You need to select an area with the tile selection tool, and then select this tool and then move the selection to someplace else. I have a few doubts in mind regarding the flow but I think a feedback would be great!

What do you think about trying to merge the new Move tool with the selection tools? Since we have rectangular select, magic wand and select-same-tile, I imagine we could introduce AbstractSelectionTool, which would be used to add the ability to drag the selected tiles to all selection tools. In GIMP, you hold Ctrl+Alt modifiers while dragging to trigger this move behavior, and I think we could do the same in Tiled.

The current behavior is quite good (except, I think clicking outside of the preview should still perform the paste operation), but I also have my doubts about this switching between tools.

const int dragDistance = (mMouseScreenStart - screenPos).manhattanLength();

if (dragDistance >= QApplication::startDragDistance() / 2) {
if (!mCut) {

This comment has been minimized.

@bjorn

bjorn Jun 15, 2017
Member

I guess you don't actually need mCut, since you could use if (!mPreviewLayer) instead.

if (event->button() != Qt::LeftButton)
return;

if (mDragging) {

This comment has been minimized.

@bjorn

bjorn Jun 15, 2017
Member

No need for this condition.

QUndoStack *stack = mapDocument()->undoStack();
stack->beginMacro(tr("Move Selection"));

if (!selectedArea.isEmpty()) {

This comment has been minimized.

@bjorn

bjorn Jun 15, 2017
Member

Actually the selected area can't be empty when cut is called, right? Maybe it's a good idea to make sure anyway, but then this check should be moved up.

@ketanhwr ketanhwr closed this Jul 10, 2017
@bjorn
Copy link
Member

@bjorn bjorn commented Jul 10, 2017

Just to add an explanatory note: this PR was closed because @ketanhwr introduced an AbstractTileSelectionTool in #1620 and will now merge the functionality of the Move Selection tool added here with that class.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.