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

Double click splits a segment #1705

Closed
wants to merge 2 commits into from
Closed

Conversation

@ketanhwr
Copy link
Contributor

@ketanhwr ketanhwr commented Aug 24, 2017

Double clicking on a segment splits it. Single click selects the two nodes of that particular segment.

Single click selects the two nodes of a segment
Copy link
Contributor

@Ablu Ablu left a comment

I think the calculation is a bit unnessecary complex. I will try to create an example to make it easier to read.

* Returns the distance between QPointF a and QLineF(a, b)
* Returns infinite if angle b-a-c is not obtuse
*/
static qreal helper(QPointF a, QPointF b, QPointF c)

This comment has been minimized.

@Ablu

Ablu Aug 24, 2017
Contributor

hm... helper does not really explain anything here... Maybe distanceOfPointToLine and then as arguments point, lineStart, lineEnd?

if (angleA < M_PI / 2.0)
return INT_MAX;

qreal x0 = a.x();

This comment has been minimized.

@Ablu

Ablu Aug 24, 2017
Contributor

why not ax? (or according to the naming suggestions I made before...)

}

if (object->shape() == MapObject::Polygon) {
qreal temp = helper(point, polygon.first(), polygon.last());

This comment has been minimized.

@Ablu

Ablu Aug 24, 2017
Contributor

temp = helper. If you see this code, what do you think does it do? distance = distanceOfPointToLine allows you to continue reading without checking the helper function.

@Ablu
Copy link
Contributor

@Ablu Ablu commented Aug 24, 2017

See Ablu@14fc326 for an alternative solution. Granted... it is very verbose, I guess one could meet somewhere in the middle. Also there is no real need for the square root to get the length, the squared length works perfectly fine too.

Otherwise I find it slightly confusing that it does not add the new point at the positionof the double click, but in the center. Also there are still two points selected after the split...

@ketanhwr
Copy link
Contributor Author

@ketanhwr ketanhwr commented Aug 24, 2017

Regarding your solution, it works a bit weirdly. Clicking on one side of segment works and on the other side doesn't.

@ketanhwr
Copy link
Contributor Author

@ketanhwr ketanhwr commented Aug 24, 2017

Because we won't expect the user to click on a point lying perfectly on the line.

@Ablu
Copy link
Contributor

@Ablu Ablu commented Aug 24, 2017

Ah right... There is an issue with the unbounded check. it does not work with the normal like this. I will update it.

@ketanhwr
Copy link
Contributor Author

@ketanhwr ketanhwr commented Aug 24, 2017

No I don't think that's the issue. Maybe because we need to check with both the directions of the normal vector. Currently you are checking with only one orientation of the normal

@Ablu
Copy link
Contributor

@Ablu Ablu commented Aug 24, 2017

well... yes. Because the normal goes into the other direction it only results in an unbounded intersection.

- simpler math
- less duplicate code
@Ablu
Copy link
Contributor

@Ablu Ablu commented Aug 24, 2017

see Ablu@76fc754 for the fix. I need to check the bounds of the line again...

@bjorn
Copy link
Member

@bjorn bjorn commented Aug 29, 2017

I tested this a bit and noticed a few problems:

  • The split happens at the center, whereas I would expect it to be at the location where I double-clicked.

  • Half of the time that I try to double-click, it does not work. I guess that's the problem you're still trying to find a good solution to.

  • It seems I can select the two nodes at the ends of a segment by clicking the segment. However, sometimes they seem to get selected on press, but then they deselect again on release. This happens even when I'm not moving the mouse at all between press and release. Also, it should be possible to hold Shift to add to the selection.

  • There should probably be a highlight of the segment when you hover it.

@ketanhwr
Copy link
Contributor Author

@ketanhwr ketanhwr commented Aug 29, 2017

The split happens at the center, whereas I would expect it to be at the location where I double-clicked.

Okay, this might be a bit tricky. I'll try it after the next three points are solved first.

Half of the time that I try to double-click, it does not work. I guess that's the problem you're still trying to find a good solution to.

Highlighting will help here, I think.

It seems I can select the two nodes at the ends of a segment by clicking the segment. However, sometimes they seem to get selected on press, but then they deselect again on release. This happens even when I'm not moving the mouse at all between press and release. Also, it should be possible to hold Shift to add to the selection.

This is something I'm trying to debug. Will fix soon.

There should probably be a highlight of the segment when you hover it.

Highlight as in? The edge changes color when highlighted over it?

@bjorn
Copy link
Member

@bjorn bjorn commented Aug 29, 2017

Highlight as in? The edge changes color when highlighted over it?

Exactly. I think it's not the full solution to the second issue though, since sometimes I'm pretty sure I should be hitting the segment.

@ketanhwr
Copy link
Contributor Author

@ketanhwr ketanhwr commented Sep 1, 2017

However, sometimes they seem to get selected on press, but then they deselect again on release.

This is something even I'm experiencing, and I have no idea why. Probably some function which is called on mouse release..I'll dig further.

@bjorn bjorn closed this in 3b99820 Jan 19, 2018
@bjorn
Copy link
Member

@bjorn bjorn commented Jan 19, 2018

I've decided to implement an alternative solution in changes c7dae1b and 3b99820.

The first change only implements a number of expected basic interactions with segments, like clicking them to select the handles on both sides, dragging them and right-clicking them. The second change added the splitting of the segment on double-click.

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.