From 60c55896718c9ed596632b06b0fb303d67667db2 Mon Sep 17 00:00:00 2001 From: rdb Date: Tue, 16 Nov 2021 14:04:54 +0100 Subject: [PATCH] pgraph: Optimize handling/checking of identity/invalid transforms --- panda/src/pgraph/transformState.I | 29 +++- panda/src/pgraph/transformState.cxx | 241 ++++++++++++++-------------- panda/src/pgraph/transformState.h | 10 +- tests/pgraph/test_transforms.py | 68 ++++++++ 4 files changed, 221 insertions(+), 127 deletions(-) create mode 100644 tests/pgraph/test_transforms.py diff --git a/panda/src/pgraph/transformState.I b/panda/src/pgraph/transformState.I index 908575ea73..6876d4c2a0 100644 --- a/panda/src/pgraph/transformState.I +++ b/panda/src/pgraph/transformState.I @@ -40,6 +40,31 @@ get_hash() const { return _hash; } +/** + * Constructs an identity transform. + */ +INLINE CPT(TransformState) TransformState:: +make_identity() { + if (UNLIKELY(_states_lock == nullptr)) { + init_states(); + } + + return _identity_state; +} + +/** + * Constructs an invalid transform; for instance, the result of inverting a + * singular matrix. + */ +INLINE CPT(TransformState) TransformState:: +make_invalid() { + if (UNLIKELY(_states_lock == nullptr)) { + init_states(); + } + + return _invalid_state; +} + /** * Makes a new TransformState with the specified components. */ @@ -204,7 +229,7 @@ make_pos_rotate_scale2d(const LVecBase2 &pos, PN_stdfloat rotate, */ INLINE bool TransformState:: is_identity() const { - return ((_flags & F_is_identity) != 0); + return this == _identity_state; } /** @@ -214,7 +239,7 @@ is_identity() const { */ INLINE bool TransformState:: is_invalid() const { - return ((_flags & F_is_invalid) != 0); + return this == _invalid_state; } /** diff --git a/panda/src/pgraph/transformState.cxx b/panda/src/pgraph/transformState.cxx index aac37c306b..c80b88e677 100644 --- a/panda/src/pgraph/transformState.cxx +++ b/panda/src/pgraph/transformState.cxx @@ -51,18 +51,17 @@ CacheStats TransformState::_cache_stats; TypeHandle TransformState::_type_handle; /** - * Actually, this could be a private constructor, since no one inherits from - * TransformState, but gcc gives us a spurious warning if all constructors are - * private. + * */ TransformState:: -TransformState() : _lock("TransformState") { +TransformState() : + _flags(F_is_identity | F_singular_known | F_is_2d), + _lock("TransformState") { + if (_states_lock == nullptr) { init_states(); } - _saved_entry = -1; - _flags = F_is_identity | F_singular_known | F_is_2d; - _inv_mat = nullptr; + _cache_stats.add_num_states(1); #ifdef DO_MEMORY_USAGE @@ -232,43 +231,13 @@ operator == (const TransformState &other) const { } } -/** - * Constructs an identity transform. - */ -CPT(TransformState) TransformState:: -make_identity() { - // The identity state is asked for so often, we make it a special case and - // store a pointer forever once we find it the first time. - if (_identity_state == nullptr) { - TransformState *state = new TransformState; - _identity_state = return_unique(state); - } - - return _identity_state; -} - -/** - * Constructs an invalid transform; for instance, the result of inverting a - * singular matrix. - */ -CPT(TransformState) TransformState:: -make_invalid() { - if (_invalid_state == nullptr) { - TransformState *state = new TransformState; - state->_flags = F_is_invalid | F_singular_known | F_is_singular | F_components_known | F_mat_known; - _invalid_state = return_unique(state); - } - - return _invalid_state; -} - /** * Makes a new TransformState with the specified components. */ CPT(TransformState) TransformState:: make_pos_hpr_scale_shear(const LVecBase3 &pos, const LVecBase3 &hpr, const LVecBase3 &scale, const LVecBase3 &shear) { - nassertr(!(pos.is_nan() || hpr.is_nan() || scale.is_nan() || shear.is_nan()) , make_invalid()); + nassertr(!(pos.is_nan() || hpr.is_nan() || scale.is_nan() || shear.is_nan()), make_invalid()); // Make a special-case check for the identity transform. if (pos == LVecBase3(0.0f, 0.0f, 0.0f) && hpr == LVecBase3(0.0f, 0.0f, 0.0f) && @@ -293,7 +262,7 @@ make_pos_hpr_scale_shear(const LVecBase3 &pos, const LVecBase3 &hpr, CPT(TransformState) TransformState:: make_pos_quat_scale_shear(const LVecBase3 &pos, const LQuaternion &quat, const LVecBase3 &scale, const LVecBase3 &shear) { - nassertr(!(pos.is_nan() || quat.is_nan() || scale.is_nan() || shear.is_nan()) , make_invalid()); + nassertr(!(pos.is_nan() || quat.is_nan() || scale.is_nan() || shear.is_nan()), make_invalid()); // Make a special-case check for the identity transform. if (pos == LVecBase3(0.0f, 0.0f, 0.0f) && quat == LQuaternion::ident_quat() && @@ -336,7 +305,7 @@ CPT(TransformState) TransformState:: make_pos_rotate_scale_shear2d(const LVecBase2 &pos, PN_stdfloat rotate, const LVecBase2 &scale, PN_stdfloat shear) { - nassertr(!(pos.is_nan() || cnan(rotate) || scale.is_nan() || cnan(shear)) , make_invalid()); + nassertr(!(pos.is_nan() || cnan(rotate) || scale.is_nan() || cnan(shear)), make_invalid()); // Make a special-case check for the identity transform. if (pos == LVecBase2(0.0f, 0.0f) && rotate == 0.0f && @@ -700,7 +669,7 @@ invert_compose(const TransformState *other) const { if (other == this) { // a->invert_compose(a) always produces identity. - return make_identity(); + return _identity_state; } if (!transform_cache) { @@ -1421,6 +1390,40 @@ init_states() { _states_lock = new LightReMutex("TransformState::_states_lock"); _cache_stats.init(); nassertv(Thread::get_current_thread() == Thread::get_main_thread()); + + // The identity and invalid states are asked for so often, we make them a + // special case and store a pointer forever. + { + TransformState *state = new TransformState; + state->_pos.set(0.0f, 0.0f, 0.0f); + state->_scale.set(1.0f, 1.0f, 1.0f); + state->_shear.set(0.0f, 0.0f, 0.0f); + state->_hpr.set(0.0f, 0.0f, 0.0f); + state->_quat.set(1.0f, 0.0f, 0.0f, 0.0f); + state->_norm_quat.set(1.0f, 0.0f, 0.0f, 0.0f); + state->_mat.set(1.0f, 0.0f, 0.0f, 0.0f, + 0.0f, 1.0f, 0.0f, 0.0f, + 0.0f, 0.0f, 1.0f, 0.0f, + 0.0f, 0.0f, 0.0f, 1.0f); + state->_inv_mat = new LMatrix4(state->_mat); + state->_hash = int_hash::add_hash(0, F_is_invalid | F_is_identity | F_is_2d); + state->_flags = F_is_identity | F_singular_known | F_components_known + | F_has_components | F_mat_known | F_quat_known | F_hpr_known + | F_uniform_scale | F_identity_scale | F_is_2d | F_hash_known + | F_norm_quat_known; + state->cache_ref(); + state->_saved_entry = _states.store(state, nullptr); + _identity_state = state; + } + { + TransformState *state = new TransformState; + state->_hash = int_hash::add_hash(0, F_is_invalid); + state->_flags = F_is_singular | F_singular_known | F_components_known + | F_mat_known | F_is_invalid | F_hash_known; + state->cache_ref(); + state->_saved_entry = _states.store(state, nullptr); + _invalid_state = state; + } } /** @@ -1604,7 +1607,7 @@ do_invert_compose(const TransformState *other) const { // First, invert our own transform. if (scale == 0.0f) { ((TransformState *)this)->_flags |= F_is_singular | F_singular_known; - return make_invalid(); + return _invalid_state; } scale = 1.0f / scale; quat.invert_in_place(); @@ -1633,7 +1636,7 @@ do_invert_compose(const TransformState *other) const { // First, invert our own transform. if (scale == 0.0f) { ((TransformState *)this)->_flags |= F_is_singular | F_singular_known; - return make_invalid(); + return _invalid_state; } scale = 1.0f / scale; quat.invert_in_place(); @@ -1657,7 +1660,7 @@ do_invert_compose(const TransformState *other) const { pgraph_cat.warning() << "Unexpected singular matrix found for " << *this << "\n"; } else { - nassertr(_inv_mat != nullptr, make_invalid()); + nassertr(_inv_mat != nullptr, _invalid_state); LMatrix4 new_mat; new_mat.multiply(other->get_mat(), *_inv_mat); if (!new_mat.almost_equal(result->get_mat(), 0.1)) { @@ -1676,12 +1679,12 @@ do_invert_compose(const TransformState *other) const { } if (is_singular()) { - return make_invalid(); + return _invalid_state; } // Now that is_singular() has returned false, we can assume that _inv_mat // has been allocated and filled in. - nassertr(_inv_mat != nullptr, make_invalid()); + nassertr(_inv_mat != nullptr, _invalid_state); if (is_2d() && other->is_2d()) { const LMatrix4 &i = *_inv_mat; @@ -2009,40 +2012,40 @@ do_calc_hash() { int flags = (_flags & significant_flags); _hash = int_hash::add_hash(_hash, flags); - if ((_flags & (F_is_invalid | F_is_identity)) == 0) { - // Only bother to put the rest of the stuff in the hash if the transform - // is not invalid or empty. + nassertv((flags & (F_is_invalid | F_is_identity)) == 0); - if ((_flags & F_components_given) != 0) { - // If the transform was specified componentwise, hash it componentwise. - _hash = _pos.add_hash(_hash); - if ((_flags & F_hpr_given) != 0) { - _hash = _hpr.add_hash(_hash); + // Only bother to put the rest of the stuff in the hash if the transform + // is not invalid or empty. - } else if ((_flags & F_quat_given) != 0) { - _hash = _quat.add_hash(_hash); + if ((_flags & F_components_given) != 0) { + // If the transform was specified componentwise, hash it componentwise. + _hash = _pos.add_hash(_hash); + if ((_flags & F_hpr_given) != 0) { + _hash = _hpr.add_hash(_hash); + + } else if ((_flags & F_quat_given) != 0) { + _hash = _quat.add_hash(_hash); + } + + _hash = _scale.add_hash(_hash); + _hash = _shear.add_hash(_hash); + + } else { + // Otherwise, hash the matrix . . . + if (_uniquify_matrix) { + // . . . but only if the user thinks that's worthwhile. + if ((_flags & F_mat_known) == 0) { + // Calculate the matrix without doubly-locking. + do_calc_mat(); } - - _hash = _scale.add_hash(_hash); - _hash = _shear.add_hash(_hash); + _hash = _mat.add_hash(_hash); } else { - // Otherwise, hash the matrix . . . - if (_uniquify_matrix) { - // . . . but only if the user thinks that's worthwhile. - if ((_flags & F_mat_known) == 0) { - // Calculate the matrix without doubly-locking. - do_calc_mat(); - } - _hash = _mat.add_hash(_hash); + // Otherwise, hash the pointer only--any two different matrix-based + // TransformStates are considered to be different, even if their + // matrices have the same values. - } else { - // Otherwise, hash the pointer only--any two different matrix-based - // TransformStates are considered to be different, even if their - // matrices have the same values. - - _hash = pointer_hash::add_hash(_hash, this); - } + _hash = pointer_hash::add_hash(_hash, this); } } @@ -2063,7 +2066,7 @@ calc_singular() { PStatTimer timer(_transform_calc_pcollector); - nassertv((_flags & F_is_invalid) == 0); + nassertv((_flags & (F_is_invalid | F_is_identity)) == 0); // We determine if a matrix is singular by attempting to invert it (and we // save the result of this invert operation for a subsequent @@ -2100,39 +2103,30 @@ do_calc_components() { PStatTimer timer(_transform_calc_pcollector); - nassertv((_flags & F_is_invalid) == 0); - if ((_flags & F_is_identity) != 0) { - _scale.set(1.0f, 1.0f, 1.0f); - _shear.set(0.0f, 0.0f, 0.0f); - _hpr.set(0.0f, 0.0f, 0.0f); - _quat = LQuaternion::ident_quat(); - _pos.set(0.0f, 0.0f, 0.0f); - _flags |= F_has_components | F_components_known | F_hpr_known | F_quat_known | F_uniform_scale | F_identity_scale; + nassertv((_flags & (F_is_invalid | F_is_identity)) == 0); + + // If we don't have components and we're not identity, the only other + // explanation is that we were constructed via a matrix. + nassertv((_flags & F_mat_known) != 0); + + if ((_flags & F_mat_known) == 0) { + do_calc_mat(); + } + bool possible = decompose_matrix(_mat, _scale, _shear, _hpr, _pos); + if (!possible) { + // Some matrices can't be decomposed into scale, hpr, pos. In this + // case, we now know that we cannot compute the components; but the + // closest approximations are stored, at least. + _flags |= F_components_known | F_hpr_known; } else { - // If we don't have components and we're not identity, the only other - // explanation is that we were constructed via a matrix. - nassertv((_flags & F_mat_known) != 0); - - if ((_flags & F_mat_known) == 0) { - do_calc_mat(); - } - bool possible = decompose_matrix(_mat, _scale, _shear, _hpr, _pos); - if (!possible) { - // Some matrices can't be decomposed into scale, hpr, pos. In this - // case, we now know that we cannot compute the components; but the - // closest approximations are stored, at least. - _flags |= F_components_known | F_hpr_known; - - } else { - // Otherwise, we do have the components, or at least the hpr. - _flags |= F_has_components | F_components_known | F_hpr_known; - check_uniform_scale(); - } - - // However, we can always get at least the pos. - _mat.get_row3(_pos, 3); + // Otherwise, we do have the components, or at least the hpr. + _flags |= F_has_components | F_components_known | F_hpr_known; + check_uniform_scale(); } + + // However, we can always get at least the pos. + _mat.get_row3(_pos, 3); } /** @@ -2148,7 +2142,7 @@ do_calc_hpr() { PStatTimer timer(_transform_calc_pcollector); - nassertv((_flags & F_is_invalid) == 0); + nassertv((_flags & (F_is_invalid | F_is_identity)) == 0); if ((_flags & F_components_known) == 0) { do_calc_components(); } @@ -2174,7 +2168,7 @@ calc_quat() { PStatTimer timer(_transform_calc_pcollector); - nassertv((_flags & F_is_invalid) == 0); + nassertv((_flags & (F_is_invalid | F_is_identity)) == 0); if ((_flags & F_components_known) == 0) { do_calc_components(); } @@ -2214,20 +2208,16 @@ do_calc_mat() { PStatTimer timer(_transform_calc_pcollector); - nassertv((_flags & F_is_invalid) == 0); - if ((_flags & F_is_identity) != 0) { - _mat = LMatrix4::ident_mat(); + nassertv((_flags & (F_is_invalid | F_is_identity)) == 0); - } else { - // If we don't have a matrix and we're not identity, the only other - // explanation is that we were constructed via components. - nassertv((_flags & F_components_known) != 0); - if ((_flags & F_hpr_known) == 0) { - do_calc_hpr(); - } - - compose_matrix(_mat, _scale, _shear, get_hpr(), _pos); + // If we don't have a matrix and we're not identity, the only other + // explanation is that we were constructed via components. + nassertv((_flags & F_components_known) != 0); + if ((_flags & F_hpr_known) == 0) { + do_calc_hpr(); } + + compose_matrix(_mat, _scale, _shear, get_hpr(), _pos); _flags |= F_mat_known; } @@ -2341,7 +2331,18 @@ make_from_bam(const FactoryParams ¶ms) { parse_params(params, scan, manager); state->fillin(scan, manager); - manager->register_change_this(change_this, state); + + if (state->_flags & F_is_identity) { + delete state; + return (TypedWritable *)_identity_state.p(); + } + else if (state->_flags & F_is_invalid) { + delete state; + return (TypedWritable *)_invalid_state.p(); + } + else { + manager->register_change_this(change_this, state); + } return state; } diff --git a/panda/src/pgraph/transformState.h b/panda/src/pgraph/transformState.h index 7c34f3ff6e..60d377c06e 100644 --- a/panda/src/pgraph/transformState.h +++ b/panda/src/pgraph/transformState.h @@ -52,7 +52,7 @@ class FactoryParams; * And instead of modifying a TransformState object, create a new one. */ class EXPCL_PANDA_PGRAPH TransformState final : public NodeCachedReferenceCount { -protected: +private: TransformState(); public: @@ -69,8 +69,8 @@ PUBLISHED: bool operator == (const TransformState &other) const; INLINE size_t get_hash() const; - static CPT(TransformState) make_identity(); - static CPT(TransformState) make_invalid(); + INLINE static CPT(TransformState) make_identity(); + INLINE static CPT(TransformState) make_invalid(); INLINE static CPT(TransformState) make_pos(const LVecBase3 &pos); INLINE static CPT(TransformState) make_hpr(const LVecBase3 &hpr); INLINE static CPT(TransformState) make_quat(const LQuaternion &quat); @@ -268,7 +268,7 @@ private: // This iterator records the entry corresponding to this TransformState // object in the above global set. We keep the index around so we can // remove it when the TransformState destructs. - int _saved_entry; + int _saved_entry = -1; // This data structure manages the job of caching the composition of two // TransformStates. It's complicated because we have to be sure to remove @@ -372,7 +372,7 @@ private: LVecBase3 _hpr, _scale, _shear; LQuaternion _quat, _norm_quat; LMatrix4 _mat; - LMatrix4 *_inv_mat; + LMatrix4 *_inv_mat = nullptr; size_t _hash; unsigned int _flags; diff --git a/tests/pgraph/test_transforms.py b/tests/pgraph/test_transforms.py new file mode 100644 index 0000000000..d4ac4d2fdb --- /dev/null +++ b/tests/pgraph/test_transforms.py @@ -0,0 +1,68 @@ +from panda3d.core import TransformState, Mat4, Mat3 + + +def test_transform_identity(): + state = TransformState.make_identity() + + assert state.is_identity() + assert not state.is_invalid() + assert not state.is_singular() + assert state.is_2d() + + assert state.has_components() + assert not state.components_given() + assert not state.hpr_given() + assert not state.quat_given() + assert state.has_pos() + assert state.has_hpr() + assert state.has_quat() + assert state.has_scale() + assert state.has_identity_scale() + assert state.has_uniform_scale() + assert state.has_shear() + assert not state.has_nonzero_shear() + assert state.has_mat() + + assert state.get_pos() == (0, 0, 0) + assert state.get_hpr() == (0, 0, 0) + assert state.get_quat() == (1, 0, 0, 0) + assert state.get_norm_quat() == (1, 0, 0, 0) + assert state.get_scale() == (1, 1, 1) + assert state.get_uniform_scale() == 1 + assert state.get_shear() == (0, 0, 0) + assert state.get_mat() == Mat4.ident_mat() + + assert state.get_pos2d() == (0, 0) + assert state.get_rotate2d() == 0 + assert state.get_scale2d() == (1, 1) + assert state.get_shear2d() == 0 + assert state.get_mat3() == Mat3.ident_mat() + + state2 = TransformState.make_identity() + assert state.this == state2.this + + +def test_transform_invalid(): + state = TransformState.make_invalid() + + assert not state.is_identity() + assert state.is_invalid() + assert state.is_singular() + assert not state.is_2d() + + assert not state.has_components() + assert not state.components_given() + assert not state.hpr_given() + assert not state.quat_given() + assert not state.has_pos() + assert not state.has_hpr() + assert not state.has_quat() + assert not state.has_scale() + assert not state.has_identity_scale() + assert not state.has_uniform_scale() + assert not state.has_shear() + assert not state.has_nonzero_shear() + assert not state.has_mat() + + state2 = TransformState.make_invalid() + assert state.this == state2.this