Added check for valid uiRaycastResults and fixed pointer registration

[StephenHodgson]

Changes

• Fixes: Null refernence errors when running a scene #1346
• Fixes: Clicking buttons on both controllers Crashes Unity #1344 Fixed Pointer registration. We still register pointers on the fly but now we only unregister them when we get a source lost event raised by the InputManager.
• Removed OnValidate checking the UIRaycastCamera so there's not a warning every time we recompile. (For some reason during deserailization when the script is loading, the uiRaycastCamera is null, even though the serialized value is valid.)
• Fixes: Simulated Hands in Editor show when using IMHD #1356
• FIxes: Unity crashes after releasing the Trigger Button (Motion Controller). #1363

[SimonDarksideJ]

Will test this in the morning.

[Railboy]

This fixes the exception on my end.

While we're in here, how do we feel about adding some Debug.Asserts in FocusManager, on lines 544 & 603 (or wherever else makes sense):

Debug.Assert(pointer.PointingSource.Rays != null);
Debug.Assert(pointer.PointingSource.Rays.Length > 0);

Might as well get ahead of incorrectly configured pointers.

[SabinMG]

OnInputClicked(InputClickedEventData eventData) event is not called anymore perfectly after adding this changes.

[StephenHodgson]

@sabingeo can you elaborate?

[SabinMG]

After merging the changes to the latest commit it's no longer possible to select uGUI controls. OnIputClicked of the IInputClickHandler interface is no longer called. tested in the MotionControllerTest scene.

eg : TapResponder.cs

[StephenHodgson]

Which inputs are you using? Your Hands or the Motion controllers?

[SabinMG]

Motion controllers

[StephenHodgson]

Did you double check to make sure the RaycastCamera on the InputManager was assigned to your canvases?

[SabinMG]

in the MotionControllerTest.unity there is no canvas in the scene. There is blue cube attached with TapResponder.cs script.

public void OnInputClicked(InputClickedEventData eventData)
{
           // Increase the scale of the object just as a response.
           gameObject.transform.localScale += 0.05f * gameObject.transform.localScale;

           eventData.Use(); // Mark the event as used, so it doesn't fall through to other handlers.
}

and this part is never called after pressing the trigger.

[StephenHodgson]

Are you also getting the failed assertions and nullable errors?

[SabinMG]

sometimes I see the assertion failed and nullable errors. but not always

[StephenHodgson]

@sabingeo try the latest

[Railboy]

Are you also getting the failed assertions and nullable errors?

So the failed assertions / errors I'm seeing are caused when GazeManager.UpdateGazeInfo() calls FocusManager.Instance.GetPointingExtent(this) to get its extent.

public float GetPointingExtent(IPointingSource pointingSource)
{
        return GetPointingExtent(GetPointer(pointingSource));
}

GetPointingExtent (IPointingSource pointingSource) attempts to track down the PointerData associated with this pointing source via TryGetPointerIndex(IPointingSource pointingSource) before passing it to this function:

private float GetPointingExtent(PointerData pointer)
{
        return (pointer.PointingSource.ExtentOverride ?? pointingExtent);
}

Changing GetPointer to return a default doesn't seem to get ahead of this, because the pointers array isn't always populated before GetPointingExtent is called.

Since IPointingSource has all the information you need to determine its extent, I don't understand the purpose of passing an IPointingSource to one function, then looking up its PointerData, then passing that to a private function, only to use the original IPointerData in the end. I could be missing something, but this seems like it could be cruft?

We could make GetPointingExtent(IPointingSource pointingSource) a public function and eliminate the private function altogether:

public float GetPointingExtent (IPointingSource pointingSource) {
        if (pointingSource == null)
                return 0;

        return (pointingSource.ExtentOverride ?? pointingExtent);
}

This gives us a universal way for pointers to determine their own extent without causing errors.

[StephenHodgson]

Agreed on all counts. In the middle of some updates, that I'm testing out atm.

Essentially I removed the nullable types and replaced them with nicer fallbacks.

[StephenHodgson]

Just working on one last small issue with the gaze getting unregistered when we should probably never do that.

[StephenHodgson]

@Railboy I think I fixed all the issues.

Please let me know what you think.

All tests passed, and built to HoloLens and PC with successful input testing.
Test scenes:

• Motion Controller Test
• Input Manager Test

new changes

[Railboy]

Checking it out now.

[Railboy]

Looks good.

[Railboy]

autoRegisterGazePointerIfNoPointersRegistered
what about:
autoRegisterGazeIfPointersEmpty ?

(matter of taste, feel free to ignore)

[StephenHodgson]

Yeah that field name is obnoxiously long haha

[Railboy]

FYI in Unity 2017.0.2f3 I'm getting 3 assertion failures on startup in the InputManagerTest scene. But they're unrelated to the pointer issue:

Assertion failed: Main camera does not exist!
UnityEngine.XR.WSA.HolographicSettings:get_IsDisplayOpaque()

IsDisplayOpaque is being called by FadeScript and BoundaryManager. These failures don't happen in Unity MRTP 4 so I'm guessing it's an already-fixed XR bug on Unity's end.

[StephenHodgson]

To be honest, I'm trying to understand why you'd ever want to skip registering the gaze pointer.

[keveleigh]

I think that was a hold over from only having active pointers registered. It looks like you've changed the way that works though, but I haven't had a chance to review all these recent changes.

[SabinMG]

@StephenHodgson I have checkout the latest source from your branch https://github.com/StephenHodgson/MixedRealityToolkit-Unity/tree/MRTK-uiRaycastHotfix

Input events triggers perfectly fine now. But I see some other issues

1. After the build headset camera postions are not updating anymore, still rotation works fine though. It even happens sometimes running on editor as well. (if you restart mixed reality-Portal app some time it fixes the issue and then fall back to the same case )
   I even downloaded some apps from the store and some has the same issue. Anything wrong with my develop environment setup ?

2. Cursor Default distance is no more reflefted to the value that you set in the inspector.

3. And I also see slight peformance drop in the app after the build.

I am using Unity 2017.2.0p1-MRTP4 (64-bit) and OS Windows 10 pro Insider preview. Version 1709

[StephenHodgson]

@sabingeo Thanks for taking a look. Are those issues related to this PR specifically?

You may want to open a separate issue for these. I will not address them here unless they're directly related to this.

Cursor Default distance is no more reflected to the value that you set in the inspector.

I'll look into this.

[StephenHodgson]

@Railboy I updated the PR with some of your suggestions about directly getting the pointer extents, etc.

[Railboy]

Great, the new function names / purposes seem a lot clearer to me.

I'm not getting the camera translation errors @sabingeo is seeing so it may be a setup issue.

The cursor distance issue is caused by using the terminus of the last RayStep in the cursor's pointer:

protected virtual void UpdateCursorTransform()
{
    FocusDetails focusDetails = FocusManager.Instance.GetFocusDetails(Pointer);
    GameObject newTargetedObject = focusDetails.Object;

    // Get the forward vector looking back along the pointing ray.
    RayStep lastStep = Pointer.Rays[Pointer.Rays.Length - 1];
    Vector3 lookForward = -lastStep.direction;

We can honor the maximum distance by using a static function to get a point along a set of RaySteps by distance. I can open up a new pull request to do this.

[StephenHodgson]

Just open a PR on this branch I'm workin on if you got a fix.

[Railboy]

Since it's only tangentially related I'd rather roll it into #1364. It may take time to implement / test and I don't want to hold this merge up.