[Deferred] SDL 0137 - TouchCoord outside video screen range

theresalech commented 6 years ago

Hello SDL community,

The review of "TouchCoord outside video screen range" begins now and runs through February 13, 2018. The proposal is available here:

https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0137-TouchCoord-outside-video-screen-range.md

Reviews are an important part of the SDL evolution process. All reviews should be sent to the associated Github issue at:

https://github.com/smartdevicelink/sdl_evolution/issues/392

What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of SDL. When writing your review, here are some questions you might want to answer in your review:

Is the problem being addressed significant enough to warrant a change to SDL?
Does this proposal fit well with the feel and direction of SDL?
If you have used competitors with a similar feature, how do you feel that this proposal compares to those?
How much effort did you put into your review? A glance, a quick reading, or an in-depth study? Please state explicitly whether you believe that the proposal should be accepted into SDL.

More information about the SDL evolution process is available at

https://github.com/smartdevicelink/sdl_evolution/blob/master/process.md

Thank you, Theresa Lech

Program Manager - Livio theresa@livio.io

AndrewRMitchell commented 6 years ago

In "Detailed Design", can we get version number bumps in both the XML files. I don't know what the policy is for changes that are forward compatible but not backward compatible as for which number to bump, but I'm sure someone who's been her longer can chime in.

Then you may wish to mention a version check for compatibility. In general I'm seeing a lack of version checking discussion in the proposals I'm reading, so that might be the accepted way of doing things, I'm still figuring these things out.

In "Alternatives Considered" can we label the first XML block with Additions to Mobile_API. And can we get some discussion as to why this alternative (add negative values to x and y) was not chosen? That seems to be lost here.

Finally as these are changes or modifications to the API instead of additions, can we label them as such to be consistent with other proposals.

masatoogawa commented 6 years ago

@AndrewRMitchell Thank you for your review!

In "Detailed Design", can we get version number bumps in both the XML files. I don't know what the policy is for changes that are forward compatible but not backward compatible as for which number to bump, but I'm sure someone who's been her longer can chime in.

Then you may wish to mention a version check for compatibility. In general I'm seeing a lack of version checking discussion in the proposals I'm reading, so that might be the accepted way of doing things, I'm still figuring these things out.

Sorry , I am not familiar with version bumps and check.

(This proposal is not a change of TouchCoord's format, it expands the range, so I think this have both backward compatibility and forward compatibility, but It doesn't matter)

In "Alternatives Considered" can we label the first XML block with Additions to Mobile_API.

Sorry, it is mistake. I will correct it.

And can we get some discussion as to why this alternative (add negative values to x and y) was not chosen? That seems to be lost here.

I guess there is no basis for minvalue = 0 and maxvalue = 10000. So I thought that removing max/min is more resonable than set up another range.

Please let me know if there is a max/min value basis.

Finally as these are changes or modifications to the API instead of additions, can we label them as such to be consistent with other proposals.

I could not find such a label. https://github.com/smartdevicelink/sdl_evolution/labels

joeljfischer commented 6 years ago

Finally as these are changes or modifications to the API instead of additions, can we label them as such to be consistent with other proposals.

I could not find such a label.

I think he meant in the proposal under detailed design that you have "Additions to Mobile_API" when in fact this is a change.

I'm concerned about a few things with this proposal. First is that we are expanding the range unexpectedly for developers. @joeygrover would be better suited to discuss the version implications of this change. It is an expansion, so it may be considered a minor version change.

Second, I'm concerned about the iOS / Android proxy implications of this change. Allowing values outside of the screen can lead to some undiscussed odd implications. For example, attempting to pinch to zoom on a center point outside of the on-screen coordinates. The coordinate API and touch in general was not designed for haptic interfaces, and weird behavior such as I just mentioned would need to be discussed and defined.

masatoogawa commented 6 years ago

@joeljfischer Thank you for your review!

weird behavior such as I just mentioned would need to be discussed and defined.

Agree. How to handle touch coordinates outside the range depends on the application. Existing applications may behave like odd as you said and others may do what users expect. I would like to hear the opinions of SDL applications stakeholders.

If this change is delicate to the existing application, how about the following idea?

Add offset for out of screen to TouchCoord

