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

Allows extending of the polyline #1683

Merged
merged 12 commits into from Jan 16, 2018
Merged

Allows extending of the polyline #1683

merged 12 commits into from Jan 16, 2018

Conversation

@ketanhwr
Copy link
Contributor

@ketanhwr ketanhwr commented Aug 11, 2017

#157

TODOs:

  • Don't stop extending after one point. Keep extending until right-click or escape key is encountered.
@@ -566,6 +596,59 @@ void EditPolygonTool::updateMovingItems(const QPointF &pos,
}
}

void EditPolygonTool::addNode(const QPointF &pos,

This comment has been minimized.

@bjorn

bjorn Aug 11, 2017
Member

The name addNode does not seem to reflect what the function is doing. I think updateAddingNode would be better, but it still leaves two kinds of terminology for the same thing. Since you're already using the mode Extending and have mExtendingFirst, I would suggest to rename all the other functions as follows:

  • updateExtending
  • finishExtending
  • cancelExtending
void addNode(const QPointF &pos, Qt::KeyboardModifiers modifiers);

void finishAddingNode();

This comment has been minimized.

@bjorn

bjorn Aug 11, 2017
Member

Coding style: I'd suggest to leave out these empty lines so that the related functions are grouped together.

@@ -98,6 +108,7 @@ private slots:
QPointF mStart;
QPoint mScreenStart;
Qt::KeyboardModifiers mModifiers;
bool mExtendingFirst;

This comment has been minimized.

@bjorn

bjorn Aug 11, 2017
Member

I know this variable isn't expected to be used before being set, but please anyway initialize the variable.

@Ablu
Copy link
Contributor

@Ablu Ablu commented Aug 18, 2017

Hm. Could you rebase this instead of merging the master? For example you reordered the WangTool declaration as it is.

@ketanhwr ketanhwr force-pushed the ketanhwr:extending branch from 107ccd1 to f86ecb6 Aug 18, 2017
@ketanhwr
Copy link
Contributor Author

@ketanhwr ketanhwr commented Aug 20, 2017

Wait, I'll rebase this.

@ketanhwr ketanhwr force-pushed the ketanhwr:extending branch from f9e88c1 to 3441ab4 Aug 20, 2017
mOverlayPolygonItem->setPolygon(next);

if (mExtending)

This comment has been minimized.

@bjorn

bjorn Aug 21, 2017
Member

Coding style: since the body wraps multiple lines, this should have braces.

@@ -75,15 +82,24 @@ void CreateMultipointObjectTool::mousePressedWhileCreatingObject(QGraphicsSceneM
QPolygonF next = mOverlayPolygonObject->polygon();

// If the last position is still the same, ignore the click
if (next.last() == current.last())
if (next.last() == current.last() && !mExtending)

This comment has been minimized.

@bjorn

bjorn Aug 21, 2017
Member

Why is this check irrelevant while extending? I think it was introduced to avoid inserting overlapping points in case the user double-clicks. It seems to me like it would be useful to check the first or last point, depending on mExtendingFirst.

@@ -66,6 +69,8 @@ class CreateObjectTool : public AbstractObjectTool
ObjectGroupItem *mObjectGroupItem;
MapObjectItem *mNewMapObjectItem;
MapObjectItem *mOverlayPolygonItem;
bool mExtending;
bool mExtendingFirst;

This comment has been minimized.

@bjorn

bjorn Aug 21, 2017
Member

You can't in general extend an object so I'd prefer these variables to be in the CreateMultipointObjectTool. At least mExtendingFirst could easily be moved there since it isn't used in the base class. Regarding mExtending, are there any functions that couldn't be just overridden?

This comment has been minimized.

@ketanhwr

ketanhwr Aug 21, 2017
Author Contributor

I tried overriding the functions cancelNewMapObject() and finishNewMapObject(), but apparently, the parent function gets called only and not the child one.

This comment has been minimized.

@bjorn

bjorn Aug 21, 2017
Member

That makes no sense, so I quickly searched where these functions are being called.

Turns out it's because the CreatePolylineObjectTool is explicitly calling the functions of the CreateObjectTool class here:

void CreatePolylineObjectTool::finishNewMapObject()
{
    if (mNewMapObjectItem->mapObject()->polygon().size() >= 2)
        CreateObjectTool::finishNewMapObject();
    else
        CreateObjectTool::cancelNewMapObject();
}

I see no reason why it should be doing that. There are no existing overrides that would need to be skipped. So please just remove the CreateObjectTool:: part there.

This comment has been minimized.

@ketanhwr

ketanhwr Aug 21, 2017
Author Contributor

Oh, right 🤦‍♂️

This comment has been minimized.

@ketanhwr

ketanhwr Aug 21, 2017
Author Contributor

I removed CreateObjectTool:: but added CreateMultipointObjectTool::, because without any className::, their was some runtime error (the window wasn't responding at all). I wasn't able to understand why this happened.

This comment has been minimized.

@bjorn

bjorn Aug 21, 2017
Member

Well, I missed it too, but this is obviously because we're in CreatePolylineObjectTool::finishNewMapObject there, so we should at least avoid calling that exact function recursively.

This comment has been minimized.

@ketanhwr

ketanhwr Aug 21, 2017
Author Contributor

Oh, right 🤦‍♂️

@@ -673,6 +677,17 @@ void MapEditor::setWangFill(bool value)
mBucketFillTool->setWangFill(value);
}

void MapEditor::extend(MapObjectItem *mapObjectItem, bool extendingFirst)
{
setSelectedTool(mPolylineObjectsTool);

This comment has been minimized.

@bjorn

bjorn Aug 21, 2017
Member

Using this instead of mToolManager->selectTool(mPolylineObjectsTool) makes the tool switch without this being visible on the "Tools" tool bar. Was that a conscious decision? I think it wouldn't hurt for the user to see the active tool changing. Same for extendingFinished.

Copy link
Member

@bjorn bjorn left a comment

It looks like I got interrupted since I just found these comments still pending. :-/

@@ -88,7 +88,6 @@ void CreateObjectTool::keyPressed(QKeyEvent *event)
case Qt::Key_Escape:
if (mNewMapObjectItem) {
cancelNewMapObject();
return;

This comment has been minimized.

@bjorn

bjorn Aug 24, 2017
Member

Please don't remove this return, since it will cause the event to be ignored.

@@ -203,6 +202,7 @@ void CreateObjectTool::finishNewMapObject()
}

MapObject *newMapObject = mNewMapObjectItem->mapObject();

This comment has been minimized.

@bjorn

bjorn Aug 24, 2017
Member

No point in adding a newline here.

@@ -52,7 +52,7 @@ MapObject *CreatePolygonObjectTool::createNewMapObject()
void CreatePolygonObjectTool::finishNewMapObject()
{
if (mNewMapObjectItem->mapObject()->polygon().size() >= 3)
CreateObjectTool::finishNewMapObject();
finishNewMapObject();

This comment has been minimized.

@bjorn

bjorn Aug 24, 2017
Member

Should be CreateMultipointObjectTool::finishNewMapObject(); due to the problem you noticed in the polyline object tool.

mExtendingFirst = extendingFirst;

mNewMapObjectItem = mapObjectItem;
mOverlayPolygonObject = mapObjectItem->mapObject()->clone();

This comment has been minimized.

@bjorn

bjorn Aug 24, 2017
Member

The overlay polygon object is created already in the constructor of the CreateMultipointObjectTool, so you can't just assign another object to it (causes leaking of the other object). I think you could just use the object that is already there, since it's only for showing a polygon preview anyway.

@ketanhwr ketanhwr force-pushed the ketanhwr:extending branch from 4128372 to 8b0b680 Aug 24, 2017
@ketanhwr
Copy link
Contributor Author

@ketanhwr ketanhwr commented Aug 29, 2017

After we start extending the polyline, since we've shifted to the Create Polyline Tool, the color changes back to blue. Is this fine?

bjorn added 2 commits Sep 6, 2017
The mExtendingFirst variable wasn't being reset.
The shape and position were not being set on the overlay object when
extending a polyline, so it only worked when those were still set from
the last created polyline.
@bjorn
Copy link
Member

@bjorn bjorn commented Sep 6, 2017

After we start extending the polyline, since we've shifted to the Create Polyline Tool, the color changes back to blue. Is this fine?

I hoped it may be fine, but while checking it out it felt strange and it'd probably be better for just the new segment to be blue.

I've pushed two bugfixes for things I noticed while testing.

In addition to not coloring the whole line blue, I think we need to find a way to have the new segment show up immediately. Currently, you have to move your mouse at least once after clicking "Extend Polyline" to make the new segment show up.

@ketanhwr
Copy link
Contributor Author

@ketanhwr ketanhwr commented Sep 6, 2017

So probably, when the user clicks on the first/last node, the extending should begin?

@bjorn
Copy link
Member

@bjorn bjorn commented Sep 6, 2017

So probably, when the user clicks on the first/last node, the extending should begin?

Well, then it would work mostly like Vectr, right? But if it's just activated on click, then we can't switch tools because we'd need it to not interfere with just trying to select some nodes.

Or, it would need to work like Inkscape, where the nodes to click for extending the polyline are only visible in the "Insert Polyline" tool.

Or we find some way to place the new node based on the current mouse position before we get the next mouse move event.

@ketanhwr
Copy link
Contributor Author

@ketanhwr ketanhwr commented Dec 14, 2017

@bjorn what exactly was wrong here? I'd like to get this one merged.

@bjorn
Copy link
Member

@bjorn bjorn commented Dec 14, 2017

@ketanhwr The main issues I seem to have ran into was that you need to move the mouse for the line to show up when extending the polyline, and that the whole thing turns blue rather than just the new line.

For the overal implementation, I'd have to take a detailed look again and consider alternatives, but right now I'm trying to finish some stuff people have been paying for. Either this week or the next I'm also going to look at making some improvements to the polygon editing in general.

@ketanhwr
Copy link
Contributor Author

@ketanhwr ketanhwr commented Dec 14, 2017

Alright 🙂

Conflicts:
	src/tiled/editpolygontool.cpp
@bjorn
Copy link
Member

@bjorn bjorn commented Jan 12, 2018

Hmm, I realized another problem with this implementation. Since it goes through the MapEditor, the "Extend Polyline" action does nothing when used in the Tile Collision Editor, unless we duplicate the same connection setup in the TileCollisionDock.

I'm considering a rather big rewrite of the whole polygon editing functionality, or at least to re-think it all from scratch based on the wishlist of functionality, because of the difficulties with putting such small functions on top of the current implementation.

@ketanhwr
Copy link
Contributor Author

@ketanhwr ketanhwr commented Jan 12, 2018

I'm considering a rather big rewrite of the whole polygon editing functionality, or at least to re-think it all from scratch based on the wishlist of functionality, because of the difficulties with putting such small functions on top of the current implementation.

Interested in helping with this.

bjorn added 5 commits Jan 16, 2018
This way, the "Extend Polyline" action also works in the Tile Collision
Editor.

Also, it no longer appears in the Template view, because with the
current implementation it can't work there since the "Insert Polyline"
tool is not available there.
Don't switch to EditPolygonTool when user selected another tool. This
isn't what the user wants and it broke stuff.
There appears to be no difference between finishing or canceling the
extending of a polyline. In both cases we switch back to the "Edit
Polygons" tool and in both cases we should also make sure the object we
were extending is selected.

There shouldn't actually be a reason to touch the selection at all when
starting or stopping to extend a polyline, but currently the selection
outline does not update as expected while extending a polyline.
@bjorn
Copy link
Member

@bjorn bjorn commented Jan 16, 2018

A few more fixes later, I think we can merge this, but I'll still be looking at redoing the general setup.

Interested in helping with this.

That is appreciated, but I'm under some time pressure to improve this aspect of Tiled and I don't currently see how we can do this together.

@bjorn bjorn merged commit 5dc2194 into mapeditor:master Jan 16, 2018
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
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.