From 668a093c26f3b4f7873d65c224d38f1b751646e9 Mon Sep 17 00:00:00 2001 From: rdb Date: Fri, 3 Aug 2018 20:45:29 +0200 Subject: [PATCH] gobj: try to fix some deadlocks modifying geometry while rendering The correct locking order should be: the Geom's GeomVertexArrayData first, *then* the GeomPrimitive. --- panda/src/cull/cullBinBackToFront.cxx | 5 ++- panda/src/cull/cullBinFixed.cxx | 5 ++- panda/src/cull/cullBinFrontToBack.cxx | 5 ++- panda/src/cull/cullBinStateSorted.cxx | 5 ++- panda/src/cull/cullBinUnsorted.cxx | 8 ++-- panda/src/gobj/geom.cxx | 61 +++++++++++++++++++++------ panda/src/gobj/geomPrimitive.I | 14 ++++-- panda/src/gobj/geomPrimitive.h | 1 + 8 files changed, 76 insertions(+), 28 deletions(-) diff --git a/panda/src/cull/cullBinBackToFront.cxx b/panda/src/cull/cullBinBackToFront.cxx index 00c6ba7c32..c27c7e8ed4 100644 --- a/panda/src/cull/cullBinBackToFront.cxx +++ b/panda/src/cull/cullBinBackToFront.cxx @@ -96,9 +96,10 @@ draw(bool force, Thread *current_thread) { nassertd(object->_geom != nullptr) continue; _gsg->set_state_and_transform(object->_state, object->_internal_transform); - data_reader.set_object(object->_munged_data); + + GeomPipelineReader geom_reader(object->_geom, current_thread); + GeomVertexDataPipelineReader data_reader(object->_munged_data, current_thread); data_reader.check_array_readers(); - geom_reader.set_object(object->_geom); geom_reader.draw(_gsg, &data_reader, force); } else { // It has a callback associated. diff --git a/panda/src/cull/cullBinFixed.cxx b/panda/src/cull/cullBinFixed.cxx index 557b3435a3..3507ce4582 100644 --- a/panda/src/cull/cullBinFixed.cxx +++ b/panda/src/cull/cullBinFixed.cxx @@ -82,9 +82,10 @@ draw(bool force, Thread *current_thread) { nassertd(object->_geom != nullptr) continue; _gsg->set_state_and_transform(object->_state, object->_internal_transform); - data_reader.set_object(object->_munged_data); + + GeomPipelineReader geom_reader(object->_geom, current_thread); + GeomVertexDataPipelineReader data_reader(object->_munged_data, current_thread); data_reader.check_array_readers(); - geom_reader.set_object(object->_geom); geom_reader.draw(_gsg, &data_reader, force); } else { // It has a callback associated. diff --git a/panda/src/cull/cullBinFrontToBack.cxx b/panda/src/cull/cullBinFrontToBack.cxx index 4337b42d7c..5f5d99e51c 100644 --- a/panda/src/cull/cullBinFrontToBack.cxx +++ b/panda/src/cull/cullBinFrontToBack.cxx @@ -96,9 +96,10 @@ draw(bool force, Thread *current_thread) { nassertd(object->_geom != nullptr) continue; _gsg->set_state_and_transform(object->_state, object->_internal_transform); - data_reader.set_object(object->_munged_data); + + GeomPipelineReader geom_reader(object->_geom, current_thread); + GeomVertexDataPipelineReader data_reader(object->_munged_data, current_thread); data_reader.check_array_readers(); - geom_reader.set_object(object->_geom); geom_reader.draw(_gsg, &data_reader, force); } else { // It has a callback associated. diff --git a/panda/src/cull/cullBinStateSorted.cxx b/panda/src/cull/cullBinStateSorted.cxx index ad4d26cd12..3e2fca2e9a 100644 --- a/panda/src/cull/cullBinStateSorted.cxx +++ b/panda/src/cull/cullBinStateSorted.cxx @@ -81,9 +81,10 @@ draw(bool force, Thread *current_thread) { nassertd(object->_geom != nullptr) continue; _gsg->set_state_and_transform(object->_state, object->_internal_transform); - data_reader.set_object(object->_munged_data); + + GeomPipelineReader geom_reader(object->_geom, current_thread); + GeomVertexDataPipelineReader data_reader(object->_munged_data, current_thread); data_reader.check_array_readers(); - geom_reader.set_object(object->_geom); geom_reader.draw(_gsg, &data_reader, force); } else { // It has a callback associated. diff --git a/panda/src/cull/cullBinUnsorted.cxx b/panda/src/cull/cullBinUnsorted.cxx index 14766f9bbd..0f37ae97b0 100644 --- a/panda/src/cull/cullBinUnsorted.cxx +++ b/panda/src/cull/cullBinUnsorted.cxx @@ -55,9 +55,6 @@ void CullBinUnsorted:: draw(bool force, Thread *current_thread) { PStatTimer timer(_draw_this_pcollector, current_thread); - GeomPipelineReader geom_reader(current_thread); - GeomVertexDataPipelineReader data_reader(current_thread); - Objects::iterator oi; for (oi = _objects.begin(); oi != _objects.end(); ++oi) { CullableObject *object = (*oi); @@ -66,9 +63,10 @@ draw(bool force, Thread *current_thread) { nassertd(object->_geom != nullptr) continue; _gsg->set_state_and_transform(object->_state, object->_internal_transform); - data_reader.set_object(object->_munged_data); + + GeomPipelineReader geom_reader(object->_geom, current_thread); + GeomVertexDataPipelineReader data_reader(object->_munged_data, current_thread); data_reader.check_array_readers(); - geom_reader.set_object(object->_geom); geom_reader.draw(_gsg, &data_reader, force); } else { // It has a callback associated. diff --git a/panda/src/gobj/geom.cxx b/panda/src/gobj/geom.cxx index 5cf18838d8..565f8c47b4 100644 --- a/panda/src/gobj/geom.cxx +++ b/panda/src/gobj/geom.cxx @@ -195,6 +195,9 @@ offset_vertices(const GeomVertexData *data, int offset) { cdata->_data = (GeomVertexData *)data; #ifndef NDEBUG + GeomVertexDataPipelineReader data_reader(data, current_thread); + data_reader.check_array_readers(); + bool all_is_valid = true; #endif Primitives::iterator pi; @@ -203,7 +206,7 @@ offset_vertices(const GeomVertexData *data, int offset) { prim->offset_vertices(offset); #ifndef NDEBUG - if (!prim->check_valid(data)) { + if (!prim->check_valid(&data_reader)) { gobj_cat.warning() << *prim << " is invalid for " << *data << ":\n"; prim->write(gobj_cat.warning(false), 4); @@ -423,6 +426,9 @@ decompose_in_place() { CDWriter cdata(_cycler, true, current_thread); #ifndef NDEBUG + GeomVertexDataPipelineReader data_reader(cdata->_data.get_read_pointer(current_thread), current_thread); + data_reader.check_array_readers(); + bool all_is_valid = true; #endif Primitives::iterator pi; @@ -431,7 +437,7 @@ decompose_in_place() { (*pi) = (GeomPrimitive *)new_prim.p(); #ifndef NDEBUG - if (!new_prim->check_valid(cdata->_data.get_read_pointer(current_thread))) { + if (!new_prim->check_valid(&data_reader)) { all_is_valid = false; } #endif @@ -457,6 +463,9 @@ doubleside_in_place() { CDWriter cdata(_cycler, true, current_thread); #ifndef NDEBUG + GeomVertexDataPipelineReader data_reader(cdata->_data.get_read_pointer(current_thread), current_thread); + data_reader.check_array_readers(); + bool all_is_valid = true; #endif Primitives::iterator pi; @@ -465,7 +474,7 @@ doubleside_in_place() { (*pi) = (GeomPrimitive *)new_prim.p(); #ifndef NDEBUG - if (!new_prim->check_valid(cdata->_data.get_read_pointer(current_thread))) { + if (!new_prim->check_valid(&data_reader)) { all_is_valid = false; } #endif @@ -491,6 +500,9 @@ reverse_in_place() { CDWriter cdata(_cycler, true, current_thread); #ifndef NDEBUG + GeomVertexDataPipelineReader data_reader(cdata->_data.get_read_pointer(current_thread), current_thread); + data_reader.check_array_readers(); + bool all_is_valid = true; #endif Primitives::iterator pi; @@ -499,7 +511,7 @@ reverse_in_place() { (*pi) = (GeomPrimitive *)new_prim.p(); #ifndef NDEBUG - if (!new_prim->check_valid(cdata->_data.get_read_pointer(current_thread))) { + if (!new_prim->check_valid(&data_reader)) { all_is_valid = false; } #endif @@ -525,6 +537,9 @@ rotate_in_place() { CDWriter cdata(_cycler, true, current_thread); #ifndef NDEBUG + GeomVertexDataPipelineReader data_reader(cdata->_data.get_read_pointer(current_thread), current_thread); + data_reader.check_array_readers(); + bool all_is_valid = true; #endif Primitives::iterator pi; @@ -533,7 +548,7 @@ rotate_in_place() { (*pi) = (GeomPrimitive *)new_prim.p(); #ifndef NDEBUG - if (!new_prim->check_valid(cdata->_data.get_read_pointer(current_thread))) { + if (!new_prim->check_valid(&data_reader)) { all_is_valid = false; } #endif @@ -640,6 +655,9 @@ unify_in_place(int max_indices, bool preserve_order) { // primitives.) nassertv(false); } + + GeomVertexDataPipelineReader data_reader(cdata->_data.get_read_pointer(current_thread), current_thread); + data_reader.check_array_readers(); #endif // Finally, iterate through the remaining primitives, and copy them to the @@ -649,7 +667,7 @@ unify_in_place(int max_indices, bool preserve_order) { for (npi = new_prims.begin(); npi != new_prims.end(); ++npi) { GeomPrimitive *prim = (*npi).second; - nassertv(prim->check_valid(cdata->_data.get_read_pointer(current_thread))); + nassertv(prim->check_valid(&data_reader)); // Each new primitive, naturally, inherits the Geom's overall shade model. prim->set_shade_model(cdata->_shade_model); @@ -748,6 +766,9 @@ make_lines_in_place() { CDWriter cdata(_cycler, true, current_thread); #ifndef NDEBUG + GeomVertexDataPipelineReader data_reader(cdata->_data.get_read_pointer(current_thread), current_thread); + data_reader.check_array_readers(); + bool all_is_valid = true; #endif Primitives::iterator pi; @@ -756,7 +777,7 @@ make_lines_in_place() { (*pi) = (GeomPrimitive *)new_prim.p(); #ifndef NDEBUG - if (!new_prim->check_valid(cdata->_data.get_read_pointer(current_thread))) { + if (!new_prim->check_valid(&data_reader)) { all_is_valid = false; } #endif @@ -782,6 +803,9 @@ make_points_in_place() { CDWriter cdata(_cycler, true, current_thread); #ifndef NDEBUG + GeomVertexDataPipelineReader data_reader(cdata->_data.get_read_pointer(current_thread), current_thread); + data_reader.check_array_readers(); + bool all_is_valid = true; #endif Primitives::iterator pi; @@ -790,7 +814,7 @@ make_points_in_place() { (*pi) = (GeomPrimitive *)new_prim.p(); #ifndef NDEBUG - if (!new_prim->check_valid(cdata->_data.get_read_pointer(current_thread))) { + if (!new_prim->check_valid(&data_reader)) { all_is_valid = false; } #endif @@ -816,6 +840,9 @@ make_patches_in_place() { CDWriter cdata(_cycler, true, current_thread); #ifndef NDEBUG + GeomVertexDataPipelineReader data_reader(cdata->_data.get_read_pointer(current_thread), current_thread); + data_reader.check_array_readers(); + bool all_is_valid = true; #endif Primitives::iterator pi; @@ -824,7 +851,7 @@ make_patches_in_place() { (*pi) = (GeomPrimitive *)new_prim.p(); #ifndef NDEBUG - if (!new_prim->check_valid(cdata->_data.get_read_pointer(current_thread))) { + if (!new_prim->check_valid(&data_reader)) { all_is_valid = false; } #endif @@ -850,6 +877,9 @@ make_adjacency_in_place() { CDWriter cdata(_cycler, true, current_thread); #ifndef NDEBUG + GeomVertexDataPipelineReader data_reader(cdata->_data.get_read_pointer(current_thread), current_thread); + data_reader.check_array_readers(); + bool all_is_valid = true; #endif Primitives::iterator pi; @@ -859,7 +889,7 @@ make_adjacency_in_place() { (*pi) = (GeomPrimitive *)new_prim.p(); #ifndef NDEBUG - if (!new_prim->check_valid(cdata->_data.get_read_pointer(current_thread))) { + if (!new_prim->check_valid(&data_reader)) { all_is_valid = false; } #endif @@ -1465,13 +1495,20 @@ clear_prepared(PreparedGraphicsObjects *prepared_objects) { */ bool Geom:: check_will_be_valid(const GeomVertexData *vertex_data) const { - CDReader cdata(_cycler); + Thread *current_thread = Thread::get_current_thread(); + + CDReader cdata(_cycler, current_thread); + + GeomVertexDataPipelineReader data_reader(vertex_data, current_thread); + data_reader.check_array_readers(); Primitives::const_iterator pi; for (pi = cdata->_primitives.begin(); pi != cdata->_primitives.end(); ++pi) { - if (!(*pi).get_read_pointer()->check_valid(vertex_data)) { + GeomPrimitivePipelineReader reader((*pi).get_read_pointer(), current_thread); + reader.check_minmax(); + if (!reader.check_valid(&data_reader)) { return false; } } diff --git a/panda/src/gobj/geomPrimitive.I b/panda/src/gobj/geomPrimitive.I index 0984dbecb3..f314c81090 100644 --- a/panda/src/gobj/geomPrimitive.I +++ b/panda/src/gobj/geomPrimitive.I @@ -223,11 +223,19 @@ get_modified() const { INLINE bool GeomPrimitive:: check_valid(const GeomVertexData *vertex_data) const { Thread *current_thread = Thread::get_current_thread(); - GeomPrimitivePipelineReader reader(this, current_thread); - reader.check_minmax(); GeomVertexDataPipelineReader data_reader(vertex_data, current_thread); data_reader.check_array_readers(); - return reader.check_valid(&data_reader); + return check_valid(&data_reader); +} + +/** + * + */ +INLINE bool GeomPrimitive:: +check_valid(const GeomVertexDataPipelineReader *data_reader) const { + GeomPrimitivePipelineReader reader(this, data_reader->get_current_thread()); + reader.check_minmax(); + return reader.check_valid(data_reader); } /** diff --git a/panda/src/gobj/geomPrimitive.h b/panda/src/gobj/geomPrimitive.h index b8012507b8..392fffe1cf 100644 --- a/panda/src/gobj/geomPrimitive.h +++ b/panda/src/gobj/geomPrimitive.h @@ -146,6 +146,7 @@ PUBLISHED: bool request_resident(Thread *current_thread = Thread::get_current_thread()) const; INLINE bool check_valid(const GeomVertexData *vertex_data) const; + INLINE bool check_valid(const GeomVertexDataPipelineReader *data_reader) const; virtual void output(std::ostream &out) const; virtual void write(std::ostream &out, int indent_level) const;