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

MSC3489: Sharing streams of location data with history #3489

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

ara4n
Copy link
Member

@ara4n ara4n commented Nov 14, 2021

@ara4n ara4n added the proposal A matrix spec change proposal label Nov 14, 2021
@ara4n ara4n marked this pull request as draft November 14, 2021 20:58
@vranki
Copy link

vranki commented Nov 19, 2021

Looks good, but I don't like this part: "m.beacon.* events should be sent every 2 seconds while the location of the asset is moving. If the asset is not moving, it should be refreshed every 30 seconds."

The location data can come from another source via bridge or bot and this data source may have it's own update interval. Also the practical update rate depends on application. Some applications may need 2 seconds, but some may be happy with 60 secs or even longer. An application may choose use smart algorithm that calculates error between true location and location extrapolated from velocity vector and updates only if it grows high enough.

I'd change this to recommendation, although 2 secs is quite high rate, even for moving cars or people being followed live on map. Maybe there should also be a field for maximum time of next update. If the update isn't sent before this, the recipient can assume that sender was unable to send update for some reason.

relax rate requirements
spell out that we're ignoring velocity for now
@ara4n
Copy link
Member Author

ara4n commented Nov 21, 2021

feedback incorporated - thank you. (please always provide feedback by commenting on the relevant line of the PR, or the heading if there isn't a specific relevant line, so that we can track the feedback & resolve it as a thread, until github finally implements threaded comments...)

@ananace
Copy link
Contributor

ananace commented Nov 30, 2021

Sneaking in a little note here that another alternative could be done with a combination of MSC2477 and a slower rate of state events as in here for the "breadcrumb trail" - even described that particular use-case as an example in the proposal there. Something much like this (like Telegram's live location) was one of the original thoughts for a use-case when I first wrote that proposal after all.

@turt2live turt2live added client-server Client-Server API kind:feature MSC for not-core and not-maintenance stuff needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. labels Dec 20, 2021
* Various tweaks following initial implementation:
- switched from `m.beacon.*` state event to `m.becon` reference relationship to original `m.beacon_info.*`
- changed `beacon_info` created to `m.ts`
- renamed `lifetime` to `timeout`
- added `m.asset` `type`
@ara4n ara4n changed the title MSC3489: Sharing streams of location data MSC3489: Sharing streams of location data with history Jan 25, 2022
* Clarify that this proposal includes history - see MSC3672 for EDUs
* Note dependency on MSC3267 for reference relationships
* Remove variable event_type - instead use state_key
* Propose state_key prefixes inspired by MSC3757 and MSC3779
* Update links to matrix-spec-proposals
* Clarify how to stop sharing, and when a share is stopped
* Mention the need for redactions to work
* Clarify the use of milliseconds for all timing
* Mention implementations
events will automatically have permission to share their live location, because
the `state_key` matches the pattern required to be an "owned" state event.

## Alternatives
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is another alternative that appears not to have been discussed yet, although the idea is quite obvious:

Instead of introducing a new event m.beacon that for the most part duplicates an MSC3488 m.location:

  • use m.location
  • when used for location streams (i.e. in this context and MSC3672), require the m.relates_to mixin to be set

Pros compared to the MSC as it currently is:

  • automatic fallback for clients that support static m.location
  • further fallback to m.text

Cons:

  • this could result in clients without live location support having their timelines spammed. while one could still view the movement trail similar to the original intention, this might be considered undesireable

`description` is the same as an `m.location` description
([MSC3488](https://github.com/matrix-org/matrix-spec-proposals/pull/3488)).

`live` should be true when a user starts sharing location.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When live is false, m.ts and timeout become meaningless, right? So those fields are optional?

If they are, couldn’t we get rid of live and say that the location sharing is live when both m.ts and timeout are present and the timeout is not reached?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could these still be relevant for displaying clients to visualize the time interval during which location has been shared? I guess they could also use the m.ts fields from the first and last m.beacon event for the same purpose. So maybe that's irrelevant. 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client-server Client-Server API kind:feature MSC for not-core and not-maintenance stuff needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. proposal A matrix spec change proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.