Mixedrealitytoolkit-unity: Remove handedness parameter from pointer input events.

[StephenHodgson]

Overview

Pointers shouldn't know about handedness as it doesn't have a property for it (nor should it) Pointers are scene objects that are driven by input sources like controllers. Pointers (like gaze) may be driven by multiple input sources (like hand input). Scripts utilizing this should be determining their handedness from the IMixedRealityController.

[keveleigh]

I don't think this change should be made. Most of our pointer implementations do know about their handedness, via the underlying ControllerPoseSynchronizer. It's set in the synchronizer's Controller setter, which is set on controller pointers when they're created. This property is directly referenced when firing the events.

In the case of the gaze pointer, forwarding along the handedness (the way the events currently work) is the only way to get handedness off a gaze-pointer-fired Pointer event. If we remove that forwarding, the only IMixedRealityController that's accessible is the gaze pointer's, not the hand's (and I'm not even sure the GazeProvider ever sets its controller to anything).

[StephenHodgson]

We should def make this change, and I'll show you the best way to get the handedness when we cross that bridge.

This change is also not a breaking change, unlike the changes you proposed in your PR.

[keveleigh]

I'll show you the best way to get the handedness when we cross that bridge.

Can you show me? I can't see a way to get handedness from the gaze pointer's events.

This change is also not a breaking change

This is also a breaking change, as it contains changes to an interface.

[StephenHodgson]

lol, not it's not a breaking change as a developer getting this update wouldn't have to update their code at all.

[StephenHodgson]

Can you show me?

Already a step ahead of you

Now _that_ is a breaking change :p

[keveleigh]

it's not a breaking change as a developer getting this update wouldn't have to update their code at all.

If they have their own concrete implementation of IMixedRealityInputSystem, they will have to update their code with the interface change.

[StephenHodgson]

Ah, yes, in that case then I suppose it would be.

[keveleigh]

I think removing the Pointer event data setting this value will be confusing. The associated PR doesn't remove the ability to query for Handedness, as that's baked into the underlying InputEventData definition. Instead, it removes meaning from that value while it's still accessible.

Needing to know to query for the specific controller in some cases (re: #3299) while relying on eventData.Handedness in others seems like a confusing pattern.

[StephenHodgson]

We could update that property to just return the input source controller handedness with that utility method I proposed.

So no need to remove it. In fact I'll add that to the change in the other PR.

[keveleigh]

That's exactly the handedness that's already used.

What's the value in needing to ask the input system for a handedness when it's already accessible in one line (eventData.Handedness) or in the pointer class's own property?

[StephenHodgson]

See the change in the other PR where I updated the event.Handedness property.

[StephenHodgson]

I actually decided to roll in all the changes together into the same PR.

In either case, pointer event data now inherits from BaseInputEventData instead.

[keveleigh]

In either case, pointer event data now inherits from BaseInputEventData instead.

The solution for providing handedness in pointer events was to remove handedness from the pointer event data? I'm not sure how making this information harder to discover or access adds value.

Handedness in all event data based on controllers / pointers is valuable information for handed interactions.

[StephenHodgson]

Sure, but here's the thing: the pointers shouldn't know that information. It should only be provided by the controller.

[StephenHodgson]

And I added an easy way to get that information though the input system.

void IMixedRealityPointerHandler.OnPointerUp(MixedRealityPointerEventData eventData)
{
    IMixedRealityController controller;
    if(MixedRealityToolkit.InputSystem.TryGetController(eventData.InputSource, out controller))
    {
        Handedness handedness = controller.ControllerHandedness;
    }
}

[keveleigh]

Pointers either already know that information or simply forward it along.

Pointers that know about their handedness know about it from the controller in the first place.

[keveleigh]

an easy way to get that information though the input system

It's both less easy and less discoverable than eventData.Handedness.

[StephenHodgson]

In either case pointers shouldn't know about handedness.

Likely if you need to know about handedness then the logic needs to be in (InputUp/Down/Pressed) as those are _specific to controllers_.

Pointer events are really only for notifying the UI that events need to take place, and those events should not need to know specifically about the input source nor their handedness.

[StephenHodgson]

For example:
A button doesn't need to know that my left hand clicked it. It just needs to know that it was clicked.

[keveleigh]

A button doesn't need to know that my left hand clicked it. It just needs to know that it was clicked.

But a button can care if it was clicked by the left hand. It might need to in some cases, or in some interactions that are triggered by specific pointer events.

[StephenHodgson]

I'd argue that if it _did_ care, then that logic shouldn't be in the presentation layer, but in the business app logic.

[StephenHodgson]

Or if anything Focus events should handle if a button can or cannot be clicked by a specific handedness input source.

[keveleigh]

that logic shouldn't be in the presentation layer, but in the business app logic.

And that's an option if that's how you want to design your app. As is the reverse.

if anything Focus events should handle if a button can or cannot be clicked by a specific handedness input source.

Instead of filtering within a single event (OnPointerClick), the proposed method after this change is to need to chain state between two events (OnFocusEnter, OnPointerClick)? Twice as many events to handle doesn't seem better.

[StephenHodgson]

And that's an option if that's how you want to design your app. As is the reverse.

Haha, no that's how the whole Framework architecture was designed.

[StephenHodgson]

We have very clear separations between business app "POCO" logic and presentation layer "GameObject" logic

[davidkline-ms]

Not needed.