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

Polygon Editing Tools Improvements #1674

Closed
wants to merge 4 commits into from
Closed

Conversation

@ketanhwr
Copy link
Contributor

@ketanhwr ketanhwr commented Aug 3, 2017

The opacity for preview edge is (1/3)rd of the fixed edges. Here's a screenshot:

screenshot from 2017-08-03 20-30-28

@ketanhwr ketanhwr changed the title Preview last edge while creating polyline Polygon Editing Tools Improvements Aug 4, 2017
@ketanhwr
Copy link
Contributor Author

@ketanhwr ketanhwr commented Aug 4, 2017

I've also added an option to extend a polyline (#157). Here's a gif for demonstration:

extend-poygon


if (mapObject->shape() == MapObject::Polyline) {
QAction *extendPolyline = menu.addAction(tr("Extend Polyline"));
extendPolyline->setEnabled(n == 1 && firstHandle->pointIndex() == mapObject->polygon().size() - 1);
Copy link
Contributor

@Ablu Ablu Aug 4, 2017

I found this boolean expression a bit hard to read... Maybe passing it into a variable could help here (would also shorten the line a bit). But that might be personal taste...

@ketanhwr
Copy link
Contributor Author

@ketanhwr ketanhwr commented Aug 4, 2017

Now, the user can extend a polyline from both the first point and the end point. Also, the user can use right click to cancel extending the polyline.

@ketanhwr
Copy link
Contributor Author

@ketanhwr ketanhwr commented Aug 5, 2017

While 'Extending' a polyline, the user can now choose to close it as well (Basically convert it into a polygon). (#404)

@ketanhwr
Copy link
Contributor Author

@ketanhwr ketanhwr commented Aug 5, 2017

When the user selects two nodes on a polygon which are side-by-side, then the user can delete that edge which will convert that polygon into a polyline.

@ketanhwr
Copy link
Contributor Author

@ketanhwr ketanhwr commented Aug 5, 2017

Conversion between Polygon/Polyline is shown in the gif here 🙂

peek 2017-08-05 17-49

@ketanhwr
Copy link
Contributor Author

@ketanhwr ketanhwr commented Aug 6, 2017

My previous commit closes #1555. Delete key now deletes the selected nodes. If no nodes are selected, then the object is deleted which was previously the standard action.

@Ablu
Copy link
Contributor

@Ablu Ablu commented Aug 6, 2017

Looks nice already

Maybe ESC should cancel the extension process too (not only right click).
And I find it a bit weird that during closing a polyline, the polygon preview is displayed without snapping the previewed point to the starting point.


QPolygonF polygon = mapObject->polygon();

if ((polygon.last() - polygon.first()).manhattanLength() < QApplication::startDragDistance() * 1.0)
Copy link
Contributor

@Ablu Ablu Aug 6, 2017

Maybe introduce a variable for the left side of the < here, to keep the line shorter?

QPolygonF polygon = mapObject->polygon();

if ((polygon.last() - polygon.first()).manhattanLength() < QApplication::startDragDistance() * 1.0) {

Copy link
Contributor

@Ablu Ablu Aug 6, 2017

unneeded empty line

Copy link
Contributor

@Ablu Ablu left a comment

left some quick comments

if (mapObject->shape() == MapObject::Polyline) {
QAction *extendPolyline = menu.addAction(tr("Extend Polyline"));

bool indexCheck = (firstHandle->pointIndex() == mapObject->polygon().size() - 1) ||
Copy link
Contributor

@Ablu Ablu Aug 6, 2017

I find the name indexCheck a bit weird... Maybe handleCanBeExtended?

@@ -479,7 +480,10 @@ void MapEditor::performStandardAction(StandardAction action)
paste(ClipboardManager::PasteInPlace);
break;
case DeleteAction:
MapDocumentActionHandler::instance()->delete_();
if (mEditPolygonTool->countSelectedHandles())
Copy link
Contributor

@Ablu Ablu Aug 6, 2017

I personally do not like these implicit > 0 checks on integer values in this context. I usually prefer to write > 0 here. But not sure what @bjorn. Thinks here...

Copy link
Member

@bjorn bjorn Aug 6, 2017

Well, I think, since this is really checking if there are any selected handles, it should be:

if (mEditPolygonTool->hasSelectedHandles())

And

bool hasSelectedHandles() const { return !mSelectedHandles.isEmpty(); }

Copy link
Member

@bjorn bjorn left a comment

Just two points of feedback, didn't get around to a full review yet.

@@ -479,7 +480,10 @@ void MapEditor::performStandardAction(StandardAction action)
paste(ClipboardManager::PasteInPlace);
break;
case DeleteAction:
MapDocumentActionHandler::instance()->delete_();
if (mEditPolygonTool->countSelectedHandles())
Copy link
Member

@bjorn bjorn Aug 6, 2017

Well, I think, since this is really checking if there are any selected handles, it should be:

if (mEditPolygonTool->hasSelectedHandles())

And

bool hasSelectedHandles() const { return !mSelectedHandles.isEmpty(); }

@@ -441,22 +441,52 @@ void IsometricRenderer::drawMapObject(QPainter *painter,
const QPolygonF polygon = object->polygon().translated(pos);
QPolygonF screenPolygon = pixelToScreenCoords(polygon);

if (screenPolygon.size() == 1)
screenPolygon.push_back(screenPolygon[0]);
Copy link
Member

@bjorn bjorn Aug 6, 2017

Please prefer to use the Qt-style members instead of those provided for STL compatibility, so append instead of push_back and removeLast / removeFirst instead of pop_back / pop_front.

@bjorn
Copy link
Member

@bjorn bjorn commented Aug 6, 2017

In general, I think this pull request is too much of a collection of different issues. It would really help if separate pull requests were opened for things that aren't directly related to each other.

@ketanhwr
Copy link
Contributor Author

@ketanhwr ketanhwr commented Aug 6, 2017

In general, I think this pull request is too much of a collection of different issues. It would really help if separate pull requests were opened for things that aren't directly related to each other.

So should I close this PR, and open separate PRs for different issues? :|

@bjorn
Copy link
Member

@bjorn bjorn commented Aug 6, 2017

So should I close this PR, and open separate PRs for different issues? :|

I'd say keep this one for "reduced opacity of last edge" and "add option to extend polyline" (including from first node and right-click to cancel). And create separate PRs for:

  • Added action to delete polygon edge
  • Added option to close a polyline
  • Delete key now deletes selected nodes

Unless inter-dependent code makes this very difficult.

ketanhwr added 4 commits Aug 6, 2017
Extending can be started from both first point and last point. Escape and Delete Key can be used to cancel the extending.
)

Need to select two consecutive nodes in a polygon to delete an edge.
Copy link
Member

@bjorn bjorn left a comment

Don't forget to initialize mComplete and mLastEdgeIncomplete.

In trying this out, I'm not convinced that rendering the new edge at lower opacity is currently helpful. All it does for me is to make it less visible.

Regarding the extending of the polyline, I'm wondering why it works only for a single segment. After extending by one segment, I think it should automatically allow me to extend with another segment, until I decide to press Enter or right-click, like it is when creating a new polyline.

Also, if we snap to the other end of the polyline to create a polygon, then I'd expect this snapping to be also available when creating a new polyline. Essentially implementing both parts of issue #404.

In general, I still think all these enhancements are quite independent and it would be better to do it as separate pull requests. It's easier to provide targeted feedback that way and some parts could be merged, like the handling of the Delete key.

QPolygonF polygon = mapObject->polygon();
qreal distance = (polygon.last() - polygon.first()).manhattanLength();

if (distance < QApplication::startDragDistance() * 1.0)
Copy link
Member

@bjorn bjorn Aug 10, 2017

What's the point of multiplying by 1.0?

Also, distance should be measured in screen pixels. Unfortunately, there is no way to determine the screen coordinates for a polygon point without a reference to the MapView, but for now we should at least use renderer->pixelToScreenCoords.

Finally, when the snapping triggers, it should change pixelCoords to match the point to which it was snapped.

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

void EditPolygonTool::addNode(const QPointF &pos,
Copy link
Member

@bjorn bjorn Aug 10, 2017

The name addNode does not seem to reflect what this function is doing.


QPolygonF polygon = mapObject->polygon();

if (mapObject->shape() == MapObject::Polygon)
Copy link
Member

@bjorn bjorn Aug 10, 2017

There is no point in this check, is there?

@ketanhwr
Copy link
Contributor Author

@ketanhwr ketanhwr commented Aug 10, 2017

Also, if we snap to the other end of the polyline to create a polygon, then I'd expect this snapping to be also available when creating a new polyline. Essentially implementing both parts of issue #404.

So basically, merge both the tools into a single tool ?

@ketanhwr
Copy link
Contributor Author

@ketanhwr ketanhwr commented Aug 10, 2017

In general, I still think all these enhancements are quite independent and it would be better to do it as separate pull requests. It's easier to provide targeted feedback that way and some parts could be merged, like the handling of the Delete key.

Alright, then I'll close the current pull request.

@ketanhwr ketanhwr closed this Aug 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants