sigonasr2/jmonkeyengine
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Removing the dodgy 'optimization' that broke some people and caused other 'makeup' changes to better support the dodginess.

Browse Source 
(And I do realize I have the benefit of analyzing the aftermath, hindsight is 20/20, etc.)  Included a big long comment about the right way to implement this optimization.
This commit is contained in:
Paul Speed 2015-11-16 03:03:53 -05:00
parent 3a4624a5fe
commit 31cab674b3
1 changed files with 30 additions and 0 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

30
jme3-core/src/main/java/com/jme3/scene/Node.java
View File

@ -570,6 +570,35 @@ public class Node extends Spatial {
// optimization: try collideWith BoundingVolume to avoid possibly redundant tests on children // optimization: try collideWith BoundingVolume to avoid possibly redundant tests on children
// number 4 in condition is somewhat arbitrary. When there is only one child, the boundingVolume test is redundant at all. // number 4 in condition is somewhat arbitrary. When there is only one child, the boundingVolume test is redundant at all.
// The idea is when there are few children, it can be too expensive to test boundingVolume first. // The idea is when there are few children, it can be too expensive to test boundingVolume first.
/*
I'm removing this change until some issues can be addressed and I really
think it needs to be implemented a better way anyway.
First, it causes issues for anyone doing collideWith() with BoundingVolumes
and expecting it to trickle down to the children. For example, children
with BoundingSphere bounding volumes and collideWith(BoundingSphere). Doing
a collision check at the parent level then has to do a BoundingSphere to BoundingBox
collision which isn't resolved. (Having to come up with a collision point in that
case is tricky and the first sign that this is the wrong approach.)
Second, the rippling changes this caused to 'optimize' collideWith() for this
special use-case are another sign that this approach was a bit dodgy. The whole
idea of calculating a full collision just to see if the two shapes collide at all
is very wasteful.
A proper implementation should support a simpler boolean check that doesn't do
all of that calculation. For example, if 'other' is also a BoundingVolume (ie: 99.9%
of all non-Ray cases) then a direct BV to BV intersects() test can be done. So much
faster. And if 'other' _is_ a Ray then the BV.intersects(Ray) call can be done.
I don't have time to do it right now but I'll at least un-break a bunch of peoples'
code until it can be 'optimized' properly. Hopefully it's not too late to back out
the other dodgy ripples this caused. -pspeed (hindsight-expert ;))
Note: the code itself is relatively simple to implement but I don't have time to
a) test it, and b) see if '> 4' is still a decent check for it. Could be it's fast
enough to do all the time for > 1.
if (children.size() > 4) if (children.size() > 4)
{ {
BoundingVolume bv = this.getWorldBound(); BoundingVolume bv = this.getWorldBound();
@ -578,6 +607,7 @@ public class Node extends Spatial {
// collideWith without CollisionResults parameter used to avoid allocation when possible // collideWith without CollisionResults parameter used to avoid allocation when possible
if (bv.collideWith(other) == 0) return 0; if (bv.collideWith(other) == 0) return 0;
} }
*/
for (Spatial child : children.getArray()){ for (Spatial child : children.getArray()){
total += child.collideWith(other, results); total += child.collideWith(other, results);
} }