Skip to content

Commit

Permalink
Several fixes to make GLES2 on HTML5 work much better.
Browse files Browse the repository at this point in the history
Changed math class error reporting to be a bit less paranoid.
  • Loading branch information
reduz committed Feb 26, 2019
1 parent 51c1d55 commit a32b26d
Show file tree
Hide file tree
Showing 16 changed files with 314 additions and 99 deletions.
4 changes: 4 additions & 0 deletions SConstruct
Expand Up @@ -141,6 +141,7 @@ opts.Add(EnumVariable('target', "Compilation target", 'debug', ('debug', 'releas
opts.Add(EnumVariable('optimize', "Optimization type", 'speed', ('speed', 'size')))
opts.Add(BoolVariable('tools', "Build the tools (a.k.a. the Godot editor)", True))
opts.Add(BoolVariable('use_lto', 'Use link-time optimization', False))
opts.Add(BoolVariable('use_precise_math_checks', 'Math checks use very precise epsilon (useful to debug the engine)', False))

# Components
opts.Add(BoolVariable('deprecated', "Enable deprecated features", True))
Expand Down Expand Up @@ -224,6 +225,9 @@ env_base.Append(CPPPATH=['#editor', '#'])
env_base.platform_exporters = platform_exporters
env_base.platform_apis = platform_apis

if (env_base["use_precise_math_checks"]):
env_base.Append(CPPDEFINES=['PRECISE_MATH_CHECKS'])

if (env_base['target'] == 'debug'):
env_base.Append(CPPDEFINES=['DEBUG_MEMORY_ALLOC','DISABLE_FORCED_INLINE'])

Expand Down
74 changes: 46 additions & 28 deletions core/math/basis.cpp
Expand Up @@ -76,15 +76,23 @@ void Basis::invert() {
}

void Basis::orthonormalize() {
/* this check is undesired, the matrix could be wrong but we still may want to generate a valid one
* for practical purposes
#ifdef MATH_CHECKS
ERR_FAIL_COND(determinant() == 0);
#endif
*/
// Gram-Schmidt Process

Vector3 x = get_axis(0);
Vector3 y = get_axis(1);
Vector3 z = get_axis(2);

#ifdef MATH_CHECKS
ERR_FAIL_COND(x.length_squared() == 0);
ERR_FAIL_COND(y.length_squared() == 0);
ERR_FAIL_COND(z.length_squared() == 0);
#endif
x.normalize();
y = (y - x * (x.dot(y)));
y.normalize();
Expand Down Expand Up @@ -118,16 +126,16 @@ bool Basis::is_diagonal() const {
}

bool Basis::is_rotation() const {
return Math::is_equal_approx(determinant(), 1) && is_orthogonal();
return Math::is_equal_approx(determinant(), 1, UNIT_EPSILON) && is_orthogonal();
}

bool Basis::is_symmetric() const {

if (!Math::is_equal_approx(elements[0][1], elements[1][0]))
if (!Math::is_equal_approx_ratio(elements[0][1], elements[1][0], UNIT_EPSILON))
return false;
if (!Math::is_equal_approx(elements[0][2], elements[2][0]))
if (!Math::is_equal_approx_ratio(elements[0][2], elements[2][0], UNIT_EPSILON))
return false;
if (!Math::is_equal_approx(elements[1][2], elements[2][1]))
if (!Math::is_equal_approx_ratio(elements[1][2], elements[2][1], UNIT_EPSILON))
return false;

return true;
Expand Down Expand Up @@ -488,6 +496,11 @@ void Basis::set_euler_xyz(const Vector3 &p_euler) {
// as the x, y, and z components of a Vector3 respectively.
Vector3 Basis::get_euler_yxz() const {

/* checking this is a bad idea, because obtaining from scaled transform is a valid use case
#ifdef MATH_CHECKS
ERR_FAIL_COND(!is_rotation());
#endif
*/
// Euler angles in YXZ convention.
// See https://en.wikipedia.org/wiki/Euler_angles#Rotation_matrix
//
Expand All @@ -496,9 +509,7 @@ Vector3 Basis::get_euler_yxz() const {
// cy*sx*sz-cz*sy cy*cz*sx+sy*sz cy*cx

Vector3 euler;
#ifdef MATH_CHECKS
ERR_FAIL_COND_V(!is_rotation(), euler);
#endif

real_t m12 = elements[1][2];

if (m12 < 1) {
Expand Down Expand Up @@ -556,7 +567,7 @@ bool Basis::is_equal_approx(const Basis &a, const Basis &b) const {

for (int i = 0; i < 3; i++) {
for (int j = 0; j < 3; j++) {
if (!Math::is_equal_approx(a.elements[i][j], b.elements[i][j]))
if (!Math::is_equal_approx_ratio(a.elements[i][j], b.elements[i][j], UNIT_EPSILON))
return false;
}
}
Expand Down Expand Up @@ -599,34 +610,38 @@ Basis::operator String() const {
}

Quat Basis::get_quat() const {
#ifdef MATH_CHECKS
ERR_FAIL_COND_V(!is_rotation(), Quat());
#endif
real_t trace = elements[0][0] + elements[1][1] + elements[2][2];

/* Allow getting a quaternion from an unnormalized transform */

This comment has been minimized.

Copy link
@tagcup

tagcup Feb 26, 2019

Contributor

@reduz The functionality you want was already implemented in rotation and scale decomposition functions: get_rotation_axis_angle and get_rotation_quat. get_quat don't normalize because they're not decomposition functions.

You can read my comment for get_scale in basis.cpp for an explanation why decomposition functions exist. Basically, one needs to choose a branch cut, and it has to be enforced consistently across get_rotation* and get_scale* decomposition functions.

The "valid use case" you are referring to has already been implemented in those functions. get_quat, get_euler_yxz, get_axis_angle serve a different purpose, they don't do decomposition, they convert a rotation from its matrix representation to quaternion representation.

Your modifications in this commit don't implement a consistent branch cut, and break the decomposition of matrices with negative determinant; that is: the combination of get_rotation() and get_scale() no longer give the original transform after this commit.

Bottom line: those checks weren't a bad idea, and they are necessary for the purpose they serve. The use case you are referring to requires a particular branch cut which needs to be implemented consistently, and this was already there. The changes in this commit don't implement a consistent branch cut and break the existing one.

This comment has been minimized.

Copy link
@reduz

reduz Mar 6, 2019

Author Member

Makes sense, I didn't see the other functions, so I will add a check suggesting the use of the other function instead.

This comment has been minimized.

Copy link
@reduz

reduz Apr 1, 2019

Author Member

Fixed in dee98d3

Basis m = *this;
m.elements[0].normalize();
m.elements[1].normalize();
m.elements[2].normalize();

real_t trace = m.elements[0][0] + m.elements[1][1] + m.elements[2][2];
real_t temp[4];

if (trace > 0.0) {
real_t s = Math::sqrt(trace + 1.0);
temp[3] = (s * 0.5);
s = 0.5 / s;

temp[0] = ((elements[2][1] - elements[1][2]) * s);
temp[1] = ((elements[0][2] - elements[2][0]) * s);
temp[2] = ((elements[1][0] - elements[0][1]) * s);
temp[0] = ((m.elements[2][1] - m.elements[1][2]) * s);
temp[1] = ((m.elements[0][2] - m.elements[2][0]) * s);
temp[2] = ((m.elements[1][0] - m.elements[0][1]) * s);
} else {
int i = elements[0][0] < elements[1][1] ?
(elements[1][1] < elements[2][2] ? 2 : 1) :
(elements[0][0] < elements[2][2] ? 2 : 0);
int i = m.elements[0][0] < m.elements[1][1] ?
(m.elements[1][1] < m.elements[2][2] ? 2 : 1) :
(m.elements[0][0] < m.elements[2][2] ? 2 : 0);
int j = (i + 1) % 3;
int k = (i + 2) % 3;

real_t s = Math::sqrt(elements[i][i] - elements[j][j] - elements[k][k] + 1.0);
real_t s = Math::sqrt(m.elements[i][i] - m.elements[j][j] - m.elements[k][k] + 1.0);
temp[i] = s * 0.5;
s = 0.5 / s;

temp[3] = (elements[k][j] - elements[j][k]) * s;
temp[j] = (elements[j][i] + elements[i][j]) * s;
temp[k] = (elements[k][i] + elements[i][k]) * s;
temp[3] = (m.elements[k][j] - m.elements[j][k]) * s;
temp[j] = (m.elements[j][i] + m.elements[i][j]) * s;
temp[k] = (m.elements[k][i] + m.elements[i][k]) * s;
}

return Quat(temp[0], temp[1], temp[2], temp[3]);
Expand Down Expand Up @@ -696,9 +711,11 @@ void Basis::set_orthogonal_index(int p_index) {
}

void Basis::get_axis_angle(Vector3 &r_axis, real_t &r_angle) const {
/* checking this is a bad idea, because obtaining from scaled transform is a valid use case
#ifdef MATH_CHECKS
ERR_FAIL_COND(!is_rotation());
#endif
*/
real_t angle, x, y, z; // variables for result
real_t epsilon = 0.01; // margin to allow for rounding errors
real_t epsilon2 = 0.1; // margin to distinguish between 0 and 180 degrees
Expand Down Expand Up @@ -835,14 +852,15 @@ void Basis::set_diagonal(const Vector3 p_diag) {
}

Basis Basis::slerp(const Basis &target, const real_t &t) const {
// TODO: implement this directly without using quaternions to make it more efficient
#ifdef MATH_CHECKS
ERR_FAIL_COND_V(!is_rotation(), Basis());
ERR_FAIL_COND_V(!target.is_rotation(), Basis());
#endif

//consider scale
Quat from(*this);
Quat to(target);

return Basis(from.slerp(to, t));
Basis b(from.slerp(to, t));
b.elements[0] *= Math::lerp(elements[0].length(), target.elements[0].length(), t);
b.elements[1] *= Math::lerp(elements[1].length(), target.elements[1].length(), t);
b.elements[2] *= Math::lerp(elements[2].length(), target.elements[2].length(), t);

return b;
}
9 changes: 9 additions & 0 deletions core/math/math_defs.h
Expand Up @@ -33,6 +33,7 @@

#define CMP_EPSILON 0.00001
#define CMP_EPSILON2 (CMP_EPSILON * CMP_EPSILON)

#define CMP_NORMALIZE_TOLERANCE 0.000001
#define CMP_POINT_IN_PLANE_EPSILON 0.00001

Expand All @@ -49,6 +50,14 @@
#define MATH_CHECKS
#endif

//this epsilon is for values related to a unit size (scalar or vector len)
#ifdef PRECISE_MATH_CHECKS
#define UNIT_EPSILON 0.00001
#else
//tolerate some more floating point error normally
#define UNIT_EPSILON 0.001
#endif

#define USEC_TO_SEC(m_usec) ((m_usec) / 1000000.0)

enum ClockDirection {
Expand Down
16 changes: 14 additions & 2 deletions core/math/math_funcs.h
Expand Up @@ -249,13 +249,25 @@ class Math {
static float random(float from, float to);
static real_t random(int from, int to) { return (real_t)random((real_t)from, (real_t)to); }

static _ALWAYS_INLINE_ bool is_equal_approx(real_t a, real_t b) {
static _ALWAYS_INLINE_ bool is_equal_approx_ratio(real_t a, real_t b, real_t epsilon = CMP_EPSILON) {
// this is an approximate way to check that numbers are close, as a ratio of their average size
// helps compare approximate numbers that may be very big or very small
real_t diff = abs(a - b);
if (diff == 0.0) {
return true;
}
real_t avg_size = (abs(a) + abs(b)) / 2.0;
diff /= avg_size;
return diff < epsilon;
}

static _ALWAYS_INLINE_ bool is_equal_approx(real_t a, real_t b, real_t epsilon = CMP_EPSILON) {

This comment has been minimized.

Copy link
@aaronfranke

aaronfranke Mar 8, 2019

Member

I thought we were waiting until after 3.1 is released to make these kinds of changes? I've had a similar pull request open for months: #18992

From looking at it, it seems that is_equal_approx_ratio and my changes to is_equal_approx accomplish almost the same thing. You're scaling the left-hand side, I'm scaling the tolerance. But your code handles differences between ultra-small values, mine treats ultra-small values as equal, we could leave both to allow different handling of ultra-small values. I'd like to get feedback on this so that I can update my PR. (Note: My PR also does several other things)

This comment has been minimized.

Copy link
@aaronfranke

aaronfranke Apr 1, 2019

Member

Hmm, I suppose I have to ping @reduz to notify him.

// TODO: Comparing floats for approximate-equality is non-trivial.
// Using epsilon should cover the typical cases in Godot (where a == b is used to compare two reals), such as matrix and vector comparison operators.
// A proper implementation in terms of ULPs should eventually replace the contents of this function.
// See https://randomascii.wordpress.com/2012/02/25/comparing-floating-point-numbers-2012-edition/ for details.

return abs(a - b) < CMP_EPSILON;
return abs(a - b) < epsilon;
}

static _ALWAYS_INLINE_ float absf(float g) {
Expand Down
2 changes: 1 addition & 1 deletion core/math/quat.cpp
Expand Up @@ -135,7 +135,7 @@ Quat Quat::normalized() const {
}

bool Quat::is_normalized() const {
return Math::is_equal_approx(length_squared(), 1.0);
return Math::is_equal_approx(length_squared(), 1.0, UNIT_EPSILON); //use less epsilon
}

Quat Quat::inverse() const {
Expand Down
2 changes: 1 addition & 1 deletion core/math/vector2.cpp
Expand Up @@ -65,7 +65,7 @@ Vector2 Vector2::normalized() const {

bool Vector2::is_normalized() const {
// use length_squared() instead of length() to avoid sqrt(), makes it more stringent.
return Math::is_equal_approx(length_squared(), 1.0);
return Math::is_equal_approx(length_squared(), 1.0, UNIT_EPSILON);
}

real_t Vector2::distance_to(const Vector2 &p_vector2) const {
Expand Down
2 changes: 1 addition & 1 deletion core/math/vector3.h
Expand Up @@ -414,7 +414,7 @@ Vector3 Vector3::normalized() const {

bool Vector3::is_normalized() const {
// use length_squared() instead of length() to avoid sqrt(), makes it more stringent.
return Math::is_equal_approx(length_squared(), 1.0);
return Math::is_equal_approx(length_squared(), 1.0, UNIT_EPSILON);
}

Vector3 Vector3::inverse() const {
Expand Down
1 change: 1 addition & 0 deletions drivers/gles2/rasterizer_canvas_gles2.cpp
Expand Up @@ -2019,6 +2019,7 @@ void RasterizerCanvasGLES2::initialize() {
state.canvas_shader.init();

state.canvas_shader.set_conditional(CanvasShaderGLES2::USE_TEXTURE_RECT, true);
state.canvas_shader.set_conditional(CanvasShaderGLES2::USE_RGBA_SHADOWS, storage->config.use_rgba_2d_shadows);

state.canvas_shader.bind();

Expand Down

0 comments on commit a32b26d

Please sign in to comment.