Merge pull request #5149 from vcmi/fix_visitablePos_calls

Fix visitable pos calls

include/vstd/CLoggerBase.h

@@ -58,6 +58,7 @@ public:
 
 	virtual void log(ELogLevel::ELogLevel level, const std::string & message) const = 0;
 	virtual void log(ELogLevel::ELogLevel level, const boost::format & fmt) const = 0;
+	virtual ELogLevel::ELogLevel getEffectiveLevel() const = 0;
 
 	/// Returns true if a debug/trace log message will be logged, false if not.
 	/// Useful if performance is important and concatenating the log message is a expensive task.
@@ -67,16 +68,19 @@ public:
 	template<typename T, typename ... Args>
 	void log(ELogLevel::ELogLevel level, const std::string & format, T t, Args ... args) const
 	{
-		try
+		if (getEffectiveLevel() <= level)
 		{
-			boost::format fmt(format);
-			makeFormat(fmt, t, args...);
-			log(level, fmt);
-		}
-		catch(...)
-		{
-			log(ELogLevel::ERROR, "Log formatting failed, format was:");
-			log(ELogLevel::ERROR, format);
+			try
+			{
+				boost::format fmt(format);
+				makeFormat(fmt, t, args...);
+				log(level, fmt);
+			}
+			catch(...)
+			{
+				log(ELogLevel::ERROR, "Log formatting failed, format was:");
+				log(ELogLevel::ERROR, format);
+			}
 		}
 	}
 

lib/logging/CLogger.cpp

@@ -146,13 +146,16 @@ void CLogger::log(ELogLevel::ELogLevel level, const std::string & message) const
 
 void CLogger::log(ELogLevel::ELogLevel level, const boost::format & fmt) const
 {
-	try
+	if (getEffectiveLevel() <= level)
 	{
-		log(level, fmt.str());
-	}
-	catch(...)
-	{
-		log(ELogLevel::ERROR, "Invalid log format!");
+		try
+		{
+			log(level, fmt.str());
+		}
+		catch (...)
+		{
+			log(ELogLevel::ERROR, "Invalid log format!");
+		}
 	}
 }
 

lib/logging/CLogger.h

@@ -46,7 +46,7 @@ private:
 
 /// The logger is used to log messages to certain targets of a specific domain/name.
 /// It is thread-safe and can be used concurrently by several threads.
-class DLL_LINKAGE CLogger: public vstd::CLoggerBase
+class DLL_LINKAGE CLogger final: public vstd::CLoggerBase
 {
 public:
 	ELogLevel::ELogLevel getLevel() const;
@@ -70,7 +70,7 @@ public:
 
 private:
 	explicit CLogger(const CLoggerDomain & domain);
-	inline ELogLevel::ELogLevel getEffectiveLevel() const; /// Returns the log level applied on this logger whether directly or indirectly.
+	inline ELogLevel::ELogLevel getEffectiveLevel() const override; /// Returns the log level applied on this logger whether directly or indirectly.
 	inline void callTargets(const LogRecord & record) const;
 
 	CLoggerDomain domain;

lib/rmg/RmgObject.cpp

@@ -255,6 +255,14 @@ Object::Instance & Object::addInstance(CGObjectInstance & object, const int3 & p
 	return dInstances.back();
 }
 
+bool Object::isVisitable() const
+{
+	return vstd::contains_if(dInstances, [](const Instance & instance)
+	{
+		return instance.object().isVisitable();
+	});
+}
+
 const int3 & Object::getPosition() const
 {
 	return dPosition;
@@ -464,6 +472,12 @@ uint32_t rmg::Object::getValue() const
 
 rmg::Area Object::Instance::getBorderAbove() const
 {
+	if (!object().isVisitable())
+	{
+		// TODO: Non-visitable objects don't need this, but theoretically could return valid area
+		return rmg::Area();
+	}
+
 	int3 visitablePos = getVisitablePosition();
 	auto areaVisitable = rmg::Area({visitablePos});
 	auto borderAbove = areaVisitable.getBorderOutside();

lib/rmg/RmgObject.h

@@ -85,6 +85,7 @@ public:
 	const Area getEntrableArea() const;
 	const Area & getBorderAbove() const;
 	
+	bool isVisitable() const;
 	const int3 & getPosition() const;
 	void setPosition(const int3 & position);
 	void setTemplate(const TerrainId & terrain, vstd::RNG &);

lib/rmg/modificators/ObjectManager.cpp

@@ -587,17 +587,22 @@ void ObjectManager::placeObject(rmg::Object & object, bool guarded, bool updateD
 		Zone::Lock lock(zone.areaMutex);
 
 		zone.areaPossible()->subtract(object.getArea());
-		bool keepVisitable = zone.freePaths()->contains(object.getVisitablePosition());
+		bool keepVisitable = object.isVisitable() && zone.freePaths()->contains(object.getVisitablePosition());
 		zone.freePaths()->subtract(object.getArea()); //just to avoid areas overlapping
-		if(keepVisitable)
-			zone.freePaths()->add(object.getVisitablePosition());
 		zone.areaUsed()->unite(object.getArea());
-		zone.areaUsed()->erase(object.getVisitablePosition());
+		if (keepVisitable)
+		{
+			zone.freePaths()->add(object.getVisitablePosition());
+			zone.areaUsed()->erase(object.getVisitablePosition());
+		}
 
 		if(guarded) //We assume the monster won't be guarded
 		{
 			auto guardedArea = object.instances().back()->getAccessibleArea();
-			guardedArea.add(object.instances().back()->getVisitablePosition());
+			if (object.isVisitable())
+			{
+				guardedArea.add(object.instances().back()->getVisitablePosition());
+			}
 			auto areaToBlock = object.getAccessibleArea(true);
 			areaToBlock.subtract(guardedArea);
 			zone.areaPossible()->subtract(areaToBlock);
@@ -641,7 +646,10 @@ void ObjectManager::placeObject(rmg::Object & object, bool guarded, bool updateD
 	// TODO: Add multiple tiles in one operation to avoid multiple invalidation
 	for(auto * instance : object.instances())
 	{
-		objectsVisitableArea.add(instance->getVisitablePosition());
+		if (instance->object().isVisitable())
+		{
+			objectsVisitableArea.add(instance->getVisitablePosition());
+		}
 		objects.push_back(&instance->object());
 		if(auto * rp = zone.getModificator<RoadPlacer>())
 		{

test/mock/mock_vstd_CLoggerBase.h

@@ -39,5 +39,6 @@ public:
 
 	bool isDebugEnabled() const override {return true;}
 	bool isTraceEnabled() const override {return true;}
+	ELogLevel::ELogLevel getEffectiveLevel() const override {return ELogLevel::TRACE;}
 };