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

Investigate possibility of synchronizing coordinate systems #257

Closed
alexshalamov opened this issue Aug 19, 2017 · 35 comments

Comments

Projects
None yet
7 participants
@alexshalamov
Copy link

commented Aug 19, 2017

Many sensors operate in device coordinate system, where X and Y axes are aligned with screen X & Y axes in the natural orientation.

When OS changes rendering surface orientation from it's natural orientation, sensor and screen coordinate systems may become out of sync, thus, require usage of ScreenOrientation API, matrix math, swapping of X / Y or changing Euler angle order.

Would be nice to have a way to say to such sensors to be in sync with screen coordinate system, so that extra calculations and/or API usage is not required from developers.

@anssiko

This comment has been minimized.

Copy link
Member

commented Aug 22, 2017

@larsgk has provided related feedback in the past in the context of the Screen Orientation API. Looping him in to gather further feedback, since it's been a while.

@larsgk, for the context, we're discussing whether there should be affordances in the low-level sensor APIs^ Accelerometer, Gyroscope Magnetometer, Orientation Sensor to sync the local coordinate system used by these sensors with the current orientation type of the screen, defined by the Screen Orientation API.

Currently, the local coordinate system used by these low-level sensors is said to be "relative to the device’s screen when the device in its default orientation", aka the natural orientation in the Screen Orientation API.

^ This is a new set of sensor APIs that decompose the Device Orientation into low-level primitives.

@anssiko

This comment has been minimized.

Copy link
Member

commented Aug 22, 2017

@larsgk

This comment has been minimized.

Copy link

commented Dec 7, 2017

sorry, I missed this one (drowned in github notification mails, I think).

Is it still relevant?

@anssiko

This comment has been minimized.

Copy link
Member

commented Dec 19, 2017

@kenchris' https://github.com/kenchris/sensor-polyfills now implements the synchronization of the sensor and screen coordinate systems as discussed in this issue. The sync is enabled by default in the polyfill for testing purposes.

To try it out:

When you change the orientation from portrait to landscape and back, you should see the back of the rendered object always point towards the center of the Earth.

Would people like to see this feature cherry picked into the v1 spec and implemented natively?

If so, we should spec the opt-in mechanism. The most obvious extension point would be to add a new flag to SensorOptions defaulting to what is currently specified.

I'll start the bikeshedding with an initial name proposal for such a flag: syncScreenCoords 😇

let orientation = new AbsoluteOrientationSensor({ syncScreenCoords: true });
@anssiko

This comment has been minimized.

Copy link
Member

commented Dec 19, 2017

@alexshalamov made a point that the feature is specific to sensors that operate in a device coordinate system (currently motion sensors) and as such is not applicable to environment sensors.

So rather than extending the generic SensorOptions directly, we'd spec derived dictionaries that inherit from SensorOptions per the example WebIDL, e.g.:

dictionary OrientationSensorOptions : SensorOptions {
  boolean syncScreenCoords = false;
};

[Constructor(optional OrientationSensorOptions orientationSensorOptions),
 SecureContext, Exposed=Window]
interface AbsoluteOrientationSensor : OrientationSensor {
};

Or alternatively, spec MotionSensorOptions : SensorOptions to be reused by all motion sensors.

@pozdnyakov

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2018

I'm not in favor of using SensorOptions for it, IMHO such API would be quite opaque (e.g. function readDataFromSensor(sensor) { /* what coord system this sensor uses? */ }

IMO it's simpler from both implementation and usage perspectives if sensors always stick to the device coord system, like it happens now. However we could provide some convenience conversion functions (or methods), like they do on Android.

For example, OrientationSensor.remapCoordinateSystem(matrix, screenOrientation), where matrix is rotation matrix and screenOrientation is screen orientation type.

@kenchris

This comment has been minimized.

Copy link
Collaborator

commented Jan 11, 2018

I do automatic mapping in the polyfill right now, https://github.com/kenchris/sensor-polyfills/blob/master/src/motion-sensors.js#L310

