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

Add shaking a dragged node free from its link #1599

Draft
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

pwbh
Copy link

@pwbh pwbh commented Feb 9, 2024

I have added a simple shake detection for #904

It seems to work well enough, although I can change it to work against some time delta which will check the differences between values in each "aggressive" movement back and forth, it probably will be more accurate, but please test this see how it feels first :)

@0HyperCube 0HyperCube marked this pull request as draft February 9, 2024 16:34
Copy link

github-actions bot commented Feb 9, 2024

📦 Build Complete for 2af578f
https://7d1ca7d1.graphite.pages.dev

@pwbh
Copy link
Author

pwbh commented Feb 9, 2024

Actually I am seeing that fast movement to left or right is triggering the shake check, I will improve it to use deltas between movement left and right and only trigger if the movement has been both to left and right in interval of some threshold, this should produce more accurate results.

@Keavon
Copy link
Member

Keavon commented Feb 9, 2024

Yes, following what I mentioned in my comment in the issue, you need to take a much more sophisticated approach specifically designed to actually detect a user's intentional pointer shake over a series of samples spanning time, probably about half a second. That means recording every sample over that time in a running queue (discarding any that are older than that age) and performing analysis on them each frame. I suggested a metric which optimizes for detecting (beyond some threshold) a high ratio of direction change over average displacement, meaning the user is keeping the mouse in a relatively similar position but shaking a lot.

@pwbh pwbh marked this pull request as ready for review February 11, 2024 15:40
@pwbh
Copy link
Author

pwbh commented Feb 11, 2024

@Keavon I have updated the implementation, feels much more accurate 😄

@Keavon
Copy link
Member

Keavon commented Feb 11, 2024

!build

Copy link

📦 Build Complete for 16c4cf2
https://d59f087a.graphite.pages.dev

@Keavon
Copy link
Member

Keavon commented Feb 12, 2024

I gave it a quick test and it seems to have a very high rate of false positives. I also gave a quick glance at the code and I don't see a division sign anywhere, meaning I'm not sure if you took my suggestion to use an algorithm based on ratio of direction change over average displacement. Can you please carefully re-read my suggested algorithm and precisely implement that? (I also noticed that you seem to be using only the X axis, which I don't think is robust compared to considering both axes together. Direction change can be measured as the cross product of the vectors of mouse deltas in neighboring samples.)

@pwbh
Copy link
Author

pwbh commented Feb 12, 2024

I gave it a quick test and it seems to have a very high rate of false positives. I also gave a quick glance at the code and I don't see a division sign anywhere, meaning I'm not sure if you took my suggestion to use an algorithm based on ratio of direction change over average displacement. Can you please carefully re-read my suggested algorithm and precisely implement that? (I also noticed that you seem to be using only the X axis, which I don't think is robust compared to considering both axes together. Direction change can be measured as the cross product of the vectors of mouse deltas in neighboring samples.)

It returns a lot of false positives because of the alert message, it felt better when I tested it against trigger with console.log instead which doesn't blurs the focus of dragging the node, but I'll tweak it to reflect your comment anyway, thanks.

@pwbh pwbh marked this pull request as draft February 12, 2024 20:50
@pwbh
Copy link
Author

pwbh commented Feb 12, 2024

@Keavon just an FYI, but before going with your suggestions I'd like to try to tweak this implementation tomorrow morning, I just realized its missing a few things (for example I am not checking whether a movement occurred in both directions, and not only in one before checking for difference in captured positions), I'll test it again and if those changes won't achieve the desired effect, I'll reimplement it by your suggestion.

@pwbh pwbh marked this pull request as ready for review February 13, 2024 15:48
@Keavon
Copy link
Member

Keavon commented Feb 14, 2024

!build

Copy link

📦 Build Complete for 08e1f30
https://ac031e5b.graphite.pages.dev

@Keavon
Copy link
Member

Keavon commented Feb 15, 2024

!build

Copy link

📦 Build Complete for 5859d65
https://283259b3.graphite.pages.dev

@Keavon
Copy link
Member

Keavon commented Feb 15, 2024

I'll look more closely at the code for shake detection, but it behaves reasonably well at the moment. Please proceed with implementing the actual shake removal now which should call into the backend (and I think that will require changes to the backend to implement the actual removal, you can reference the deletion code which does the job of reconnecting the link.)

@Keavon Keavon marked this pull request as draft February 17, 2024 07:31
@Keavon Keavon changed the title Added simple node shaking detection Add shaking a dragged node free from its link Feb 18, 2024
@Keavon
Copy link
Member

Keavon commented Feb 29, 2024

Checking on your status.

@pwbh
Copy link
Author

pwbh commented Mar 30, 2024

hey hey hey, will try to finish it up in the coming week, sorry for the delay, was working on couple of personal stuff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants