enable non-recursive raycaster (fixes #2329) #2331

machenmusik · 2017-01-30T18:41:16Z

dmarcos · 2017-01-30T20:20:49Z

We need to fix the tests

machenmusik · 2017-01-30T20:52:08Z

this is turning out to be more insidious than expected... still working on it

i can see now why the implementation did what it did, in that once the change makes entities require geometry in order to intersect, a slew of the tests break because they never actually cared, and doing it correctly is not as simple as one would expect.

(frustrating, because functionally things work correctly...)

machenmusik · 2017-01-30T21:17:39Z

looks like there is an issue with refreshObjects() happening before geometry is attached (and thus there are no children of the entity's object3D to add yet). if there is some better way to understand when refreshObjects() can be invoked correctly, that would be most welcome...

machenmusik · 2017-01-30T23:53:43Z

ok now I am a little baffled, as npm run test:chrome works locally with all non-skipped tests passing, but apparently something in travis-ci changes, and things fail.

ngokevin · 2017-01-30T23:54:08Z

Probably just some race condition or timing issue.

codecov-io · 2017-01-31T00:29:01Z

Codecov Report

Merging #2331 into master will increase coverage by 0.03%.

@@            Coverage Diff            @@
##           master   #2331      +/-   ##
=========================================
+ Coverage   85.97%     86%   +0.03%     
=========================================
  Files         109     109              
  Lines        4519    4538      +19     
=========================================
+ Hits         3885    3903      +18     
- Misses        634     635       +1

Impacted Files	Coverage Δ
src/index.js	`97.72% <100%> (ø)`	✅
src/components/raycaster.js	`98.79% <96.29%> (-1.21%)`	❌

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 652f23f...d9c5ed2. Read the comment docs.

usernamealreadyis

T< TIME> [FATAL] AppErrorHandler: Error #1132: Invalid JSON parse input.
SyntaxError: Error #1132: Invalid JSON parse input.
at Error$/throwError()
at JSON$/parse()
at com.vmware.vsphere.client.model.vm.config.devices.connection::DeviceControllerServerConnection$/getConnectedCdroms()
at com.vmware.vsphere.client.views.vm.config.devices.connection::DeviceSummaryEntry/onMenuButtonClick()
Logger Stack Trace:
at com.vmware.vsphere.client.util::AppErrorHandler/onUncaughtError()

machenmusik · 2017-01-31T03:57:23Z

latest PR incorporates this...

ngokevin [8:00 PM] 
i suggest in the raycaster component, we do a refresh on:
1. `sceneEl.addEventListener('loaded')`
2. `sceneEl.addEventListener('child-attached')` + `loaded` on that attached child

usernamealreadyis

?/?

maissani · 2017-02-16T11:30:30Z

Any update about this fix ?

machenmusik · 2017-02-16T16:16:35Z

all the tests work, the latest version of PR uses strategy suggested,
maybe it got a bit buried in the push to 0.5.0...

I believe the raycaster interval fix was merged into 0.5.0 separately,
but the remainder is sitting ready and waiting

machenmusik · 2017-02-24T01:19:34Z

@dmarcos @ngokevin now that 0.5.0 rush is over, are we ok to take this?

ngokevin · 2017-02-25T02:06:33Z

What did we think about just removing the recursive property altogether and being recursive by default? I remember something about hard to figure out what element exactly was intersected in a big tree?

machenmusik · 2017-02-25T02:29:29Z

it can be significantly more expensive to do recursive raycast where non-recursive suffices as well, but perhaps @maissani and others can comment

cvan · 2017-04-18T13:27:55Z

@machenmusik did you still want to get this merged?

machenmusik · 2017-04-18T13:59:33Z

if it still works, sure... @maissani @donmccurdy is this PR still useful to you?

donmccurdy · 2017-04-19T04:52:33Z

I've forgotten the context here a bit, but as far as I remember: With raycaster="recursive:false, the raycaster tests against each entity's root Object3D, and will never detect anything. That's no good; we should either fix that or remove the option.

If we can simplify this PR a bit, great. I'll add proposed changes, if we choose to move forward, in separate comments.
Removing the recursive option would be OK, too. For users who understand the performance profile and scene graph enough to care, setting up a custom component with a THREE.Raycaster is not that difficult. If they're doing that kind of performance tuning, they should probably consider taking the querySelector out of the picture anyway.

donmccurdy · 2017-04-19T04:55:12Z

src/components/raycaster.js

+        el.removeEventListener('loaded', nowRefresh);
+        self.refreshObjects();
+      });
+    }


This is waiting for the current element, presumably the cursor, to load, right? Seems like a flaky indicator for the state of the rest of the scene. Plausible alternatives:

Refresh on first tick.

Refresh on scene load.

Refresh every time the scene has a loaded event, to pick up new entities.

wow this was from a long time ago... refreshing my memory as well...
I think this was supposed to be the last one, but listener should be on sceneEl not el

looking at it a little more closely now, shouldn't this happen when children are attached somewhere in the scene?
this.el.sceneEl.addEventListener('child-attached', this.refreshOnceChildLoaded);