Basically that makes things move along with screen orientation, ie. if an object is being rotated, then it will stay in the same place (from user point of view) when the screen changes orientation. In my experience that is the most common use-case and what feels more natural.

Unless you need to do your own mapping, I think this is what makes the most sense, which is why I would like to have that be the default.

Most people get really confused about coordinate systems (even me!) and how to do these mapping, so I rather let the burden on who really know what they are doing (so they do the extra work to turn off the auto mapping and add their own) and make things behave like what normal people would expect :-)

@kenchris

This comment has been minimized.

Copy link
Collaborator

commented Jan 11, 2018

We could provide a remap method or similar which is implemented natively and used by default. If people want to provide their own, or turn off remapping, they would just reimplement that method.

@alexshalamov alexshalamov removed this from API improvements in Level 2 Jan 12, 2018

@alexshalamov alexshalamov modified the milestones: Level 2, Level 1 Jan 12, 2018

@alexshalamov alexshalamov added this to Tasks in Level 1 Jan 12, 2018

@pozdnyakov

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2018

After taking a deeper look at this issue I think:

  1. At the moment we do not need such a powerful tool as the remapCoordinateSystem function in Android, also an extra function call might annoy the users :)
  2. It looks risky to rely on orientation types for the sensor data mapping, as orientation type values are assigned arbitrary by the UA.

After discussing this with @alexshalamov we'd like to propose the following plan for fixing the issue:

   TriaxialSensorOptions : SensorOptions {
      DOMString coordSystem = "device"
   }

to be used for construction of all motion sensors

 // Device coordinate system
let accel = new Accelerometer({ coordSystem:"device" });  // or new Accelerometer(); 

// Screen coordinate system
let orientSensor = new RelativeOrientationSensor({ coordSystem:"screen" });

In future we could add coordSystem:"custom" + user-provided remapping function as @kenchris proposed.

@pozdnyakov pozdnyakov moved this from Tasks to Security & Privacy tasks in Level 1 Jan 18, 2018

@pozdnyakov pozdnyakov moved this from Security & Privacy tasks to In Progress tasks in Level 1 Jan 18, 2018

@alexshalamov

This comment has been minimized.

Copy link
Author

commented Jan 19, 2018

@pozdnyakov 👍 and to provide backward compatibility with older APIs, and native platform behavior, by default, sensors would be providing data in 'device coordinate system'.

@anssiko

This comment has been minimized.

Copy link
Member

commented Jan 19, 2018

All - please review @pozdnyakov's proposed solution #257 (comment) and let us know your feedback.

Unless we hear concerns from the group participants, I'd ask the editors to start prepare a PR to address this issue in order to get it landed before we branch for the CR release in the near future.

As discussed on the 11 Jan WG call, the motivation for the group to cherry pick this feature into Level 1 was feedback received from web developers during the still ongoing Chrome Origin Trial. This feature will hide the complexity of remapping of coordinate systems from web developers -- a process that is both tedious and error prone. There's also small performance advantage in handling this remap natively.

@kenchris

This comment has been minimized.

Copy link
Collaborator

commented Jan 19, 2018

I am fine with this. 👍

Should we spell out coordinate? like coordinateSystem or does the Web Platform use coord in other places?

@pozdnyakov

This comment has been minimized.

Copy link
Contributor

commented Jan 19, 2018

@kenchris yeah, it make sense to spell it out like coordinateSystem

@kenchris

This comment has been minimized.

Copy link
Collaborator

commented Jan 19, 2018

I am wondering whether

Sensor {
readonly attribute SensorOptions? options;
}

is the best way or not. I think most other APIs apply the individual options directly on the object, like

Sensor {
  readonly attribute number frequency;
  readonly attribute USVString coordinateSystem;
}

Basically a bit like they were applied to the object,

sensor = {}
options = { coordinateSystem: "screen" }
Object.assign(sensor, options)

sensor.coordinateSystem
> "screen"

