immersive-web / anchors

https://immersive-web.github.io/anchors/
Other
50 stars 20 forks source link

Deleted Anchors still present in frame.trackedAnchors for that frame #66

Open AdaRoseCannon opened 3 years ago

AdaRoseCannon commented 3 years ago

This is in Samsung Internet Beta (Chromium M90) i'm not sure if this is a spec issue or an implementation issue but is something I am encountering.

i.e.

anchor.delete()

trackedAnchors.forEach(function (anchor) {
  // Uncaught DOMException: Failed to read the 'anchorSpace' property from 'XRAnchor': Unable to access anchor properties, the anchor was already deleted.
  const anchorPose = frame.getPose(anchor.anchorSpace, refSpace);
});
bialpio commented 3 years ago

Looks like a spec issue, the algorithm for XRAnchor.delete() only modifies the session's set of tracked anchors, which would only affect subsequent frames.

The fix I think would be to adjust XRAnchor to also hold an associated XRFrame - this association would need to be updated every time an anchor is added to XRFrame's set of tracked anchors (step 3.3 in update anchors algorithm), & modify the frame's set of tracked anchors when an anchor gets deleted.

Note that the change would break the current behavior. Additionally, we'll potentially be removing anchors from a collection when iterating over that collection (e.g. if the app deletes some anchors in the .forEach()) - I think it's OK to do, but I'm not certain. We could also stick to the current behavior and add a note to the spec that an application should be careful about not using anchors that have been deleted, & provide this snippet as an example to watch out for.

AdaRoseCannon commented 3 years ago

I wrapped the instance in try-catch but isn't an ideal solution. Especially in a loop in a RAF.

On Thu, 5 Aug 2021, 19:10 Piotr Bialecki, @.***> wrote:

Looks like a spec issue, the algorithm for XRAnchor.delete() only modifies the session's set of tracked anchors, which would only affect subsequent frames.

The fix I think would be to adjust XRAnchor to also hold an associated XRFrame - this association would need to be updated every time an anchor is added to XRFrame's set of tracked anchors (step 3.3 in update anchors algorithm), & modify the frame's set of tracked anchors when an anchor gets deleted.

Note that the change would break the current behavior. Additionally, we'll potentially be removing anchors from a collection when iterating over that collection (e.g. if the app deletes some anchors in the .forEach()) - I think it's OK to do, but I'm not certain. We could also stick to the current behavior and add a note to the spec that an application should be careful about not using anchors that have been deleted, & provide this snippet as an example to watch out for.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/immersive-web/anchors/issues/66#issuecomment-893675029, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABAHSMV7OYKE5NLPFLXC2XDT3LHXXANCNFSM5BUIN6HQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email .

AdaRoseCannon commented 3 years ago

Adding an anchor.isDeleted property so it's possible to check on the fly isn't as neat but at least it would be better than catching and ignoring an error and wouldn't break existing implementations.

bialpio commented 3 years ago

From today's meeting: Another alternative would be to no longer throw exceptions on property access for anchors that have been deleted, but instead have the returned XRSpace be no longer locatable. This'd be my preferred choice I think, but note the following:

let anchor = ...; // some anchor
let pose_1 = frame.getPose(anchor.anchorSpace, refSpace); // returns non-null
anchor.delete();
let pose_2 = frame.getPose(anchor.anchorSpace, refSpace); // returns null

I can see this as something that may not be expected by the app developers. If others think it's actually not something that would raise their eyebrows, let's go with this approach.