Give up compatibility of data structure, keep compatibility of data range.

 <struct name="TouchCoord">
     <param name="x" type="Integer" mandatory="true" minvalue="0" maxvalue="10000">
         <description>The x coordinate of the touch.</description>
     </param>
+    <param name="x_offset" type="Integer" mandatory="true">
+        <description>The x coordinate that supports out of screen.</description>
+    </param>
     <param name="y" type="Integer" mandatory="true" minvalue="0" maxvalue="10000">
         <description>The y coordinate of the touch.</description>
     </param>
+    <param name="y_offset" type="Integer" mandatory="true">
+        <description>The y coordinate that supports out of screen.</description>
+    </param>
 </struct>

Add screen range to VideoConfig

Removing TouchCoord's maxvalue and minvalue, SDL proxy/application notifies the supported screen range with SetVideoConfig (default: 0-10000)

 <struct name="VideoConfig">
   <description>Configuration of a video stream.</description>
   <param name="protocol" type="Common.VideoStreamingProtocol" mandatory="false">
     <description>The video protocol configuration</description>
   </param>
   <param name="codec" type="Common.VideoStreamingCodec" mandatory="false">
     <description>The video codec configuration</description>
   </param>
   <param name="width" type="Integer" mandatory="false">
     <description>Width of the video stream, in pixels.</description>
   </param>
   <param name="height" type="Integer" mandatory="false">
     <description>Height of the video stream, in pixels.</description>
   </param>
+  <param name="screenMaxRange" type="Integer" mandatory="false">
+  </param>
+  <param name="screenMinRange" type="Integer" mandatory="false">
+  </param>
 </struct>

masatoogawa commented 6 years ago

The coordinate API and touch in general was not designed for haptic interfaces

There are differences between the types of coordinate values (Float and Integer), but I think that it will not be a big issue.

joeljfischer commented 6 years ago

I think there could be a few more options:

Cap the x / y values

This option would cap the x / y values to the edges of the screen. So if the x value would be out of the minimum range, instead of being negative, the value would be 0. If it would be out of the maximum range, it would be the max screen value.

Positives

No API changes needed.
Easy for app developers.

Negatives

Multitouch gestures at the edge may not work as the user would expect from the haptic device. A visual indicator that the user is at the edge of the screen may be necessary for them to understand that they are bound by the edge of the screen.

Interpolation

This would bound the "center" of the pinch at the edges of the screen. Instead of bounding the edge-most finger of the multitouch to the edge of the screen as in my above suggestion, this would bound the edge-most finger to an equivalent distance from the edge of the screen.

Positives

Does not require any work by the developer
Keeps things in a sane state, the pinch center will never attempt to happen outside of the view bounds.

Negatives

May prevent some two-finger swipe gestures (not currently supported), but wouldn't really work in this scheme.
Requires proxy changes to support.
Requires Core/HMI changes and calculations to support.

masatoogawa commented 6 years ago

Interpolation

Let me clarify your idea. Unless multi-touch and its center coordinates are out of screen, you can accept touch coordinate out of screen (Your biggest concern is center coordinate of pinch gesture?). Am I correct?

joeljfischer commented 6 years ago

Right, my largest concern is for when the pinch to zoom center is outside of the screen bounds. This would likely be undefined behavior.

It appears that this proposal is generally oriented to "mouse" pointer-based haptic systems. Another possibility is for the HMI of this system to consider the pointer as the center point of multitouch gestures. The only currently (automatically) supported multi-touch gestures in the iOS library is the pinch gesture. This doesn't appear to be what the proposal stipulates, but would relieve some of the possible undefined behavior.

AndrewRMitchell commented 6 years ago

