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

[MRTK3] Added an option to use scalar distance instead of final displacement for bounds control toggle #11264

Merged
merged 12 commits into from
Jan 6, 2023

Conversation

akash14darshan
Copy link

@akash14darshan akash14darshan commented Dec 2, 2022

Overview

The toggling of bounds control is done when an asset is clicked but not moved.
The way we determine Clicked vs Moved is by using the asset's displacement. The initial position and final position.
Here is a video which demonstrates a case where the asset is moved significantly, yet determine the asset was clicked because we look only at the initial and final position, which are really close (lower than the drag threshold in this case)

overview.mp4

Changes

  • Added code to calculate the scalar distance the object was moved (per frame). Using this logic, we can determine better if the asset was actually moved or not.
  • Added a toggle to switch between the two modes.
result.mp4

PS: Do let me know if I need to change wordings/fieldname/comments anywhere since English is not my native language.

@akash14darshan
Copy link
Author

@microsoft-github-policy-service agree

RogPodge
RogPodge previously approved these changes Dec 20, 2022
@keveleigh
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@keveleigh
Copy link
Contributor

@akash14darshan, sorry for such a delayed review here. This idea looks great! I think we'd like to change it slightly so that this is the default behavior (as well as updating the distance check to only run until the movement threshold is reached, since at that point there's no way to reverse). Do you mind if I push some commits to your branch? I'm also happy to make suggestions and let you run with it though!

@akash14darshan
Copy link
Author

@keveleigh that's a good optimization and I totally agree with you.
Please feel free to make changes as you would like :D

@keveleigh
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Until the threshold has been passed at least once
@keveleigh
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@keveleigh keveleigh self-assigned this Jan 3, 2023
@keveleigh
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@keveleigh
Copy link
Contributor

Hey @akash14darshan, please take a look at the current state of this PR and make sure I haven't broken your original fix! I added some play mode tests to try to catch it, so it should be all good, but just want to make sure!

RogPodge
RogPodge previously approved these changes Jan 4, 2023
@keveleigh
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@keveleigh keveleigh merged commit 4adfdaa into microsoft:mrtk3 Jan 6, 2023
drusk-unity pushed a commit to drusk-unity/MixedRealityToolkit-Unity that referenced this pull request Jun 26, 2023
…acement for bounds control toggle (microsoft#11264)

* Added an option to use scalar distance instead of final displacement for bounds control toggle

* Fixed some typos in comments

* Update bool to false on host deselection

* Make per-frame distance checking for the threshold the default

Until the threshold has been passed at least once

* Update RuntimeTestUtilities.cs

* Add tests

* Update BoundsControlTests.cs

Co-authored-by: RogPodge <39840334+RogPodge@users.noreply.github.com>
Co-authored-by: Kurtis <kurtie@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants