pgraph: Optimize handling/checking of identity/invalid transforms

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;
 }
 
 /**

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) {
+  nassertv((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.
 
-    if ((_flags & F_components_given) != 0) {
+  // Only bother to put the rest of the stuff in the hash if the transform
-      // If the transform was specified componentwise, hash it componentwise.
+  // is not invalid or empty.
-      _hash = _pos.add_hash(_hash);
-      if ((_flags & F_hpr_given) != 0) {
-        _hash = _hpr.add_hash(_hash);
 
-      } else if ((_flags & F_quat_given) != 0) {
+  if ((_flags & F_components_given) != 0) {
-        _hash = _quat.add_hash(_hash);
+    // 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 = _mat.add_hash(_hash);
-      _hash = _scale.add_hash(_hash);
-      _hash = _shear.add_hash(_hash);
 
     } else {
-      // Otherwise, hash the matrix . . .
+      // Otherwise, hash the pointer only--any two different matrix-based
-      if (_uniquify_matrix) {
+      // TransformStates are considered to be different, even if their
-        // . . . but only if the user thinks that's worthwhile.
+      // matrices have the same values.
-        if ((_flags & F_mat_known) == 0) {
-          // Calculate the matrix without doubly-locking.
-          do_calc_mat();
-        }
-        _hash = _mat.add_hash(_hash);
 
-      } else {
+      _hash = pointer_hash::add_hash(_hash, this);
-        // 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);
-      }
     }
   }
 
@@ -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);
+  nassertv((_flags & (F_is_invalid | F_is_identity)) == 0);
-  if ((_flags & F_is_identity) != 0) {
+
-    _scale.set(1.0f, 1.0f, 1.0f);
+  // If we don't have components and we're not identity, the only other
-    _shear.set(0.0f, 0.0f, 0.0f);
+  // explanation is that we were constructed via a matrix.
-    _hpr.set(0.0f, 0.0f, 0.0f);
+  nassertv((_flags & F_mat_known) != 0);
-    _quat = LQuaternion::ident_quat();
+
-    _pos.set(0.0f, 0.0f, 0.0f);
+  if ((_flags & F_mat_known) == 0) {
-    _flags |= F_has_components | F_components_known | F_hpr_known | F_quat_known | F_uniform_scale | F_identity_scale;
+    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
+    // Otherwise, we do have the components, or at least the hpr.
-    // explanation is that we were constructed via a matrix.
+    _flags |= F_has_components | F_components_known | F_hpr_known;
-    nassertv((_flags & F_mat_known) != 0);
+    check_uniform_scale();
-
-    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);
   }
+
+  // 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);
+  nassertv((_flags & (F_is_invalid | F_is_identity)) == 0);
-  if ((_flags & F_is_identity) != 0) {
-    _mat = LMatrix4::ident_mat();
 
-  } else {
+  // If we don't have a matrix and we're not identity, the only other
-    // If we don't have a matrix and we're not identity, the only other
+  // explanation is that we were constructed via components.
-    // explanation is that we were constructed via components.
+  nassertv((_flags & F_components_known) != 0);
-    nassertv((_flags & F_components_known) != 0);
+  if ((_flags & F_hpr_known) == 0) {
-    if ((_flags & F_hpr_known) == 0) {
+    do_calc_hpr();
-      do_calc_hpr();
-    }
-
-    compose_matrix(_mat, _scale, _shear, get_hpr(), _pos);
   }
+
+  compose_matrix(_mat, _scale, _shear, get_hpr(), _pos);
   _flags |= F_mat_known;
 }
 
@@ -2341,7 +2331,18 @@ make_from_bam(const FactoryParams &params) {
 
   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;
 }

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();
+  INLINE static CPT(TransformState) make_identity();
-  static CPT(TransformState) make_invalid();
+  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;

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