Another option may be to leave TouchCoord as it is (since it seems to actually work for touchscreen based HMIS's), and add a separate GestureCoord and flush it out to fully deal with touchpads and "mouse" pointer-based haptic systems. If the new model isn't fitting well in the existing design then maybe the existing design is not the right place for it.

This has a downside of introducing a new paradigm, but for HMI's that support it, it lets us flush out that functionality to support their use cases. If we really wanted to go wild we could even provide both data sets in the interface as long as the GestureCoord didn't map to be outside the TouchCoord, and if it did, simply drop the input from TouchCoord as not being supported through that interface (or add a new "Input Out of Range" flag to the TouchCoord).

joeljfischer commented 6 years ago

@AndrewRMitchell I definitely considered proposing that, and we've discussed it before, but it has one massive disadvantage: that both us (the mobile library developers) and the app developers would have to support two separate ways of interacting with the app. This puts too much of a burden on the app developer and makes the barrier to entry too high. I think we can solve this without starting over, and too much development time has probably been spent to restart now.

masatoogawa commented 6 years ago

It appears that this proposal is generally oriented to "mouse" pointer-based haptic systems.

This proposal is also considered about touch screen which notifies the absolute coordinate values not the relative movement amount.

Motivation

We have use cases for keeping gesture recognition from the SDL Video screen to the Native screen.

For example, the SDL video displays as a part of the screen, and the video displays in redused size on touch screen.

To find an acceptable compromise, Joel's Interpolation works for me. Updates HMI and MOBIEL API xml like my 1st proposal and updates core to fileter multi-touch event which the center is out of screen.

kshala-ford commented 6 years ago

Since the current TouchCoord does not support negative values, SDL applications cannot recognize some gestures for upward or leftward direction.

I'm not sure I can follow. Aren't up-/leftward movements result into MOVE events with reduced x/y coords as topleft of a HU display is marked as (0,0)?

masatoogawa commented 6 years ago

@kshala-ford Thank you for your review! Sorry, please let me know a bit more about what you are concerned.

jacobkeeler commented 6 years ago

@AndrewRMitchell @masatoogawa With regards to the API version changes that this proposal would need, this proposal would institute breaking changes, requiring a major version bump. Apps could previously guarantee that the coordinates they received would be in a positive range. With this change, that assumption would have to be changed, which would break compatibility with older versions.

Personally, I think that the Cap the x / y values alternative proposal would be the simplest solution to this issue. If you feel that this solution is too limiting, the interpolation solution should still work, it would just need to be included in a major version.

theresalech commented 6 years ago

The Steering Committee voted to defer this proposal, keeping it in review until our next meeting on 2018-02-20, to allow for more discussion on the review issue.

masatoogawa commented 6 years ago

Thanks everybody. If there is no particular issue, I will update proposal with "Interpolation". Does anybody have comments?

joeljfischer commented 6 years ago

@masatoogawa As @jacobkeeler noted above, to do so would require a major version bump of Core as it would be a breaking change for HMI <-> Core communication.

What do you think of considering the mouse pointer the center point of multitouch gestures (though this could still have out-of-bounds data, which is a problem), or of capping at the screen maximums?

masatoogawa commented 6 years ago

@joeljfischer

to do so would require a major version bump of Core

Let me clarify

Is the version you mentioned the interface version?

MOBILE_API.xml: <interface name="SmartDeviceLink RAPI" version="4.5.0" date="2017-09-22">

HMI_API.xml: <interface name="Common" version="1.7.0" date="2017-10-20">

or SDL Core product version (may be SDL Core 5.0) ?

masatoogawa commented 6 years ago

What do you think of considering the mouse pointer the center point of multitouch gestures (though this could still have out-of-bounds data, which is a problem), or of capping at the screen maximums?

SDL core calculates pinch center for each multi-touch update and cap TouchCoord to keep the pinch center in the screen bounds.

masatoogawa commented 6 years ago

Thanks everybody. I think it is too early to conclude this discussion. I would like to deffer this proposal for the feasibility study with reconsider pinch center issue which Joel mentioned.

joeljfischer commented 6 years ago

Is the version you mentioned the interface version?

I think all 3 would probably need a major version bump. Because Core is sending previously undefined data to mobile, unexpected results may occur. Same for HMI. They aren't backward compatible. Because those need a major version bump, so would Core.

theresalech commented 6 years ago

The Steering Committee has voted to defer this proposal per the author's request. The author will study the feasibility of considering the mouse pointer as the center point of multi-touch gestures, where SDL Core calculates the pinch center for each multi-touch update and TouchCoord is capped to keep the pinch center within the screen bounds. The x/y coordinates will also be capped.

theresalech commented 6 years ago

Author has advised that they are unable to take action on this proposal at this time, therefore the proposal will be closed. The issue will remain in a deferred status and unlocked so the author can notify the Project Maintainer when it is ready to be revisited.

smartdevicelink / sdl_evolution