From c5c1d4557bf63d9a9343ea76a1ae3f1be5d36639 Mon Sep 17 00:00:00 2001 From: rdb Date: Tue, 10 Mar 2020 12:01:51 +0100 Subject: [PATCH 1/5] pgraph: fix double free if weak ptr to state is locked while being gc'ed Fixes #499 --- panda/src/express/referenceCount.I | 15 +++++++++++++++ panda/src/express/referenceCount.h | 1 + panda/src/pgraph/renderAttrib.cxx | 7 ++++--- panda/src/pgraph/renderState.cxx | 8 +++++--- panda/src/pgraph/transformState.cxx | 7 ++++--- 5 files changed, 29 insertions(+), 9 deletions(-) diff --git a/panda/src/express/referenceCount.I b/panda/src/express/referenceCount.I index bb4ac1fc18..6b5847407c 100644 --- a/panda/src/express/referenceCount.I +++ b/panda/src/express/referenceCount.I @@ -317,6 +317,21 @@ ref_if_nonzero() const { return true; } +/** + * Atomically decreases the reference count of this object if it is one. + * Do not use this. This exists only to implement a special case with the + * state cache. + * @return true if the reference count was decremented to zero. + */ +INLINE bool ReferenceCount:: +unref_if_one() const { +#ifdef _DEBUG + nassertr(test_ref_count_integrity(), 0); + nassertr(_ref_count > 0, 0); +#endif + return (AtomicAdjust::compare_and_exchange(_ref_count, 1, 0) == 1); +} + /** * This global helper function will unref the given ReferenceCount object, and * if the reference count reaches zero, automatically delete it. It can't be diff --git a/panda/src/express/referenceCount.h b/panda/src/express/referenceCount.h index 46f0231117..09ef7b4501 100644 --- a/panda/src/express/referenceCount.h +++ b/panda/src/express/referenceCount.h @@ -64,6 +64,7 @@ public: INLINE void weak_unref(); INLINE bool ref_if_nonzero() const; + INLINE bool unref_if_one() const; protected: bool do_test_ref_count_integrity() const; diff --git a/panda/src/pgraph/renderAttrib.cxx b/panda/src/pgraph/renderAttrib.cxx index 9bac4e666a..9b5bbd6303 100644 --- a/panda/src/pgraph/renderAttrib.cxx +++ b/panda/src/pgraph/renderAttrib.cxx @@ -215,14 +215,15 @@ garbage_collect() { do { RenderAttrib *attrib = (RenderAttrib *)_attribs->get_key(si); - if (attrib->get_ref_count() == 1) { + if (attrib->unref_if_one()) { // This attrib has recently been unreffed to 1 (the one we added when // we stored it in the cache). Now it's time to delete it. This is // safe, because we're holding the _attribs_lock, so it's not possible // for some other thread to find the attrib in the cache and ref it - // while we're doing this. + // while we're doing this. Also, we've just made sure to unref it to 0, + // to ensure that another thread can't get it via a weak pointer. attrib->release_new(); - unref_delete(attrib); + delete attrib; // When we removed it from the hash map, it swapped the last element // with the one we just removed. So the current index contains one we diff --git a/panda/src/pgraph/renderState.cxx b/panda/src/pgraph/renderState.cxx index 5ce1527247..5098f9dc34 100644 --- a/panda/src/pgraph/renderState.cxx +++ b/panda/src/pgraph/renderState.cxx @@ -934,15 +934,17 @@ garbage_collect() { } } - if (state->get_ref_count() == 1) { + if (state->unref_if_one()) { // This state has recently been unreffed to 1 (the one we added when // we stored it in the cache). Now it's time to delete it. This is // safe, because we're holding the _states_lock, so it's not possible // for some other thread to find the state in the cache and ref it - // while we're doing this. + // while we're doing this. Also, we've just made sure to unref it to 0, + // to ensure that another thread can't get it via a weak pointer. + state->release_new(); state->remove_cache_pointers(); - state->cache_unref(); + state->cache_unref_only(); delete state; // When we removed it from the hash map, it swapped the last element diff --git a/panda/src/pgraph/transformState.cxx b/panda/src/pgraph/transformState.cxx index fc8b3de606..220c9c5d8b 100644 --- a/panda/src/pgraph/transformState.cxx +++ b/panda/src/pgraph/transformState.cxx @@ -1204,15 +1204,16 @@ garbage_collect() { } } - if (state->get_ref_count() == 1) { + if (state->unref_if_one()) { // This state has recently been unreffed to 1 (the one we added when // we stored it in the cache). Now it's time to delete it. This is // safe, because we're holding the _states_lock, so it's not possible // for some other thread to find the state in the cache and ref it - // while we're doing this. + // while we're doing this. Also, we've just made sure to unref it to 0, + // to ensure that another thread can't get it via a weak pointer. state->release_new(); state->remove_cache_pointers(); - state->cache_unref(); + state->cache_unref_only(); delete state; // When we removed it from the hash map, it swapped the last element From ebd538a7f83bd4ff9938a7e5f97a9ad8e6198552 Mon Sep 17 00:00:00 2001 From: rdb Date: Sat, 14 Mar 2020 14:48:31 +0100 Subject: [PATCH 2/5] bam2egg: fix skinning bug for joints with DefaultPose --- panda/src/egg2pg/eggSaver.cxx | 55 +++++++++++++++++++++++++++++++---- panda/src/egg2pg/eggSaver.h | 3 ++ 2 files changed, 52 insertions(+), 6 deletions(-) diff --git a/panda/src/egg2pg/eggSaver.cxx b/panda/src/egg2pg/eggSaver.cxx index 451497381c..13483a55d3 100644 --- a/panda/src/egg2pg/eggSaver.cxx +++ b/panda/src/egg2pg/eggSaver.cxx @@ -345,18 +345,61 @@ convert_anim_node(AnimBundleNode *node, const WorkingNodePath &node_path, */ void EggSaver:: convert_character_bundle(PartGroup *bundleNode, EggGroupNode *egg_parent, CharacterJointMap *joint_map) { + convert_character_bundle(bundleNode, egg_parent, joint_map, nullptr); +} + +/** + * Converts the indicated Character Bundle to the corresponding Egg joints + * structure. + */ +void EggSaver:: +convert_character_bundle(PartGroup *bundleNode, EggGroupNode *egg_parent, + CharacterJointMap *joint_map, const CharacterJoint *parent_joint) { int num_children = bundleNode->get_num_children(); + const CharacterJoint *character_joint = nullptr; + EggGroupNode *joint_group = egg_parent; if (bundleNode->is_of_type(CharacterJoint::get_class_type())) { - CharacterJoint *character_joint = DCAST(CharacterJoint, bundleNode); + character_joint = DCAST(CharacterJoint, bundleNode); - LMatrix4 transformf; - character_joint->get_transform(transformf); - LMatrix4d transformd(LCAST(double, transformf)); EggGroup *joint = new EggGroup(bundleNode->get_name()); - joint->add_matrix4(transformd); joint->set_group_type(EggGroup::GT_joint); + + // The default_value originally passed to the CharacterJoint is what is used + // for skinning. However, the _default_value can be changed after joint + // construction (such as via a ), so we can't just pull the + // current _default_value. + // + // We have to instead work back from the _initial_net_transform_inverse, + // which is computed at construction time from the original default_value: + // + // _net_transform = default_value * parent_joint->_net_transform; + // _initial_net_transform_inverse = invert(_net_transform); + // + // So we should be able to reconstruct the original default_value like so: + // + // default_value = invert(_initial_net_transform_inverse) + // * parent_joint->_initial_net_transform_inverse; + // + LMatrix4d net_transform = invert(LCAST(double, character_joint->_initial_net_transform_inverse)); + if (parent_joint != nullptr) { + if (parent_joint->_initial_net_transform_inverse != character_joint->_initial_net_transform_inverse) { + LMatrix4d parent_inverse = LCAST(double, parent_joint->_initial_net_transform_inverse); + joint->add_matrix4(net_transform * parent_inverse); + } + } else if (!net_transform.is_identity()) { + joint->add_matrix4(net_transform); + } + + // The joint's _default_value, if different, goes into a . + LMatrix4d default_pose = LCAST(double, character_joint->_default_value); + if (default_pose != joint->get_transform3d()) { + EggTransform transform; + transform.add_matrix4(LCAST(double, default_pose)); + joint->set_default_pose(transform); + } + joint_group = joint; egg_parent->add_child(joint_group); if (joint_map != nullptr) { @@ -373,7 +416,7 @@ convert_character_bundle(PartGroup *bundleNode, EggGroupNode *egg_parent, Charac for (int i = 0; i < num_children ; i++) { PartGroup *partGroup= bundleNode->get_child(i); - convert_character_bundle(partGroup, joint_group, joint_map); + convert_character_bundle(partGroup, joint_group, joint_map, character_joint); } } diff --git a/panda/src/egg2pg/eggSaver.h b/panda/src/egg2pg/eggSaver.h index 48a9693dec..c3b3dd8d4c 100644 --- a/panda/src/egg2pg/eggSaver.h +++ b/panda/src/egg2pg/eggSaver.h @@ -78,6 +78,9 @@ private: void convert_character_node(Character *node, const WorkingNodePath &node_path, EggGroupNode *egg_parent, bool has_decal); void convert_character_bundle(PartGroup *bundleNode, EggGroupNode *egg_parent, CharacterJointMap *jointMap); + void convert_character_bundle(PartGroup *bundleNode, EggGroupNode *egg_parent, + CharacterJointMap *jointMap, + const CharacterJoint *parent_joint); void convert_collision_node(CollisionNode *node, const WorkingNodePath &node_path, EggGroupNode *egg_parent, bool has_decal, CharacterJointMap *joint_map); From 9966ddaa33b6940a5745d0715ecda6a709f7b322 Mon Sep 17 00:00:00 2001 From: rdb Date: Sat, 14 Mar 2020 14:53:07 +0100 Subject: [PATCH 3/5] bam2egg: support multitexturing, multiple UV sets --- panda/src/egg2pg/eggSaver.cxx | 58 +++++++++++++++++++++++++++++------ 1 file changed, 48 insertions(+), 10 deletions(-) diff --git a/panda/src/egg2pg/eggSaver.cxx b/panda/src/egg2pg/eggSaver.cxx index 13483a55d3..dc7be82d7d 100644 --- a/panda/src/egg2pg/eggSaver.cxx +++ b/panda/src/egg2pg/eggSaver.cxx @@ -750,6 +750,7 @@ convert_primitive(const GeomVertexData *vertex_data, const LMatrix4 &net_mat, EggGroupNode *egg_parent, CharacterJointMap *joint_map) { GeomVertexReader reader(vertex_data); + const GeomVertexFormat *format = vertex_data->get_format(); // Make a zygote that will be duplicated for each primitive. PT(EggPrimitive) egg_prim; @@ -808,14 +809,14 @@ convert_primitive(const GeomVertexData *vertex_data, // Check for a texture. const TextureAttrib *ta; if (net_state->get_attrib(ta)) { - EggTexture *egg_tex = get_egg_texture(ta->get_texture()); + for (size_t i = 0; i < ta->get_num_on_stages(); ++i) { + TextureStage *tex_stage = ta->get_on_stage(i); - if (egg_tex != nullptr) { - TextureStage *tex_stage = ta->get_on_stage(0); - if (tex_stage != nullptr) { + EggTexture *egg_tex = get_egg_texture(ta->get_on_texture(tex_stage)); + if (egg_tex != nullptr) { switch (tex_stage->get_mode()) { case TextureStage::M_modulate: - if (has_color_off == true) { + if (has_color_off == true && i == 0) { egg_tex->set_env_type(EggTexture::ET_replace); } else { egg_tex->set_env_type(EggTexture::ET_modulate); @@ -836,12 +837,44 @@ convert_primitive(const GeomVertexData *vertex_data, case TextureStage::M_blend_color_scale: egg_tex->set_env_type(EggTexture::ET_blend_color_scale); break; + case TextureStage::M_modulate_glow: + egg_tex->set_env_type(EggTexture::ET_modulate_glow); + break; + case TextureStage::M_modulate_gloss: + egg_tex->set_env_type(EggTexture::ET_modulate_gloss); + break; + case TextureStage::M_normal: + egg_tex->set_env_type(EggTexture::ET_normal); + break; + case TextureStage::M_normal_height: + egg_tex->set_env_type(EggTexture::ET_normal_height); + break; + case TextureStage::M_glow: + egg_tex->set_env_type(EggTexture::ET_glow); + break; + case TextureStage::M_gloss: + egg_tex->set_env_type(EggTexture::ET_gloss); + break; + case TextureStage::M_height: + egg_tex->set_env_type(EggTexture::ET_height); + break; + case TextureStage::M_selector: + egg_tex->set_env_type(EggTexture::ET_selector); + break; + case TextureStage::M_normal_gloss: + egg_tex->set_env_type(EggTexture::ET_normal_gloss); + break; default: break; } - } - egg_prim->set_texture(egg_tex); + const InternalName *name = tex_stage->get_texcoord_name(); + if (name != nullptr && name != InternalName::get_texcoord()) { + egg_tex->set_uv_name(name->get_basename()); + } + + egg_prim->add_texture(egg_tex); + } } } @@ -906,10 +939,15 @@ convert_primitive(const GeomVertexData *vertex_data, color[3] * color_scale[3])); } - if (vertex_data->has_column(InternalName::get_texcoord())) { - reader.set_column(InternalName::get_texcoord()); + for (size_t ti = 0; ti < format->get_num_texcoords(); ++ti) { + const InternalName *texcoord_name = format->get_texcoord(ti); + reader.set_column(texcoord_name); LTexCoord uv = reader.get_data2(); - egg_vert.set_uv(LCAST(double, uv)); + if (texcoord_name == InternalName::get_texcoord()) { + egg_vert.set_uv(LCAST(double, uv)); + } else { + egg_vert.set_uv(texcoord_name->get_basename(), LCAST(double, uv)); + } } EggVertex *new_egg_vert = _vpool->create_unique_vertex(egg_vert); From 32a9ea2ceffe3a0bc1e3c9672a79e58af54fcf77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Derzsi=20D=C3=A1niel?= Date: Fri, 13 Mar 2020 22:37:25 +0200 Subject: [PATCH 4/5] dgui: Fix DirectScrolledList scrollTo error in Python 3 As a rule of thumb, Python 3 divisions always have float results. Unfortunately, this piece of code is still relying on the old Python 2.7 behavior. Closes #880 --- direct/src/gui/DirectScrolledList.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/direct/src/gui/DirectScrolledList.py b/direct/src/gui/DirectScrolledList.py index 5bade6caf9..6cb39675b6 100644 --- a/direct/src/gui/DirectScrolledList.py +++ b/direct/src/gui/DirectScrolledList.py @@ -210,7 +210,7 @@ class DirectScrolledList(DirectFrame): numItemsVisible = self["numItemsVisible"] numItemsTotal = len(self["items"]) if(centered): - self.index = index - (numItemsVisible/2) + self.index = index - (numItemsVisible // 2) else: self.index = index From 1b67931f1646d4ca3a472ac2ba1b29f87a0b979c Mon Sep 17 00:00:00 2001 From: rdb Date: Sat, 14 Mar 2020 14:59:20 +0100 Subject: [PATCH 5/5] express: invert return value of unref_if_one() This is more consistent with how the return value of unref() works. Someone might otherwise trip over this. --- panda/src/express/referenceCount.I | 4 ++-- panda/src/pgraph/renderAttrib.cxx | 2 +- panda/src/pgraph/renderState.cxx | 2 +- panda/src/pgraph/transformState.cxx | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/panda/src/express/referenceCount.I b/panda/src/express/referenceCount.I index 6b5847407c..f4d71b1117 100644 --- a/panda/src/express/referenceCount.I +++ b/panda/src/express/referenceCount.I @@ -321,7 +321,7 @@ ref_if_nonzero() const { * Atomically decreases the reference count of this object if it is one. * Do not use this. This exists only to implement a special case with the * state cache. - * @return true if the reference count was decremented to zero. + * @return false if the reference count was decremented to zero. */ INLINE bool ReferenceCount:: unref_if_one() const { @@ -329,7 +329,7 @@ unref_if_one() const { nassertr(test_ref_count_integrity(), 0); nassertr(_ref_count > 0, 0); #endif - return (AtomicAdjust::compare_and_exchange(_ref_count, 1, 0) == 1); + return (AtomicAdjust::compare_and_exchange(_ref_count, 1, 0) != 1); } /** diff --git a/panda/src/pgraph/renderAttrib.cxx b/panda/src/pgraph/renderAttrib.cxx index 9b5bbd6303..9c9d9e167b 100644 --- a/panda/src/pgraph/renderAttrib.cxx +++ b/panda/src/pgraph/renderAttrib.cxx @@ -215,7 +215,7 @@ garbage_collect() { do { RenderAttrib *attrib = (RenderAttrib *)_attribs->get_key(si); - if (attrib->unref_if_one()) { + if (!attrib->unref_if_one()) { // This attrib has recently been unreffed to 1 (the one we added when // we stored it in the cache). Now it's time to delete it. This is // safe, because we're holding the _attribs_lock, so it's not possible diff --git a/panda/src/pgraph/renderState.cxx b/panda/src/pgraph/renderState.cxx index 5098f9dc34..fdc187ecbb 100644 --- a/panda/src/pgraph/renderState.cxx +++ b/panda/src/pgraph/renderState.cxx @@ -934,7 +934,7 @@ garbage_collect() { } } - if (state->unref_if_one()) { + if (!state->unref_if_one()) { // This state has recently been unreffed to 1 (the one we added when // we stored it in the cache). Now it's time to delete it. This is // safe, because we're holding the _states_lock, so it's not possible diff --git a/panda/src/pgraph/transformState.cxx b/panda/src/pgraph/transformState.cxx index 220c9c5d8b..efbb97f2bb 100644 --- a/panda/src/pgraph/transformState.cxx +++ b/panda/src/pgraph/transformState.cxx @@ -1204,7 +1204,7 @@ garbage_collect() { } } - if (state->unref_if_one()) { + if (!state->unref_if_one()) { // This state has recently been unreffed to 1 (the one we added when // we stored it in the cache). Now it's time to delete it. This is // safe, because we're holding the _states_lock, so it's not possible