Having it as "options" makes it seem like what was given to the constructor, where as having it as class fields (properties) makes it feel like what was applied

@pozdnyakov

This comment has been minimized.

Copy link
Contributor

commented Jan 19, 2018

This looks convenient for frequency, however coordinateSystem cannot be applied to every sensor class. I'd like to obviate repeated declaration: Accelerometer.coordinateSystem, Gyroscope.coordinateSystem and so on.. Sensor.options just looks more generic.

@kenchris

This comment has been minimized.

Copy link
Collaborator

commented Jan 19, 2018

That is a spec'ing issue. I haven't seen any API where the options were stored seperately, and I do think it support from the problem I stated above

Having it as "options" makes it seem like what was GIVEN to the constructor, where as having it as class fields (properties) makes it feel like what was APPLIED

We need to define coordinateSystem for the sensors who needs it anyway, so adding one attribute to those specs, shouldn't really be much work. @anssiko maybe you have ideas?

@pozdnyakov

This comment has been minimized.

Copy link
Contributor

commented Jan 19, 2018

Having it as "options" makes it seem like what was given to the constructor, where as having it as class fields (properties) makes it feel like what was applied

I see your point now, the problem with returning the applied attributes is that for some of them (i.e. frequency) we do not know their values until the sensor is started..

I suggest to remove this item from the plan for now and address it separately in future as it's just for convenience and not mandatory for solving this particular issue. WDYT?

@anssiko

This comment has been minimized.

Copy link
Member

commented Jan 19, 2018

@kenchris, the extensibility story recommended by the spec has been to extend SensorOptions, see https://w3c.github.io/sensors/#example-webidl I think we could make returning of the applied attributes a new Level 2 feature and/or try to resolve it as part of #63.

What comes to the naming coord(s) vs. coordinate(s). There's some precedence for both:

https://html.spec.whatwg.org/#dom-a-coords
https://html.spec.whatwg.org/#attr-area-coords
https://dev.w3.org/geo/api/spec-source.html#coords
https://dev.w3.org/geo/api/spec-source.html#coordinates_interface

I tend to feel spelling out coordinateSystem would be better:

  • Although attribute names and dictionary member names tend to be short and concise, there are longer names in use than that.
  • Longer form avoids confusion with the short plural form coords i.e. no coordsSystem typo opportunity.
  • This is not getElementById or getElementsByTagName that gets typed 10s of times per app :-)
@anssiko

This comment has been minimized.

Copy link
Member

commented Jan 22, 2018

Since dictionaries are not JS-exposed, their names are for spec purposes only. As such I wouldn’t worry too much about having the names perfect. Perhaps inheritance is slightly easier to spec in this case.

