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
base: master
from

Conversation

Projects
None yet
3 participants
@ketanhwr
Contributor

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 from Preview last edge while creating polyline to Polygon Editing Tools Improvements Aug 4, 2017

@ketanhwr

This comment has been minimized.

Show comment
Hide comment
@ketanhwr

ketanhwr Aug 4, 2017

Contributor

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

extend-poygon

Contributor

ketanhwr commented Aug 4, 2017

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

extend-poygon

@ketanhwr

This comment has been minimized.

Show comment
Hide comment
@ketanhwr

ketanhwr Aug 4, 2017

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@ketanhwr

ketanhwr Aug 5, 2017

Contributor

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

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@ketanhwr

ketanhwr Aug 5, 2017

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@ketanhwr

ketanhwr Aug 5, 2017

Contributor

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

peek 2017-08-05 17-49

Contributor

ketanhwr commented Aug 5, 2017

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

peek 2017-08-05 17-49

@ketanhwr

This comment has been minimized.

Show comment
Hide comment
@ketanhwr

ketanhwr Aug 6, 2017

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@Ablu

Ablu Aug 6, 2017

Contributor

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.

Contributor

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.

@Ablu

left some quick comments

Show outdated Hide outdated src/tiled/editpolygontool.cpp
Show outdated Hide outdated src/tiled/mapeditor.cpp
@bjorn

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

Show outdated Hide outdated src/tiled/mapeditor.cpp
Show outdated Hide outdated src/libtiled/isometricrenderer.cpp
@bjorn

This comment has been minimized.

Show comment
Hide comment
@bjorn

bjorn Aug 6, 2017

Owner

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.

Owner

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

This comment has been minimized.

Show comment
Hide comment
@ketanhwr

ketanhwr Aug 6, 2017

Contributor

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? :|

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@bjorn

bjorn Aug 6, 2017

Owner

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.

Owner

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 some commits Aug 3, 2017

Adds option to Extend Polyline (#157)
Extending can be started from both first point and last point. Escape and Delete Key can be used to cancel the extending.
Adds option to close Polyline into Polygon and vice versa (#404)
Need to select two consecutive nodes in a polygon to delete an edge.
@bjorn

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)

This comment has been minimized.

@bjorn

bjorn Aug 10, 2017

Owner

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.

@bjorn

bjorn Aug 10, 2017

Owner

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,

This comment has been minimized.

@bjorn

bjorn Aug 10, 2017

Owner

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

@bjorn

bjorn Aug 10, 2017

Owner

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

QPolygonF polygon = mapObject->polygon();
if (mapObject->shape() == MapObject::Polygon)

This comment has been minimized.

@bjorn

bjorn Aug 10, 2017

Owner

There is no point in this check, is there?

@bjorn

bjorn Aug 10, 2017

Owner

There is no point in this check, is there?

@ketanhwr

This comment has been minimized.

Show comment
Hide comment
@ketanhwr

ketanhwr Aug 10, 2017

Contributor

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 ?

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@ketanhwr

ketanhwr Aug 10, 2017

Contributor

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.

Contributor

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