From b2e149a737496a488adabba558391a5c46b076b3 Mon Sep 17 00:00:00 2001 From: James Turner Date: Wed, 2 Sep 2020 19:52:14 +0100 Subject: [PATCH] Refactor SGBuildingBin Trying to trace a crash that was reported, but along the way fix a leak of BuildingBins. Also avoid allocating some std::strings on the heap. --- simgear/scene/tgdb/ReaderWriterSTG.cxx | 30 ++++++--- simgear/scene/tgdb/SGBuildingBin.cxx | 93 +++++++++++++------------- simgear/scene/tgdb/SGBuildingBin.hxx | 21 +++--- 3 files changed, 79 insertions(+), 65 deletions(-) diff --git a/simgear/scene/tgdb/ReaderWriterSTG.cxx b/simgear/scene/tgdb/ReaderWriterSTG.cxx index 9aff6737..88c8b27a 100644 --- a/simgear/scene/tgdb/ReaderWriterSTG.cxx +++ b/simgear/scene/tgdb/ReaderWriterSTG.cxx @@ -221,29 +221,41 @@ struct ReaderWriterSTG::_ModelBin { if (signBuilder.getSignsGroup()) group->addChild(signBuilder.getSignsGroup()); - if (_buildingList.size() > 0) { + if (!_buildingList.empty()) { SGMaterialLibPtr matlib = _options->getMaterialLib(); bool useVBOs = (_options->getPluginStringData("SimGear::USE_VBOS") == "ON"); if (!matlib) { SG_LOG( SG_TERRAIN, SG_ALERT, "Unable to get materials definition for buildings"); } else { - for (std::list<_BuildingList>::iterator i = _buildingList.begin(); i != _buildingList.end(); ++i) { + for (const auto& b : _buildingList) { // Build buildings for each list of buildings - SGGeod geodPos = SGGeod::fromDegM(i->_lon, i->_lat, 0.0); - SGMaterial* mat = matlib->find(i->_material_name, geodPos); - SGPath path = SGPath(i->_filename); + SGGeod geodPos = SGGeod::fromDegM(b._lon, b._lat, 0.0); + SGSharedPtr mat = matlib->find(b._material_name, geodPos); + + // trying to avoid crash on null material, see: + // https://sentry.io/organizations/flightgear/issues/1867075869 + if (!mat) { + SG_LOG(SG_TERRAIN, SG_ALERT, "Building specifies unknown material: " << b._material_name); + continue; + } + + const auto path = SGPath(b._filename); SGBuildingBin* buildingBin = new SGBuildingBin(path, mat, useVBOs); - SGBuildingBinList buildingBinList; - buildingBinList.push_back(buildingBin); + SGBuildingBinList bbList; + bbList.push_back(buildingBin); osg::MatrixTransform* matrixTransform; - matrixTransform = new osg::MatrixTransform(makeZUpFrame(SGGeod::fromDegM(i->_lon, i->_lat, i->_elev))); + matrixTransform = new osg::MatrixTransform(makeZUpFrame(SGGeod::fromDegM(b._lon, b._lat, b._elev))); matrixTransform->setName("rotateBuildings"); matrixTransform->setDataVariance(osg::Object::STATIC); - matrixTransform->addChild(createRandomBuildings(buildingBinList, osg::Matrix::identity(), _options)); + matrixTransform->addChild(createRandomBuildings(bbList, osg::Matrix::identity(), _options)); group->addChild(matrixTransform); + + std::for_each(bbList.begin(), bbList.end(), [](SGBuildingBin* bb) { + delete bb; + }); } } } diff --git a/simgear/scene/tgdb/SGBuildingBin.cxx b/simgear/scene/tgdb/SGBuildingBin.cxx index 07955f29..386db9db 100644 --- a/simgear/scene/tgdb/SGBuildingBin.cxx +++ b/simgear/scene/tgdb/SGBuildingBin.cxx @@ -497,14 +497,20 @@ typedef QuadTreeBuilderget_names()[0]); - SG_LOG(SG_TERRAIN, SG_DEBUG, "Building material " << material_name); - material = mat; - texture = new std::string(mat->get_building_texture()); - lightMap = new std::string(mat->get_building_lightmap()); - buildingRange = mat->get_building_range(); - SG_LOG(SG_TERRAIN, SG_DEBUG, "Building texture " << texture); + SGBuildingBin::SGBuildingBin(const SGMaterial* mat, bool useVBOs) : _material(const_cast(mat)) + { + const auto& materialNames = mat->get_names(); + if (materialNames.empty()) { + SG_LOG(SG_TERRAIN, SG_DEV_ALERT, "SGBuildingBin: material has zero names defined"); + } else { + _materialName = materialNames.front(); + SG_LOG(SG_TERRAIN, SG_DEBUG, "Building material " << _materialName); + } + + _textureName = mat->get_building_texture(); + _lightMapName = mat->get_building_lightmap(); + buildingRange = mat->get_building_range(); + SG_LOG(SG_TERRAIN, SG_DEBUG, "Building texture " << _textureName); } SGBuildingBin::~SGBuildingBin() { @@ -652,15 +658,15 @@ typedef QuadTreeBuilderget_building_small_min_width() + mt_rand(&seed) * mt_rand(&seed) * (material->get_building_small_max_width() - material->get_building_small_min_width()); - depth = material->get_building_small_min_depth() + mt_rand(&seed) * mt_rand(&seed) * (material->get_building_small_max_depth() - material->get_building_small_min_depth()); - floors = SGMisc::round(material->get_building_small_min_floors() + mt_rand(&seed) * (material->get_building_small_max_floors() - material->get_building_small_min_floors())); + width = _material->get_building_small_min_width() + mt_rand(&seed) * mt_rand(&seed) * (_material->get_building_small_max_width() - _material->get_building_small_min_width()); + depth = _material->get_building_small_min_depth() + mt_rand(&seed) * mt_rand(&seed) * (_material->get_building_small_max_depth() - _material->get_building_small_min_depth()); + floors = SGMisc::round(_material->get_building_small_min_floors() + mt_rand(&seed) * (_material->get_building_small_max_floors() - _material->get_building_small_min_floors())); height = floors * (2.8 + mt_rand(&seed)); // Small buildings are never deeper than they are wide. if (depth > width) { depth = width; } - pitch_height = (mt_rand(&seed) < material->get_building_small_pitch()) ? 3.0 : 0.0; + pitch_height = (mt_rand(&seed) < _material->get_building_small_pitch()) ? 3.0 : 0.0; if (pitch_height == 0.0) { roof_shape = 0; @@ -671,18 +677,18 @@ typedef QuadTreeBuilderget_building_medium_min_width() + mt_rand(&seed) * mt_rand(&seed) * (material->get_building_medium_max_width() - material->get_building_medium_min_width()); - depth = material->get_building_medium_min_depth() + mt_rand(&seed) * mt_rand(&seed) * (material->get_building_medium_max_depth() - material->get_building_medium_min_depth()); - floors = SGMisc::round(material->get_building_medium_min_floors() + mt_rand(&seed) * (material->get_building_medium_max_floors() - material->get_building_medium_min_floors())); + width = _material->get_building_medium_min_width() + mt_rand(&seed) * mt_rand(&seed) * (_material->get_building_medium_max_width() - _material->get_building_medium_min_width()); + depth = _material->get_building_medium_min_depth() + mt_rand(&seed) * mt_rand(&seed) * (_material->get_building_medium_max_depth() - _material->get_building_medium_min_depth()); + floors = SGMisc::round(_material->get_building_medium_min_floors() + mt_rand(&seed) * (_material->get_building_medium_max_floors() - _material->get_building_medium_min_floors())); height = floors * (2.8 + mt_rand(&seed)); - while ((height > width) && (floors > material->get_building_medium_min_floors())) { - // Ensure that medium buildings aren't taller than they are wide - floors--; - height = floors * (2.8 + mt_rand(&seed)); + while ((height > width) && (floors > _material->get_building_medium_min_floors())) { + // Ensure that medium buildings aren't taller than they are wide + floors--; + height = floors * (2.8 + mt_rand(&seed)); } - pitch_height = (mt_rand(&seed) < material->get_building_medium_pitch()) ? 3.0 : 0.0; + pitch_height = (mt_rand(&seed) < _material->get_building_medium_pitch()) ? 3.0 : 0.0; if (pitch_height == 0.0) { roof_shape = 0; @@ -693,11 +699,11 @@ typedef QuadTreeBuilderget_building_large_min_width() + mt_rand(&seed) * (material->get_building_large_max_width() - material->get_building_large_min_width()); - depth = material->get_building_large_min_depth() + mt_rand(&seed) * (material->get_building_large_max_depth() - material->get_building_large_min_depth()); - floors = SGMisc::round(material->get_building_large_min_floors() + mt_rand(&seed) * (material->get_building_large_max_floors() - material->get_building_large_min_floors())); + width = _material->get_building_large_min_width() + mt_rand(&seed) * (_material->get_building_large_max_width() - _material->get_building_large_min_width()); + depth = _material->get_building_large_min_depth() + mt_rand(&seed) * (_material->get_building_large_max_depth() - _material->get_building_large_min_depth()); + floors = SGMisc::round(_material->get_building_large_min_floors() + mt_rand(&seed) * (_material->get_building_large_max_floors() - _material->get_building_large_min_floors())); height = floors * (2.8 + mt_rand(&seed)); - pitch_height = (mt_rand(&seed) < material->get_building_large_pitch()) ? 3.0 : 0.0; + pitch_height = (mt_rand(&seed) < _material->get_building_large_pitch()) ? 3.0 : 0.0; if (pitch_height == 0.0) { roof_shape = 0; @@ -726,36 +732,35 @@ typedef QuadTreeBuilderget_building_small_fraction()) { + return SGBuildingBin::SMALL; + } - if (roll < material->get_building_small_fraction()) { - return SGBuildingBin::SMALL; - } - - if (roll < (material->get_building_small_fraction() + material->get_building_medium_fraction())) { - return SGBuildingBin::MEDIUM; - } + if (roll < (_material->get_building_small_fraction() + _material->get_building_medium_fraction())) { + return SGBuildingBin::MEDIUM; + } return SGBuildingBin::LARGE; } float SGBuildingBin::getBuildingMaxRadius(BuildingType type) { - if (type == SGBuildingBin::SMALL) return material->get_building_small_max_width(); - if (type == SGBuildingBin::MEDIUM) return material->get_building_medium_max_width(); - if (type == SGBuildingBin::LARGE) return material->get_building_large_max_width(); - return 0; + if (type == SGBuildingBin::SMALL) return _material->get_building_small_max_width(); + if (type == SGBuildingBin::MEDIUM) return _material->get_building_medium_max_width(); + if (type == SGBuildingBin::LARGE) return _material->get_building_large_max_width(); + return 0; } float SGBuildingBin::getBuildingMaxDepth(BuildingType type) { - if (type == SGBuildingBin::SMALL) return material->get_building_small_max_depth(); - if (type == SGBuildingBin::MEDIUM) return material->get_building_medium_max_depth(); - if (type == SGBuildingBin::LARGE) return material->get_building_large_max_depth(); - return 0; + if (type == SGBuildingBin::SMALL) return _material->get_building_small_max_depth(); + if (type == SGBuildingBin::MEDIUM) return _material->get_building_medium_max_depth(); + if (type == SGBuildingBin::LARGE) return _material->get_building_large_max_depth(); + return 0; } ref_ptr SGBuildingBin::createBuildingsGroup(Matrix transInv, const SGReaderWriterOptions* options) { ref_ptr effect; - EffectMap::iterator iter = buildingEffectMap.find(*texture); + auto iter = buildingEffectMap.find(_textureName); if ((iter == buildingEffectMap.end())|| (!iter->second.lock(effect))) @@ -764,16 +769,14 @@ typedef QuadTreeBuildersetStringValue("Effects/building"); SGPropertyNode* params = makeChild(effectProp, "parameters"); // Main texture - n=0 - params->getChild("texture", 0, true)->getChild("image", 0, true) - ->setStringValue(*texture); + params->getChild("texture", 0, true)->getChild("image", 0, true)->setStringValue(_textureName); // Light map - n=3 - params->getChild("texture", 3, true)->getChild("image", 0, true) - ->setStringValue(*lightMap); + params->getChild("texture", 3, true)->getChild("image", 0, true)->setStringValue(_lightMapName); effect = makeEffect(effectProp, true, options); if (iter == buildingEffectMap.end()) - buildingEffectMap.insert(EffectMap::value_type(*texture, effect)); + buildingEffectMap.insert(EffectMap::value_type(_textureName, effect)); else iter->second = effect; // update existing, but empty observer } diff --git a/simgear/scene/tgdb/SGBuildingBin.hxx b/simgear/scene/tgdb/SGBuildingBin.hxx index d19dbc1d..27c24c64 100644 --- a/simgear/scene/tgdb/SGBuildingBin.hxx +++ b/simgear/scene/tgdb/SGBuildingBin.hxx @@ -145,20 +145,19 @@ public: }; private: + const SGSharedPtr _material; - const SGMaterial *material; + std::string _materialName; + std::string _textureName; + std::string _lightMapName; - std::string* material_name; - std::string* texture; - std::string* lightMap; - - // Visibility range for buildings - float buildingRange; + // Visibility range for buildings + float buildingRange; - // Information for an instance of a building - position and orientation - typedef std::vector BuildingInstanceList; - BuildingInstanceList buildingLocations; + // Information for an instance of a building - position and orientation + typedef std::vector BuildingInstanceList; + BuildingInstanceList buildingLocations; public: @@ -187,7 +186,7 @@ public: bool checkMinDist (SGVec3f p, float radius); - std::string* getMaterialName() { return material_name; } + const std::string& getMaterialName() const { return _materialName; } BuildingType getBuildingType(float roll); float getBuildingMaxRadius(BuildingType);