(Related, there’s an inconclusive issue on adding feature detection mechanism for dictionary members: heycam/webidl#107)

pozdnyakov pushed a commit to pozdnyakov/sensors that referenced this issue Jan 30, 2018

Mikhail Pozdnyakov
Add "Local Coordinate System" concept
This is required for fixing w3c#257

@pozdnyakov pozdnyakov self-assigned this Jan 30, 2018

pozdnyakov pushed a commit to pozdnyakov/sensors that referenced this issue Jan 31, 2018

Mikhail Pozdnyakov
Add "Local Coordinate System" concept
This is required for fixing w3c#257

pozdnyakov pushed a commit to pozdnyakov/sensors that referenced this issue Jan 31, 2018

Mikhail Pozdnyakov
Add "Local Coordinate System" concept
This is required for fixing w3c#257

pozdnyakov pushed a commit to pozdnyakov/sensors that referenced this issue Feb 1, 2018

Mikhail Pozdnyakov
Add "Local Coordinate System" concept
This is required for fixing w3c#257

pozdnyakov pushed a commit to pozdnyakov/gyroscope that referenced this issue Feb 6, 2018

Mikhail Pozdnyakov
Gyroscope can use the screen local coordinates
The `SpatialSensorOptions` dictionary and the
"construct spatial sensor object" are used, so that
the client can set the local coordinate system for a
Gyroscope instance from the constructor.

This patch is a part of fix for w3c/sensors#257

pozdnyakov pushed a commit to pozdnyakov/gyroscope that referenced this issue Feb 6, 2018

Mikhail Pozdnyakov
Gyroscope can use the screen local coordinates
The `SpatialSensorOptions` dictionary and the
"construct spatial sensor object" are used, so that
the client can set the local coordinate system for a
Gyroscope instance from the constructor.

This patch is a part of fix for w3c/sensors#257

pozdnyakov pushed a commit to pozdnyakov/gyroscope that referenced this issue Feb 7, 2018

Mikhail Pozdnyakov
Gyroscope can use the screen local coordinates
The `SpatialSensorOptions` dictionary and the
"construct spatial sensor object" are used, so that
the client can set the local coordinate system for a
Gyroscope instance from the constructor.

This patch is a part of fix for w3c/sensors#257

alexshalamov pushed a commit to alexshalamov/magnetometer that referenced this issue Feb 7, 2018

Alexander Shalamov
Magnetometer can use the screen local coordinates
The `SpatialSensorOptions` dictionary and the
"construct spatial sensor object" are used, so that
the client can set the local coordinate system for a
Magnetometer instance from the constructor.

This patch is a part of fix for w3c/sensors#257

alexshalamov pushed a commit to alexshalamov/magnetometer that referenced this issue Feb 8, 2018

Alexander Shalamov
Magnetometer can use the screen local coordinates
Introduce MagnetometerSensorOptions that can control whether screen
or device coordinate system is used.

This patch is a part of fix for w3c/sensors#257

alexshalamov pushed a commit to alexshalamov/magnetometer that referenced this issue Feb 8, 2018

Alexander Shalamov
Magnetometer can use the screen local coordinates
Introduce MagnetometerSensorOptions that can control whether screen
or device coordinate system is used.

This patch is a part of fix for w3c/sensors#257

alexshalamov pushed a commit to alexshalamov/magnetometer that referenced this issue Feb 8, 2018

Alexander Shalamov
Magnetometer can use the screen local coordinates
Introduce MagnetometerSensorOptions that can control whether screen
or device coordinate system is used.

This patch is a part of fix for w3c/sensors#257

alexshalamov pushed a commit to alexshalamov/magnetometer that referenced this issue Feb 8, 2018

Alexander Shalamov
Magnetometer can use the screen local coordinates
Introduce MagnetometerSensorOptions that can control whether screen
or device coordinate system is used.

This patch is a part of fix for w3c/sensors#257

pozdnyakov pushed a commit to pozdnyakov/orientation-sensor that referenced this issue Feb 8, 2018

@pozdnyakov

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2018

All the spatial sensor classes are now updated, so that the screen coordinate system is used to resolve their readings once the user passes {referenceFrame:"screen"} to the constructor.

@pozdnyakov pozdnyakov closed this Feb 8, 2018

@anssiko anssiko removed this from In Progress tasks in Level 1 Feb 8, 2018

@rakuco

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2018

Hey guys,

@foolip and I have been monitoring spec changes without corresponding tests, and it looks like the commits referenced here fall into this category.

For example, most of the IDLs in interfaces/ haven't been updated except for web-platform-tests/wpt#9357. Is there anything preventing WPT updates from happening concurrently with the spec changes?

@anssiko

This comment has been minimized.

Copy link
Member

commented Feb 14, 2018

Chinese New Year. @Honry will get to this when he’s back in business.

@Honry

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2018

@rakuco, thanks for your reminder, I just merge the rest PRs for updating the IDLs in interfaces/.

And will consider some functional tests for this new feature after my vacation.

@rakuco

This comment has been minimized.

Copy link
Contributor

commented Feb 19, 2018

Got it, thanks @Honry and @anssiko. I thought it'd be easier for the same people who changed the spec to also update the tests accordingly, but no objections if the current workflow is working for you :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.