batch the raycaster intersection cleared events on the raycaster el

[ngokevin]

Description:

Rather than emit an event + object on the raycaster entity on each cleared intersection, emit one with no event detail, and event handlers can look it up themselves, but this event is rarely used.

Changes proposed:

• raycaster.clearedIntersectedEls member.
• Emit raycaster-intersection-cleared only once per tick.

[vincentfretin]

super-hands component uses el property on the event. How do we get the intersected element now?
see https://github.com/wmurphyrd/aframe-super-hands-component/blob/4c6b44a09cc893bdfe1828e6576466d019047032/index.js#L366 for unWatch.
and https://github.com/wmurphyrd/aframe-super-hands-component/blob/4c6b44a09cc893bdfe1828e6576466d019047032/index.js#L325 for unHover.

cc @wmurphyrd

[vincentfretin]

This will fail only for GearVR and Daydream, because other controllers don't use raycaster, but sphere collider.
I guess in super-hands, we need to get the raycaster component with
var raycaster = this.el.querySelector('[raycaster]') and iterate over raycaster.clearedIntersectedEls. But I'm not sure what do to with your if condition @wmurphyrd to detect if it's a raycaster event or an sphere-collider event.

[vincentfretin]

Let's continue the discussion there c-frame/aframe-super-hands-component#79

[ngokevin]

Yeah, you'd iterate over them and clear them all at once versus one at a time.

[wmurphyrd]

What's the advantage of not including clearedIntersectedEls in the details of the raycaster-intersection-cleared event?

The disadvantage is other components would have to directly access the raycaster component instance object to learn the details, something I thought was discouraged

[ngokevin]

Component members can be part of the public API, it's not discouraged to expose properties/methods. Not including would just save creating an object on each tick for garbage collection purposes. Is it able to fix?

[ngokevin]

Oh, never mind, I guess you can maybe pass the array without creating an object.

[machenmusik]

This breaks everything using raycaster, right? As there is no way AFAIK to detect the behavior change, if we’re going to break the existing contract, maybe better to change event name so compatibility can be achieved by supporting both old and new event names...

[dmarcos]

@wmurphyrd I opened #3128 to pass the cleared elements as a detail of the event.

[ngokevin]

I think versions are allowed to have some small breaking changes, especially in 0.x.x, if there's some gains, as long as things can easily adapt. It'd only break things directly using the raycaster events emitted from the raycasting entity, which I thought was not used often. The contract only applies within versions, but yeah, I wish it didn't break things.

[machenmusik]

Maybe it would be nicer to have a helper component that adds a listener that calls the prior contract, so if you need the old behavior you can add raycaster-legacy-events or sonething

[machenmusik]

I completely agree that small breaking changes are allowed, but now that the framework has more traction and wider usage, correspondingly small affordances to ease migration seem more worthwhile to consider. Along the lines of gltf next and legacy, having legacy helper components that throw deprecation warnings once for a point release might be a reasonable approach?