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

Fix proximity effect #5987

Merged
merged 1 commit into from
Sep 18, 2019
Merged

Conversation

julenka
Copy link
Contributor

@julenka julenka commented Sep 18, 2019

Overview

In #5920 I modified PokePointer to disable the finger cursor when poke pointer was not enabled (IsInteractionEnabled == false). Turns out, the finger cursor drives the proximity effect, so disabling it causes not only the finger cursor to not show up, but for all proximity effects to stop working.

A longer term fix would be to ensure that the proximity effect is running even when poke interactions are off, or when the poke pointer is not present. A short term fix is just to always have the finger cursor on, and to just turn off the visual if interaction is not enabled.

Changes

Verification

Note: when hand is near a grabbable, the cursor is still off. In the shell the cursor is on even when you are near a grabbable but not a touchable. I think this is a bug in the shell, since the finger cursor is meant to communicate "you're near a touchable thing"

This optional section is a place where you can detail the specific type of verification
you want from reviewers. For example, if you want reviewers to checkout the PR locally
and validate the functionality of specific scenarios, provide instructions
on the specific scenarios and what you want verified.

If there are specific areas of concern or question feel free to highlight them here so
that reviewers can watch out for those issues.

As a reviewer, it is possible to check out this change locally by using the following
commands (substituting {PR_ID} with the ID of this pull request):

git fetch origin pull/{PR_ID}/head:name_of_local_branch

git checkout name_of_local_branch

@julenka
Copy link
Contributor Author

julenka commented Sep 18, 2019

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Member

@Cameron-Micka Cameron-Micka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Just pulled this down and it fixes the proximity light, do we also want to enable the cursor to match the shell?

@cre8ivepark
Copy link
Contributor

Confirmed, the lighting effect works well!
MRTK_LightFix

@julenka
Copy link
Contributor Author

julenka commented Sep 18, 2019

Thanks! Just pulled this down and it fixes the proximity light, do we also want to enable the cursor to match the shell?

I would say no, for two reasons:

  1. I think this is actually a bug in the shell, to show the finger cursor if nothing is touchable near. The cursor communicates "you are about to press something" so it's misleading to have it show up when you are about to grab something. I think we should have a different affordance when hand is near a grabbable.
  2. It is not a regression from 2.0 release. 2.0 release does not show the finger cursor.

@julenka julenka merged commit b7ae228 into microsoft:mrtk_development Sep 18, 2019
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

3 participants