Allows extending of the polyline #1683

ketanhwr · 2017-08-11T06:52:06Z

TODOs:

Don't stop extending after one point. Keep extending until right-click or escape key is encountered.

bjorn · 2017-08-11T17:46:33Z

src/tiled/editpolygontool.cpp

@@ -566,6 +596,59 @@ void EditPolygonTool::updateMovingItems(const QPointF &pos,
    }
 }

+void EditPolygonTool::addNode(const QPointF &pos,


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

bjorn · 2017-08-11T17:47:15Z

src/tiled/editpolygontool.h

+    void addNode(const QPointF &pos, Qt::KeyboardModifiers modifiers);
+
+    void finishAddingNode();
+


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

bjorn · 2017-08-11T17:47:52Z

src/tiled/editpolygontool.h

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


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

Ablu · 2017-08-18T17:03:10Z

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

ketanhwr · 2017-08-20T20:56:28Z

Wait, I'll rebase this.

bjorn · 2017-08-20T17:32:29Z

src/tiled/createmultipointobjecttool.cpp

        mOverlayPolygonItem->setPolygon(next);
+
+        if (mExtending)


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

bjorn · 2017-08-21T09:27:38Z

src/tiled/createmultipointobjecttool.cpp

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


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.

bjorn · 2017-08-21T09:34:30Z

src/tiled/createobjecttool.h

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


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?

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

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.

Oh, right 🤦‍♂️

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.

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.

Oh, right 🤦‍♂️

bjorn · 2017-08-21T09:37:00Z

src/tiled/mapeditor.cpp

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

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


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.

bjorn

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

bjorn · 2017-08-21T12:14:23Z

src/tiled/createobjecttool.cpp

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


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

bjorn · 2017-08-21T12:14:55Z

src/tiled/createobjecttool.cpp

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

    MapObject *newMapObject = mNewMapObjectItem->mapObject();
+


No point in adding a newline here.

bjorn · 2017-08-21T12:16:04Z

src/tiled/createpolygonobjecttool.cpp

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


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

bjorn · 2017-08-21T12:18:43Z

src/tiled/createpolylineobjecttool.cpp

+    mExtendingFirst = extendingFirst;
+
+    mNewMapObjectItem = mapObjectItem;
+    mOverlayPolygonObject = mapObjectItem->mapObject()->clone();


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.

Extending can be done from first node as well

ketanhwr · 2017-08-29T16:18:34Z

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

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 · 2017-09-06T09:08:05Z

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 · 2017-09-06T09:27:19Z

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

bjorn · 2017-09-06T10:15:42Z

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 · 2017-12-14T13:10:45Z

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

bjorn · 2017-12-14T13:18:15Z

@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 · 2017-12-14T13:36:34Z

Alright 🙂

Conflicts: src/tiled/editpolygontool.cpp

bjorn · 2018-01-12T15:22:43Z

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 · 2018-01-12T16:15:55Z

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.

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 · 2018-01-16T12:30:36Z

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 requested changes Aug 11, 2017

View reviewed changes

ketanhwr force-pushed the extending branch from 107ccd1 to f86ecb6 Compare August 18, 2017 18:30

ketanhwr force-pushed the extending branch from f9e88c1 to 3441ab4 Compare August 20, 2017 21:01

bjorn requested changes Aug 21, 2017

View reviewed changes

bjorn requested changes Aug 24, 2017

View reviewed changes

ketanhwr added 4 commits August 24, 2017 19:10

Allows extending of the polyline

471634d

Extending can be done from first node as well

Review changes

284e302

Override finish and cancel functions

679786b

Review changes

8b0b680

ketanhwr force-pushed the extending branch from 4128372 to 8b0b680 Compare August 24, 2017 13:49

bjorn added 2 commits September 6, 2017 11:01

Fixed new polylines being drawn backwards after extending first

f76249c

The mExtendingFirst variable wasn't being reset.

Fixed extending polyline other than the one just created

0009157

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.

Merge branch 'master' into ketanhwr-extending

c436b53

Conflicts: src/tiled/editpolygontool.cpp

bjorn added 5 commits January 16, 2018 12:57

Fixed behavior when switching tools while extending a polyline

49e6a40

Don't switch to EditPolygonTool when user selected another tool. This isn't what the user wants and it broke stuff.

Some further cleanups

6e2d1f7

Removed another include that doesn't need to be added

046a950

bjorn merged commit 5dc2194 into mapeditor:master Jan 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allows extending of the polyline #1683

Allows extending of the polyline #1683

ketanhwr commented Aug 11, 2017 •

edited

Loading

bjorn Aug 11, 2017

bjorn Aug 11, 2017

bjorn Aug 11, 2017

Ablu commented Aug 18, 2017

ketanhwr commented Aug 20, 2017

bjorn Aug 20, 2017

bjorn Aug 21, 2017

bjorn Aug 21, 2017

ketanhwr Aug 21, 2017

bjorn Aug 21, 2017

ketanhwr Aug 21, 2017

ketanhwr Aug 21, 2017

bjorn Aug 21, 2017

ketanhwr Aug 21, 2017

bjorn Aug 21, 2017

bjorn left a comment

bjorn Aug 21, 2017

bjorn Aug 21, 2017

bjorn Aug 21, 2017

bjorn Aug 21, 2017

ketanhwr commented Aug 29, 2017

bjorn commented Sep 6, 2017

ketanhwr commented Sep 6, 2017

bjorn commented Sep 6, 2017

ketanhwr commented Dec 14, 2017

bjorn commented Dec 14, 2017

ketanhwr commented Dec 14, 2017

bjorn commented Jan 12, 2018

ketanhwr commented Jan 12, 2018

bjorn commented Jan 16, 2018

		void addNode(const QPointF &pos, Qt::KeyboardModifiers modifiers);

		void finishAddingNode();

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

		MapObject *newMapObject = mNewMapObjectItem->mapObject();

Allows extending of the polyline #1683

Allows extending of the polyline #1683

Conversation

ketanhwr commented Aug 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ablu commented Aug 18, 2017

ketanhwr commented Aug 20, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjorn left a 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

ketanhwr commented Aug 29, 2017

bjorn commented Sep 6, 2017

ketanhwr commented Sep 6, 2017

bjorn commented Sep 6, 2017

ketanhwr commented Dec 14, 2017

bjorn commented Dec 14, 2017

ketanhwr commented Dec 14, 2017

bjorn commented Jan 12, 2018

ketanhwr commented Jan 12, 2018

bjorn commented Jan 16, 2018

ketanhwr commented Aug 11, 2017 •

edited

Loading