-
Notifications
You must be signed in to change notification settings - Fork 33
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
add altitudeAngle/azimuthAngle #316
Conversation
Harmonize on "MUST be 0", and ensure that the value of one type of attributes is not accidentally required to be zero if later we say implementations must backfill the missing one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it makes sense to include the transformation between these two sets of value as part of the spec?
Clarification that the sentence referred to untrusted (synthetic) PEs
…titudeAngle/azimuthAngle
@NavidZ @smaug---- et al, please have a look at the new informative section. i cribbed the core of the code from @k3a 's https://gist.github.com/k3a/2903719bb42b48c9198d20c2d6f73ac1 ... but my programmer chops are not what they used to be, so check that i didn't mess it up (also, opted to go for a simplified pseudo-code/javascript approach rather than Perl) |
and in fact, I just realised that @k3a's code uses radians for both sets of values ... but |
also, looking at https://github.com/jquery/PEP/pull/376/files#diff-3b5aeb16d85e265fdb40c6063ac62a96R160 it seems there the |
that's probably because from what i can see, the altitude/azimuth approach has no concept of a pen's twist. if you tilt a pen from 0 to fully perpendicular and then carry on tilting further, in the tiltX/tiltY model you can tell the pen was moved over the perpendicular point, whereas in the other model it's the same as if you tilted the pen from the other side up. if that makes sense. |
Unfortunately I don't have a pen that reports a nonzero twist. But are you saying if I twist the pen the tilt axes will change and no longer parallel to the screen borders? I know we don't do any adjustment on Chrome and just report what we get from the OS. |
no sorry, i mashed two thoughts together there. let me see if i can clarify: on windows etc. where tiltX/tiltY is used, pens also have a particular orientation/twist (e.g. where the button is on the barrel). and that lets you naturally distinguish between a pen that's only raised less than 90 degress from the surface (with the button pointing to the ceiling, if you will) and one that's gone "over" the perpendicular position (with the button pointing to the floor). but in the altitude/azimuth model, the second case doesn't exist. if you go over the perpendicular position, the azimuth simply goes 180 degrees the other way and the more you continue, the lower you're making the altitude again. you don't have a concept of a pen being upside down / gone past the perpendicular position (basically because altitude is only from 0 to 90 degrees, whereas in the tiltx/tilty model you have an additional "attitude" of the pen which makes you make more specific statements between 0 and 180 degrees). anyway, sounds like this could get very complicated and edge-casey. something i hadn't considered initially. also, worth going to the source of what @k3a based their code on, perhaps https://code.woboq.org/qt5/qtbase/src/plugins/platforms/windows/qwindowstabletsupport.cpp.html#527 as that seems to have some further nuances in the code? |
thinking about this some more (though admittedly i'm slowly out of my technical depth here), essentially the two models differ at a fundamental level, tied to the kinds of sensors (or the data provided in the low-level/OS-level APIs). the wondering if it's sufficient to address this in prose? aknowledge that going from the also yes, @grorg @graouts i'd be interested if there's an official |
So looking at these pictures both of them show different sets of tiltX/tiltY and also altitude/azimuth values. Why do you say they have the same altitude/azimuth? They might have the same altitude as it seems they are in the same "height" (I'm just eyeballing it). But the azimuth is certainly different. For one on the left it is more close to 7xpi/4 and for the one on the right it seems to be 3xpi/4. I don't see the lossy model you are referring to here.
Regarding the straight pen and the fact that perpendicular state could have any azimuth, we do have a similar situation for tiltX/tiltY. But at another location which is less likely to happen. That is when the pen is lying on the x axels on the surface. That can have any tiltY. Having said that I'm not saying we even get any events from the device in that case as the tip might not be even touching the screen and hence the device might not generate any events. So that is just hypothetical. |
it might be that i'm getting myself confused here. need to dig out my pen and look at some values coming back. i somehow had it in my head that there's a distinction between these two states, but i might be completely wrong (and if i am, good) [edit: the difference is probably only in the |
Yeah. I don't have a pen that supports twist in hand to see what its driver reports. But my understanding is that if you hold the pen at a particular position in space and just twist it only the twist value should change and not tiltX/tiltY. @Summerlw do you happen to have the stylus that reports twist handy to test this? |
I did the trig math for altitude/azimuth so please have a look. Let me know if I made mistakes. If pen's tip is resting at Now we can compute
Having the above formulas it is easy to verify the formulas at https://github.com/jquery/PEP/pull/376/files#diff-3b5aeb16d85e265fdb40c6063ac62a96R160 to be correct:
For computing
For
Added code for the above here (code includes edge cases): [1] https://www.w3.org/TR/pointerevents3/ |
- correct the X-Z -> X-Y plane error - expand the definitions to be more understandable - add the special case of perpendicular pen having an azimuthAngle of 0 (as it could theoretically be any angle since)
@liviutinta made a stab at changing the code to JavaScript (as I'm not sure if we want python code here - while in theory I guess any code would do, it feels a bit more natural providing it in JS here, as in the end it's just pseudo-code for browsers to then implement)). I'm not the cleanest or most elegant of coders, so happy for this to be made nicer/more modern/better documented. |
After discussion with @NavidZ and while re-writing the Python code in Javascript, I was able to simplify the |
thanks @liviutinta - i've rejigged it slightly (prefer to keep just two functions, rather than splitting out into helper functions for special cases). think it still all works as it should. did the changes to the explanations/prose make (more) sense? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no problem understanding the definitions now. Really like the clock analogy as it makes it very clear what is going on.
For azimuth I believe it would be useful to specify that Y values increase in the direction of "6 o'clock" to mitigate any misunderstanding of the coordinates system.
- add more special-casing to handle potential for `altitudeAngle` being `0` - HTML-encode `<` and `>` inside the `<code>` block
@@ -355,6 +359,14 @@ <h2><code>PointerEvent</code> Interface</h2> | |||
<dd> | |||
<p>The clockwise rotation (in degrees, in the range of [0,359]) of a transducer (e.g. pen stylus) around its own major axis. For hardware and platforms that do not report twist, the value MUST be 0.</p> | |||
</dd> | |||
<dt><dfn>altitudeAngle</dfn></dt> | |||
<dd> | |||
<p>The altitude (in radians) of the transducer (e.g. pen stylus), in the range [0,π/2] - where 0 is parallel to the surface (X-Y plane), and π/2 is perpendicular to the surface. For hardware and platforms that do not report tilt or angle, the value MUST be 0.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the default value of altitude should match the default value of tiltX/TiltY. So if they are 0 (for example when things aren't supported) then altitude should be pi/2.
One general suggestion is that if you add a new line after every comma or sentence it avoids the long lines here and easier to review. This is what I have seen in other specs as well and people seem to be generally happy with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
regarding the default value: this matches what touch events level 2 says https://w3c.github.io/touch-events/#dom-touch-altitudeangle
i've also tested this on an ipad (with no stylus, just using finger) and a modified version of my HUD tracker (that explicitly still shows touch events, rather than pointer events) https://patrickhlauke.github.io/touch/tracker/multi-touch-tracker-pointer-hud-toucheventsonly.html and indeed azimuthAngle
and altitudeAngle
are both defined (so the props exist), and return 0
. while i agree that it would be logical for the default to be pi/2, i think it's better to match touch events and what current implementations there do for defaults.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as for line length, agree...but as most other text in our spec already breaks this idea, i just went with this approach at this point. may need a good separate isolated reformatting pull request
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just trying to follow the same logic we did for pressure. In TouchEvent spec force is also zero if the device doesn't support it. But in PointerEvents it is 0.5 as we thought it makes more sense for the apps to still work with those devices.
With the same argument I believe we should diverge from TouchEvent spec in this case as well and set azimuth to pi/2 when it is not supported by the hardware. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, only saw this comment after i went ahead with the merge...
while yes, we diverged for the pressure, i think here the danger / potential for confusion is greater because we went for exactly the same property names as touch events. happy to revisit though if you/others feel strongly about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe altitudeAngle
of 0
is simply assumed as being the default / not supported value because it's impossible (i assume) to really have that situation, since when the stylus is truly resting fully flat on the surface, the stylus tip isn't touching the screen itself so wouldn't be detectable anyway (unless you have a stylus that is "hover-capable" like, say, the pen on the galaxy note). i assume the apple pencil doesn't do hovering, in which case this assumption of "zero can't be the 'real' value" may make sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
confirmed (as i don't have a device myself) here https://twitter.com/yatil/status/1253658517241765895
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember another reason for not exposing a "not supported" value was that developers didn't need to check for it every time. They just use the attribute and it works reasonably.
However, if we do go down the route of exposing a value that developers need to check for not supported case and do something other than what they would do with the actual value we are exposing I personally prefer to be much more explicit on that. Like expose an N/A or something that is clearly outside the range of possible values. I agree that Apple pen doesn't send 0 altitude at this point but I think we should be future proofing the API as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question is if even for non-apple-pencil stylus scenarios if there's a situation where a completely flat stylus would be detected or not.
i'm just thinking that IF there are web apps already dealing with altitudeAngle for touch events/pencil, they'd be familiar with the "if it's zero, just ignore this altogether" scenario. and because we use the exact same property name in PE, it may cause more confusion / need for forking code if in PE we then have different default.
either way yes, we need to document this more clearly either way, and also look at the pseudocode to check for that edge case/scenario.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
split this out into #320
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beside the comment regarding the value of altitude needing to be pi/2 when it is not supported everything else looks good to me.
Not forgetting that we'll then also need matching w3c tests |
Adapted the description slightly from https://w3c.github.io/touch-events/
Admittedly jotted down very handwavy advice note about implementations - this needs reviewing. Also, if this is taken up, we'll need relevant W3C tests for these new attributes.
Closes #274
Preview | Diff