-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Description
While working on getting jme compiled to javascript I've encountered an issue: clicks went through some ui elements (lemur).
After a very long debugging session i've isolated the problem to a floating point precision issue in BoundingBox.clip
that happens when bounding boxes have zExtent=0.
jmonkeyengine/jme3-core/src/main/java/com/jme3/bounding/BoundingBox.java
Lines 1015 to 1073 in 8cc3086
private boolean clip(float denom, float numerator, float[] t) { | |
// Return value is 'true' if line segment intersects the current test | |
// plane. Otherwise, 'false' is returned, in which case the line segment | |
// is entirely clipped. | |
if (denom > 0.0f) { | |
// This is the old if statement... | |
// if (numerator > denom * t[1]) { | |
// | |
// The problem is that what is actually stored is | |
// numerator/denom. In non-floating point, this math should | |
// work out the same but in floating point there can | |
// be subtle math errors. The multiply will exaggerate | |
// errors that may have been introduced when the value | |
// was originally divided. | |
// | |
// This is especially true when the bounding box has zero | |
// extents in some plane because the error rate is critical. | |
// comparing a to b * c is not the same as comparing a/b to c | |
// in this case. In fact, I tried converting this method to | |
// double and the and the error was in the last decimal place. | |
// | |
// So, instead, we now compare the divided version to the divided | |
// version. We lose some slight performance here as divide | |
// will be more expensive than the divide. Some microbenchmarks | |
// show divide to be 3x slower than multiple on Java 1.6. | |
// BUT... we also saved a multiply in the non-clipped case because | |
// we can reuse the divided version in both if checks. | |
// I think it's better to be right in this case. | |
// | |
// Bug that I'm fixing: rays going right through quads at certain | |
// angles and distances because they fail the bounding box test. | |
// Many Bothans died bring you this fix. | |
// -pspeed | |
float newT = numerator / denom; | |
if (newT > t[1]) { | |
return false; | |
} | |
if (newT > t[0]) { | |
t[0] = newT; | |
} | |
return true; | |
} else if (denom < 0.0f) { | |
// Old if statement... see above | |
// if (numerator > denom * t[0]) { | |
// | |
// Note though that denom is always negative in this block. | |
// When we move it over to the other side we have to flip | |
// the comparison. Algebra for the win. | |
float newT = numerator / denom; | |
if (newT < t[0]) { | |
return false; | |
} | |
if (newT < t[1]) { | |
t[1] = newT; | |
} | |
return true; | |
} else { | |
return numerator <= 0.0; | |
} |
As you can see, in this code there is a note by @pspeed42 regarding a similar issue that he fixed in the past.
My assumption is that, the way floating point numbers are represented in javascript (doubles?) caused this issue to reappear when the engine was compiled with teavm. I've ended up fixing it in this way.
I’m opening this issue to make it searchable in the future, in case this corner case ever arises during normal use of jME.