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

Conversation

Projects
None yet
3 participants
@ketanhwr
Contributor

ketanhwr commented Aug 24, 2017

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

Double click splits a segment
Single click selects the two nodes of a segment
@Ablu

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

Show outdated Hide outdated src/tiled/editpolygontool.cpp Outdated
Show outdated Hide outdated src/tiled/editpolygontool.cpp Outdated
Show outdated Hide outdated src/tiled/editpolygontool.cpp Outdated
@Ablu

This comment has been minimized.

Show comment
Hide comment
@Ablu

Ablu Aug 24, 2017

Contributor

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...

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@ketanhwr

ketanhwr Aug 24, 2017

Contributor

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

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@ketanhwr

ketanhwr Aug 24, 2017

Contributor

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

Contributor

ketanhwr commented Aug 24, 2017

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

@Ablu

This comment has been minimized.

Show comment
Hide comment
@Ablu

Ablu Aug 24, 2017

Contributor

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

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@ketanhwr

ketanhwr Aug 24, 2017

Contributor

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

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@Ablu

Ablu Aug 24, 2017

Contributor

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

Contributor

Ablu commented Aug 24, 2017

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

Proposal for review changes
- simpler math
- less duplicate code
@Ablu

This comment has been minimized.

Show comment
Hide comment
@Ablu

Ablu Aug 24, 2017

Contributor

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

Contributor

Ablu commented Aug 24, 2017

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

@bjorn

This comment has been minimized.

Show comment
Hide comment
@bjorn

bjorn Aug 29, 2017

Owner

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.

Owner

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

This comment has been minimized.

Show comment
Hide comment
@ketanhwr

ketanhwr Aug 29, 2017

Contributor

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?

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@bjorn

bjorn Aug 29, 2017

Owner

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.

Owner

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

This comment has been minimized.

Show comment
Hide comment
@ketanhwr

ketanhwr Sep 1, 2017

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@bjorn

bjorn Jan 19, 2018

Owner

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.

Owner

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