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

first pass at persistent anchors #75

Merged
merged 6 commits into from Sep 16, 2022
Merged

Conversation

cabanier
Copy link
Member

@cabanier cabanier commented Jul 14, 2022

@probot-label probot-label bot added the agenda label Jul 14, 2022
@cabanier
Copy link
Member Author

More work needs to be done to describe how the UUIDs are stored by the UA.

@AdaRoseCannon
Copy link
Member

Thanks @cabanier I am looking forward to this

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@bialpio
Copy link
Contributor

bialpio commented Jul 14, 2022

More work needs to be done to describe how the UUIDs are stored by the UA.

Took a quick pass now. A couple of questions:

  • Are you planning on expanding this PR w/ details on how storing the UUIDs will be done, or will that be a separate PR?
  • Do we want to add additional feature descriptor for persistent anchors, or will the requestPersistentHandle() just fail in case the UA does not support this capability?

@cabanier
Copy link
Member Author

More work needs to be done to describe how the UUIDs are stored by the UA.

Took a quick pass now. A couple of questions:

  • Are you planning on expanding this PR w/ details on how storing the UUIDs will be done, or will that be a separate PR?

I was planning on extending this PR; otherwise the spec would be in an incomplete state.
Maybe that doesn't matter?

  • Do we want to add additional feature descriptor for persistent anchors, or will the requestPersistentHandle() just fail in case the UA does not support this capability?

I would prefer if we keep the same descriptor and just fail if the UA doesn't support persistence.

@bialpio
Copy link
Contributor

bialpio commented Jul 14, 2022

More work needs to be done to describe how the UUIDs are stored by the UA.

Took a quick pass now. A couple of questions:

  • Are you planning on expanding this PR w/ details on how storing the UUIDs will be done, or will that be a separate PR?

I was planning on extending this PR; otherwise the spec would be in an incomplete state. Maybe that doesn't matter?

Extending the PR sounds good (and it'll be easier to see all the changes needed for persistent anchors).

  • Do we want to add additional feature descriptor for persistent anchors, or will the requestPersistentHandle() just fail in case the UA does not support this capability?

I would prefer if we keep the same descriptor and just fail if the UA doesn't support persistence.

Sounds good to me!

@thetuvix
Copy link
Contributor

Have a conflict for today's call. However, I took a quick look through this PR and the general approach looks good to me! It should be straightforward to implement this on top of the XR_MSFT_spatial_anchor_persistence OpenXR extension.

The major design point worth talking through is whether it's more "webby" for this API to return the persistent anchor keys upon the request, or to accept the desired key from the app. Microsoft's underlying XR_MSFT_spatial_anchor_persistence OpenXR extension accepts a key from the app, though I don't feel strongly about that approach - it seems OK to require the app to tie the UA-generated key to some meaning in its own storage.

The other thing I would be sure to clarify when you write up prose is that no promises are made that this key will be usable on any other device - this API is about local-device anchor persistence rather than cross-device anchor sharing. For privacy reasons, we may actually want the line in the spec that mentions that the keys returned must be unique per-domain to also mention that they also must be unique per-device.

@Yonet Yonet removed the agenda label Jul 26, 2022
@cabanier
Copy link
Member Author

The other thing I would be sure to clarify when you write up prose is that no promises are made that this key will be usable on any other device - this API is about local-device anchor persistence rather than cross-device anchor sharing. For privacy reasons, we may actually want the line in the spec that mentions that the keys returned must be unique per-domain to also mention that they also must be unique per-device.

Good point! I'll add that to the spec.

@cabanier
Copy link
Member Author

cabanier commented Sep 2, 2022

/tpac discuss persistent anchor proposal

@probot-label probot-label bot added the TPAC label Sep 2, 2022
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
@bialpio
Copy link
Contributor

bialpio commented Sep 16, 2022

Looks good to me, w/ minor nitpicks that can be addressed later.

github-actions bot added a commit that referenced this pull request Sep 16, 2022
SHA: a8f3e88
Reason: push, by @cabanier

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@Yonet Yonet removed the TPAC label Sep 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants