immersive-web / anchors

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

Does XRAnchor.detach() imply that the unattached anchor lives on? #34

Closed thetuvix closed 4 years ago

thetuvix commented 4 years ago

Agreed on having an explicit method for destroying an anchor synchronously. However, I also agree with @blairmacintyre that the name detach() implies to me that this newly-detached object will live in on some meaningful unattached way, when in fact there's no longer anything meaningful you can do with that anchor.

I wonder if destroy(), drop(), forget() or simply stopTracking() (to align with trackedAnchors) makes it more clear when you'd call this method: when you're no longer interested at all in that anchor.

I'll make a specific issue for bikeshedding detach() so those with more web API experience than me can call out similar cases in other web APIs.

Originally posted by @thetuvix in https://github.com/immersive-web/anchors/issues/10#issuecomment-600901756

bialpio commented 4 years ago

The newly detached object is roughly as useful as the anchor whose tracking is forever lost - you could theoretically obtain an XRSpace out of it, but you'd never get a meaningful pose out of it. I'm not sure if we could argue that this makes the object alive in a meaningful way.

Additionally, in case of detaching an anchor, I have decided to throw an InvalidStateError if the applications decide to access its properties - the reasoning was that detaching an anchor is app-driven, so it should know what it already detached and not try to access the properties. This may influence the name we decide on.

thetuvix commented 4 years ago

Yea, after you call detach(), it's like a zombie anchor - seems to be alive but will only bite you if you interact with it 🧟 We should find a name that makes that clear!

I do agree with the functional design of having implicit anchor loss from the UA side leave the object functional as a dummy object, to avoid timing issues, but having the object properly go dead if you explicitly call detach().

bialpio commented 4 years ago

Given that we do not really have a way of consuming the object on which a method is called (Rust-style), delete(), destroy(), stopTracking() or detach() all sound reasonable to me, with varying degrees of preference.

Some more notes:

I'd say that delete() should be the way to go here, unless anyone disagrees.

thetuvix commented 4 years ago

I had initially preferred destroy() to delete() because it somehow felt better as an instance method, but I can't really find much web precedent to back up my feeling 😄

In C#, there is an explicit pattern for an instance method that releases external resources: Dispose(). Is there any such precedent on the web? If not, delete() seems reasonable to me here.

bialpio commented 4 years ago

There's also remove(), for example ChildNode.remove(). In general, based on the number of results that caniuse.com returns, there does not seem to be a well-established pattern for the name, both return similar amount of results, with small advantage going to remove (49 vs 51): https://caniuse.com/#search=remove https://caniuse.com/#search=delete

I still prefer delete() so if there's no opposition, I'll issue a PR soon.

raviramachandra commented 4 years ago

Not sure, if this is the right place. But from the spec it was not clear if we can arbitrarily call for "anchor removal" (Section 6) or should we call only within a RAF callback ?

tencircles commented 4 years ago

Destroying things on the web is pretty uncommon. I think childNode.remove() is perhaps not the best example. This just detaches the node from its parent, the node is still perfectly alive. Regarding delete, generally this is used in cases where an attribute is removed from a collection, not where something is being disposed of.

This brings up a good point however, disposing of things on the web is typically left to the browser implementation. Even web audio nodes are left up to the implementation to dispose of when they need to be garbage collected. To this end, would it be an option that rather than having to explicitly dispose of an anchor, it can be disposed of by the implementation after all references to it have been deleted?

bialpio commented 4 years ago

Not sure, if this is the right place. But from the spec it was not clear if we can arbitrarily call for "anchor removal" (Section 6) or should we call only within a RAF callback ?

I think we don't have to require removal to happen within rAF callback - the implementation can immediately mark the anchor internally as no longer fully functional (to ensure that accessing properties is failing after it was removed) and notify the device as soon as it can.

This brings up a good point however, disposing of things on the web is typically left to the browser implementation.

In general I agree with the approach, but I think this is one of the cases where we should expose a method to proactively mark the object as no longer needed (there is a cost associated with maintaining anchors so the application has a way of reducing such costs early instead of relying on GC). There is also a precedent - the already mentioned WebGLRenderingContext.deleteBuffer(), which would behave similarly (and I was actually bitten by the assumption that GC takes care of things in a test site - all was fine except for 0.25s freezes every now and then because GPU was busy deleting 100's of buffers to which I just dropped references). Without proactive mechanism to delete the anchor, the site has no way to control the behavior (as web APIs are not supposed to expose the GC in any way, so there is no way to trigger GC collection) & runs into the risk of paying for something it no longer needs.