Frustum improvements + unit tests #2865

Merged
merged 13 commits into from Jan 2, 2013

Conversation

6 participants
Contributor

bhouston commented Dec 31, 2012

This PR contains the following:

  • Significant polishing of Frustum.js so that it has the characteristics of the other members of the math library (proper constructor, set, copy, clone.)
  • Additional Frustum.js functionality including containsSphere, containsPoint, containsAnyPoint and transform (could be renamed applyMatrix4.)
  • Frustum unit tests. (We now have over 1100 unit tests!)
  • Added Plane.flip() method and a unit test.

Also I updated some other unit tests to use applyMatrix4 in a few cases so that they work again and removed some unused temporary variables from Raycaster.

Contributor

gero3 commented Dec 31, 2012

Did you do a performance review??? I'm not sure if the performance of the frustum isn't slowed down now becuase of the multiple function calls in the contains method. The following example is the most stressing for the frustum.

https://github.com/mrdoob/three.js/blob/master/examples/webgl_performance.html

Contributor

bhouston commented Dec 31, 2012

I get identical performance before and after in Chrome at ~66fps (which varies up or down a few fps depending on the scene because the scene is generated randomly.) In Firefox ~53fps in both cases.

Owner

mrdoob commented Dec 31, 2012

Sweet stuff as usual.

Wondering about consistency though...

We have vector.negate() and I've always wondered whether it should be renamed to vector.invert() instead. Now you're adding plane.flip().

What's the common name for this?

Contributor

bhouston commented Dec 31, 2012

@mrdoob thanks!

Inverse/invert can often mean 1/A instead of -A (e.g. Matrix3.getInverse), so I suggest keeping negate to mean -A and using inverse to mean 1/A. I am okay with renaming Plane.flip to Plane.negate(). :-)

Contributor

alteredq commented Dec 31, 2012

Did you do a performance review??? I'm not sure if the performance of the frustum isn't slowed down now becuase of the multiple function calls in the contains method. The following example is the most stressing for the frustum.

I get identical performance before and after in Chrome at ~66fps (which varies up or down a few fps depending on the scene because the scene is generated randomly.) In Firefox ~53fps in both cases.

I think these changes should be tested separately in a standalone test.

While webgl_performance.html is the most stressing frustum test right now (actually webgl_performance_static.html is probably a better test), still frustum checks are relatively small compared to other stuff going on there.

So I wouldn't expect much differences in overall framerate even if new variant was quite much worse (e.g. 50% slowdown of something taking 5% would just make it take 7.5% of the time and here I would expect less than 50% slowdown).

Contributor

bhouston commented Dec 31, 2012

@alteredq @gero3 per your recommendations, I have re-expanded/inlined Frustum.contains( object ) to ensure no loss of performance and added a note to leave it expanded.

Uhm, this credit seems to have been misplaced. I guess it should be moved to Triangle now?
https://github.com/mrdoob/three.js/blob/master/src/math/Triangle.js#L42

Owner

bhouston replied Dec 31, 2012

Fixed here: 5876726

Collaborator

WestLangley commented Dec 31, 2012

I have some questions/comments about Frustum.js as proposed.

  1. Frustum.containsPoint( point ) returns true if the point is in the frustum. Frustum.containsSphere( sphere ) is checking for intersection, not containment.
  2. Frustum.containsObject( object ) is a very conservative intersection test and should be renamed, IMHO
  3. What is the use case for Frustum.containsAnyPoints( points )?
  4. What is the use case for Frustum.transform( matrix )?
  5. THREE.Frustum.__s0 = new THREE.Sphere(); is not used
Contributor

bhouston commented Dec 31, 2012