I think I originally had some changes that would defer this work until next actual raycast (to coalesce / avoid spurious invocations) - but that seems to have gone, should we bring it back?

donmccurdy · 2017-04-19T04:58:40Z

src/components/raycaster.js

-      return;
+    // If objects not defined, intersect with everything.
+    if (objectsAreEls) {
+      objects = this.el.sceneEl.querySelectorAll(data.objects);


IMO this code would be more readable if, by the end of this first if/else block, the objects array is guaranteed to contain only Object3Ds. And, we can avoid calling objects.length on a NodeList in a loop; that's O(n²). Instead:

objects = this.el.sceneEl.querySelectorAll(data.objects); for (var i = 0, len = objects.length; i < len; i++) { this.objects.push(objectEls[i].object3D); }

had been assuming JIT would hoist the length call, but you're right that it can be explicit.

donmccurdy · 2017-04-19T05:04:01Z

src/components/raycaster.js

+        // If there aren't any children, then until a refresh after geometry loads,
+        // raycast won't see this object... but that should happen automatically.
+      }
+    }


With the change above, this block reduces to:

for (i = 0; i < objects.length; i++) { var children = objects[i].children; if (children) { this.objects.push.apply(this.objects, children); } }

(presumably this wants the explicit length hoisting described above)

Actually not here, on the explicit length hoisting. With normal arrays, doing i = 0; i < array.length; i++ is completely fine, clean, and fast.

But if you have a NodeList instead of an array (the result of querySelectorAll() is a NodeList, not an array) then calling .length is slower, as described here.

Heh, ok it's more complicated than I thought, there are "live" and "static" NodeLists, and the result of querySelectorAll() is the latter, which means it can probably be hoisted anyway. Live NodeLists are more trouble. Feel free to ignore my concern and I'll just patch it if I actually run into a noticeable performance issue. :)

ok, that's how I thought it worked, but I already proactively patched since code is still quite readable that way

since there appears to be a wrapper around an entity's object3D, use its children instead

… box add geometry so that raycaster actually adds something wait until next tick to satisfy Firefox; remove extra tick

machenmusik · 2017-04-19T15:25:43Z

(also rebased against something more recent)

machenmusik · 2017-04-20T23:05:41Z

@donmccurdy @dmarcos @ngokevin anything else on this one?

ngokevin · 2017-04-21T02:31:53Z

src/components/raycaster.js

+   */
+  refreshOnceChildLoaded: function (evt) {
+    var self = this;
+    var el = evt.detail.el;


I'd use childEl to not confuse with the raycaster.

ngokevin · 2017-04-21T02:32:23Z

src/components/raycaster.js

+    var self = this;
+    var el = evt.detail.el;
+    if (!el) { return; }
+    if (el.hasLoaded) { this.refreshObjects(); } else {


Let's put else blocks on their own level.

if (clause) { // ... } else { // ... }

ngokevin · 2017-04-21T02:36:00Z

src/components/raycaster.js

-    var objectEls;
+    var len;
+    var objects;
+    var objectsAreEls = data.objects ? this.el.sceneEl.querySelectorAll(data.objects) : null;


Why the logic change here? Previously, we had:

if (data.objects) { // Run query selector. } else { // Intersect with everything. }

see @donmccurdy recommendations above -- basically, this.objects should have only object3D when done, so the first part is exactly that, and the second part grabs the child object3D values because the top-level group one is useless.

nit: objectsAreEls sounds like a boolean to me. Maybe call it objectEls?

ngokevin · 2017-04-21T02:37:23Z

src/components/raycaster.js

-      for (i = 0; i < objectEls.length; i++) {
-        this.objects.push(objectEls[i].object3D);
+    if (objectsAreEls) {
+      for (objects = [], i = 0, len = objectsAreEls.length; i < len; i++) {


The objects = [] assignment in the loop is a bit hard to follow. We also don't need len variable for a for loop for (i = 0; i < objectEls.length; i++).

len came from @donmccurdy request - recanted after already done - be easier if y'all would agree with each other first ;-)

I see, sounds fine. We can move the assignment out though

Haha yeah sorry. My worry about objectsAreEls.length performance turned out to be unnecessary. This loop can just use normal syntax. :)

ngokevin · 2017-04-21T02:39:09Z

src/components/raycaster.js

    }

-    // If objects not defined, intersect with everything.
-    this.objects = this.el.sceneEl.object3D.children;
+    for (this.objects = [], i = 0, len = objects.length; i < len; i++) {


Isn't this only necessary for non-recursive raycasting?

see @donmccurdy recommendations above -- basically, this.objects should have only object3D when done, so we always do this second step now

Right, my preference would be to give objects a consistent type Array<Object3D> as early as possible, rather than checking type in later loops.

And yes — this entire loop is only necessary for non-recursive raycasting. Maybe saves the raycaster some very trivial amount of work, too.

ngokevin · 2017-04-21T02:41:22Z

tests/components/raycaster.test.js

      sceneEl.appendChild(newEl);
+
+      function eventuallyDoAssert () {
+        if (newEl.hasLoaded) { doAssert(); } else { newEl.addEventListener('loaded', doAssert); }


Here as well:

if (clause) { } else { }

ngokevin · 2017-04-21T02:41:48Z

tests/components/raycaster.test.js

      targetEl.setAttribute('position', '0 0 -1');
-      targetEl.addEventListener('loaded', function finishSetup () {
+      el.sceneEl.appendChild(targetEl);
+      // `npm run test:forefox` needs the timeout for the tests to succeed.


Probably not worth mentioning, appendChild is asynchronous so it's a good idea to have always.

ngokevin · 2017-04-21T02:42:34Z

examples/test/raycaster/simple.html

+    </a-scene>
+
+    <script>
+      var el = document.createElement('a-entity');


Within an ad-hoc component would be cool to contain the logic

ngokevin · 2017-04-21T02:43:00Z

examples/test/raycaster/simple.html

+      el.setAttribute('position', '0 0 -2');
+      document.querySelector('a-scene').appendChild(el);
+
+      document.querySelector('a-entity[geometry]').addEventListener('raycaster-intersected', function (evt) {


query selector needed? this is el, right?

ngokevin · 2017-04-21T02:43:08Z

examples/test/raycaster/simple.html

+      document.querySelector('a-scene').appendChild(el);
+
+      document.querySelector('a-entity[geometry]').addEventListener('raycaster-intersected', function (evt) {
+	// getting two intersection events per tick; same element, different faces...


What does this comment mean? A bug, or what we expect?

I think the behavior is permitted.

donmccurdy · 2017-04-21T04:22:30Z

If we choose to keep the recursive:false option around, then I'm happy with this PR after the remaining comments.

But I don't have a strong opinion about whether that option should exist, one way or another.

machenmusik · 2017-05-16T12:59:14Z

@dmarcos @ngokevin - @donmccurdy and I think this one is done, did we want to merge or close?

ngokevin · 2017-05-16T19:17:31Z

Thanks!

dmarcos added the needs changes label Jan 30, 2017

usernamealreadyis suggested changes Jan 31, 2017

View reviewed changes

ngokevin added this to the 0.5.0 milestone Feb 1, 2017

ngokevin added needs review and removed needs changes labels Feb 1, 2017

ngokevin removed this from the 0.5.0 milestone Feb 7, 2017

machenmusik force-pushed the fix-recursive-raycaster branch 2 times, most recently from 5d719c1 to 7aac032 Compare February 7, 2017 23:30

usernamealreadyis suggested changes Feb 8, 2017

View reviewed changes

machenmusik force-pushed the fix-recursive-raycaster branch from 7aac032 to ae0e455 Compare February 23, 2017 23:52

ngokevin added this to the 0.6.0 milestone Mar 30, 2017

donmccurdy requested changes Apr 19, 2017

View reviewed changes

machenmusik added 2 commits April 19, 2017 10:53

change raycaster to be non-recursive

7ad479c

since there appears to be a wrapper around an entity's object3D, use its children instead

add simple example of non-recursive raycaster and dynamically created…

b2755b1

… box add geometry so that raycaster actually adds something wait until next tick to satisfy Firefox; remove extra tick

machenmusik force-pushed the fix-recursive-raycaster branch from ae0e455 to b2755b1 Compare April 19, 2017 14:55

changes per discussion on PR

53a5547

ngokevin changed the title ~~Fix recursive raycaster~~ enable non-recursive raycaster (fixes #2329) Apr 21, 2017

ngokevin requested changes Apr 21, 2017

View reviewed changes

changes per discussion on PR

1799370

ngokevin merged commit 59b3cf0 into aframevr:master May 16, 2017

enable non-recursive raycaster (fixes #2329) #2331

enable non-recursive raycaster (fixes #2329) #2331

Conversation

machenmusik commented Jan 30, 2017

dmarcos commented Jan 30, 2017

machenmusik commented Jan 30, 2017 • edited

machenmusik commented Jan 30, 2017

machenmusik commented Jan 30, 2017

ngokevin commented Jan 30, 2017

codecov-io commented Jan 31, 2017 • edited

Codecov Report

usernamealreadyis left a comment

Choose a reason for hiding this comment

machenmusik commented Jan 31, 2017 • edited

usernamealreadyis left a comment

Choose a reason for hiding this comment

maissani commented Feb 16, 2017

machenmusik commented Feb 16, 2017

machenmusik commented Feb 24, 2017

ngokevin commented Feb 25, 2017

machenmusik commented Feb 25, 2017

cvan commented Apr 18, 2017

machenmusik commented Apr 18, 2017

donmccurdy commented Apr 19, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donmccurdy Apr 19, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

machenmusik commented Apr 19, 2017

machenmusik commented Apr 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donmccurdy commented Apr 21, 2017

machenmusik commented May 16, 2017

ngokevin commented May 16, 2017

machenmusik commented Jan 30, 2017 •

edited

codecov-io commented Jan 31, 2017 •

edited

machenmusik commented Jan 31, 2017 •

edited

donmccurdy commented Apr 19, 2017 •

edited

donmccurdy Apr 19, 2017 •

edited