@WestLangley:

  1. containsSphere's name is based on the fact that it is equivalent to containsObject (which is actually called contains) and containsObject just checks if only part of that object is contained. I am of course open to any new names you can suggest that make things clearer.
  2. contains (which probably should be containsObject) could be renamed that it is using a bounding sphere, so containsObjectBoundingSphere, although I must say that I would actually prefer to not include scene graph-dependent functions within the math library component of Three.js -- I prefer to have a hierarchical modularity where the scene graph is dependent upon the math library but the math library isn't dependent upon the scene graph.
  3. Frustum.containsAnyPoints, a finer grain test if a point cloud (although it is slightly faulty for a triangle/quad-based mesh) is contained within a Frustum. But fairly slow.
  4. Transforming Frustums into object space which can be a lot faster than transforming all elements of an object into the global space. For example, see how Raycaster transforms the Ray into local object space when doing its comparisons: https://github.com/bhouston/three.js/blob/dev/src/core/Raycaster.js#L90 I am moving incrementally towards addressing this issue: #2858
  5. For sure, I can remove it. :-)
Collaborator

WestLangley commented Dec 31, 2012

@bhouston

  1. Frustum.intersectsSphere( sphere ) is a descriptive name.
  2. Frustum.intersects( object ) or Frustum.intersectsObject( object ) ?
  3. I do not see a compelling case Frustum.containsAnyPoints( points ).
  4. I do not see a compelling case Frustum.transform( matrix ) until it is needed, unless you are claiming you will need it shortly.
Contributor

bhouston commented Dec 31, 2012

@WestLangley:

1 & 2. I've made that change in my next commit.

3 & 4. So I should remove these two functions until we commit a Frustumcaster object ( #2858 )? I guess that makes sense as it is up in the air whether Frustumcaster will be accepted or not.

@bhouston bhouston Frustum.contains -> intersectsObject, containsSphere->intersectsSpher…
…e, removed Frustum.containsAnyPoints, .transform. Update unit tests accordingly.
6f72a00
Collaborator

WestLangley commented Dec 31, 2012

@bhouston

What gets included is up to @mrdoob.

In my opinion, (3) is not needed at all, and neither is (4), until it can be demonstrated that it is.

As far as the function name changes are concerned, I would make a post so it can be discussed, as @mrdoob often does, before including the change in a PR -- but then again, I tend to be rather cautious. :-)

Owner

mrdoob commented Jan 1, 2013

I think the name changes are good. I can see the use of .containsPoint but yes, .containsPoints may be a bit too much.

I'm also unsure about .transform. Isn't transforming the frustum more expensive than transforming the boundingSphere?

Btw, happy 2013 guys! /cc @alteredq, @gero3, @zz85, ... ^^

Contributor

bhouston commented Jan 1, 2013

Happy new year! To even more ThreeJS success in 2013!
On Dec 31, 2012 7:44 PM, "Mr.doob" notifications@github.com wrote:

I think the name changes are good. I can see the use of .containsPointbut yes,
.containsPoints may be a bit too much.

I'm also unsure about .transform. Isn't transforming the frustum more
expensive than transforming the boundingSphere?

Btw, happy 2013 guys! /cc @alteredq https://github.com/alteredq, @gero3https://github.com/gero3,
@zz85 https://github.com/zz85, ... ^^


Reply to this email directly or view it on GitHubhttps://github.com/mrdoob/three.js/pull/2865#issuecomment-11786303.

Contributor

zz85 commented Jan 2, 2013

happy new year too! its been crazy busy for me outside three.js this period, but nice to know there's so much things going on here too =)

Btw, happy 2013 guys! /cc @alteredq, @gero3, @zz85, ... ^^

Contributor

gero3 commented Jan 2, 2013

Happy new year to y'all.

Btw, happy 2013 guys! /cc @alteredq, @gero3, @zz85, ... ^^

Noob question...
Is this.constant *= -1 better than this.constant = -this.constant?

Owner

bhouston replied Jan 3, 2013

I am not sure. I guess I probably should have used this.constant = -this.constant as it could possibly be a bit faster.

mrdoob merged commit 6f72a00 into mrdoob:dev Jan 2, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment