- fbx: improved DOM object construction and dependency resolution.

code/FBXAnimation.cpp

@@ -100,12 +100,12 @@ AnimationCurveNode::AnimationCurveNode(uint64_t id, const Element& element, cons
 {
 	{
 	// resolve attached animation curves
-	const std::vector<const Connection*>& conns = doc.GetConnectionsByDestinationSequenced(ID());
+	const std::vector<const Connection*>& conns = doc.GetConnectionsByDestinationSequenced(ID(),"AnimationCurve");
 
 	BOOST_FOREACH(const Connection* con, conns) {
 
 		// link should go for a property
-		if (con->PropertyName().length()) {
+		if (!con->PropertyName().length()) {
 			continue;
 		}
 
@@ -127,26 +127,16 @@ AnimationCurveNode::AnimationCurveNode(uint64_t id, const Element& element, cons
 	}{
 
 	// find target node
-	const std::vector<const Connection*>& conns = doc.GetConnectionsBySourceSequenced(ID());
+	const std::vector<const Connection*>& conns = doc.GetConnectionsBySourceSequenced(ID(),"Model");
 
 	BOOST_FOREACH(const Connection* con, conns) {
 
 		// link should go for a property
-		if (con->PropertyName().length()) {
-			continue;
-		}
-
-		// note: the implicit rule in all DOM classes is to always resolve
-		// from destination to source (since the FBX object hierarchy is,
-		// with very few exceptions, a DAG, this avoids cycles). Since
-		// it goes the other way round, we have to cope with the case
-		// that the destination is not fully constructed yet.
-		LazyObject& lob = con->LazyDestinationObject();
-		if(lob.IsBeingConstructed()) {
+		if (!con->PropertyName().length()) {
 			continue;
 		}
 
-		const Object* ob = lob.Get();
+		const Object* ob = con->DestinationObject();
 		if(!ob) {
 			DOMWarning("failed to read destination object for AnimationCurveNode->Model link, ignoring",&element);
 			continue;
@@ -182,7 +172,7 @@ AnimationLayer::AnimationLayer(uint64_t id, const Element& element, const std::s
 	props = GetPropertyTable(doc,"AnimationLayer.FbxAnimLayer",element,sc);
 
 	// resolve attached animation nodes
-	const std::vector<const Connection*>& conns = doc.GetConnectionsByDestinationSequenced(ID());
+	const std::vector<const Connection*>& conns = doc.GetConnectionsByDestinationSequenced(ID(),"AnimationCurveNode");
 	nodes.reserve(conns.size());
 
 	BOOST_FOREACH(const Connection* con, conns) {
@@ -222,7 +212,7 @@ AnimationStack::AnimationStack(uint64_t id, const Element& element, const std::s
 	props = GetPropertyTable(doc,"AnimationStack.FbxAnimStack",element,sc);
 
 	// resolve attached animation layers
-	const std::vector<const Connection*>& conns = doc.GetConnectionsByDestinationSequenced(ID());
+	const std::vector<const Connection*>& conns = doc.GetConnectionsByDestinationSequenced(ID(),"AnimationLayer");
 	layers.reserve(conns.size());
 
 	BOOST_FOREACH(const Connection* con, conns) {

code/FBXConverter.cpp

@@ -102,6 +102,7 @@ public:
 	{
 		std::for_each(meshes.begin(),meshes.end(),Util::delete_fun<aiMesh>());
 		std::for_each(materials.begin(),materials.end(),Util::delete_fun<aiMaterial>());
+		std::for_each(animations.begin(),animations.end(),Util::delete_fun<aiAnimation>());
 	}
 
 
@@ -123,7 +124,7 @@ private:
 	// collect and assign child nodes
 	void ConvertNodes(uint64_t id, aiNode& parent)
 	{
-		const std::vector<const Connection*>& conns = doc.GetConnectionsByDestinationSequenced(id);
+		const std::vector<const Connection*>& conns = doc.GetConnectionsByDestinationSequenced(id, "Model");
 
 		std::vector<aiNode*> nodes;
 		nodes.reserve(conns.size());
@@ -983,6 +984,7 @@ private:
 		}
 		catch(std::exception&) {
 			std::for_each(node_anims.begin(), node_anims.end(), Util::delete_fun<aiNodeAnim>());
+			throw;
 		}
 
 		if(node_anims.size()) {
@@ -1040,6 +1042,8 @@ private:
 	// ------------------------------------------------------------------------------------------------
 	std::vector<float> GetKeyTimeList(const KeyFrameListList& inputs)
 	{
+		ai_assert(inputs.size());
+
 		// reserve some space upfront - it is likely that the keyframe lists
 		// have matching time values, so max(of all keyframe lists) should 
 		// be a good estimate.

code/FBXDocument.cpp

@@ -390,7 +390,7 @@ LazyObject::LazyObject(uint64_t id, const Element& element, const Document& doc)
 : doc(doc)
 , element(element)
 , id(id)
-, being_constructed()
+, flags()
 {
 
 }
@@ -402,9 +402,9 @@ LazyObject::~LazyObject()
 }
 
 // ------------------------------------------------------------------------------------------------
-const Object* LazyObject::Get()
+const Object* LazyObject::Get(bool dieOnError)
 {
-	if(being_constructed) {
+	if(IsBeingConstructed() || FailedToConstruct()) {
 		return NULL;
 	}
 
@@ -431,44 +431,61 @@ const Object* LazyObject::Get()
 	} 
 
 	// prevent recursive calls
-	being_constructed = true;
-
-	// this needs to be relatively fast since it happens a lot,
-	// so avoid constructing strings all the time.
-	const char* obtype = key.begin();
-	const size_t length = static_cast<size_t>(key.end()-key.begin());
-	if (!strncmp(obtype,"Geometry",length)) {
-		if (!strcmp(classtag.c_str(),"Mesh")) {
-			object.reset(new MeshGeometry(id,element,name,doc));
+	flags |= BEING_CONSTRUCTED;
+
+	try {
+		// this needs to be relatively fast since it happens a lot,
+		// so avoid constructing strings all the time.
+		const char* obtype = key.begin();
+		const size_t length = static_cast<size_t>(key.end()-key.begin());
+		if (!strncmp(obtype,"Geometry",length)) {
+			if (!strcmp(classtag.c_str(),"Mesh")) {
+				object.reset(new MeshGeometry(id,element,name,doc));
+			}
+		}
+		else if (!strncmp(obtype,"Model",length)) {
+			object.reset(new Model(id,element,doc,name));
+		}
+		else if (!strncmp(obtype,"Material",length)) {
+			object.reset(new Material(id,element,doc,name));
+		}
+		else if (!strncmp(obtype,"Texture",length)) {
+			object.reset(new Texture(id,element,doc,name));
+		}
+		else if (!strncmp(obtype,"AnimationStack",length)) {
+			object.reset(new AnimationStack(id,element,name,doc));
+		}
+		else if (!strncmp(obtype,"AnimationLayer",length)) {
+			object.reset(new AnimationLayer(id,element,name,doc));
+		}
+		// note: order matters for these two
+		else if (!strncmp(obtype,"AnimationCurveNode",length)) {
+			object.reset(new AnimationCurveNode(id,element,name,doc));
+		}	
+		else if (!strncmp(obtype,"AnimationCurve",length)) {
+			object.reset(new AnimationCurve(id,element,name,doc));
 		}
 	}
-	else if (!strncmp(obtype,"Model",length)) {
-		object.reset(new Model(id,element,doc,name));
-	}
-	else if (!strncmp(obtype,"Material",length)) {
-		object.reset(new Material(id,element,doc,name));
-	}
-	else if (!strncmp(obtype,"Texture",length)) {
-		object.reset(new Texture(id,element,doc,name));
-	}
-	else if (!strncmp(obtype,"AnimationStack",length)) {
-		object.reset(new AnimationStack(id,element,name,doc));
-	}
-	else if (!strncmp(obtype,"AnimationLayer",length)) {
-		object.reset(new AnimationLayer(id,element,name,doc));
-	}
-	else if (!strncmp(obtype,"AnimationCurveNode",length)) {
-		object.reset(new AnimationCurveNode(id,element,name,doc));
-	}
-	else if (!strncmp(obtype,"AnimationCurve",length)) {
-		object.reset(new AnimationCurve(id,element,name,doc));
+	catch(std::exception& ex) {
+		flags &= ~BEING_CONSTRUCTED;
+		flags |= FAILED_TO_CONSTRUCT;
+
+		//if(dieOnError) {
+			throw;
+		//}
+
+		// note: the error message is already formatted, so raw logging is ok
+		if(!DefaultLogger::isNullLogger()) {
+			DefaultLogger::get()->error(ex.what());
+		}
+		return NULL;
 	}
 
 	if (!object.get()) {
 		//DOMError("failed to convert element to DOM object, class: " + classtag + ", name: " + name,&element);
 	}
 
-	being_constructed = false;
+	flags &= ~BEING_CONSTRUCTED;
 	return object.get();
 }
 
@@ -756,7 +773,7 @@ std::vector<const Connection*> Document::GetConnectionsSequenced(uint64_t id, co
 
 
 // ------------------------------------------------------------------------------------------------
-std::vector<const Connection*> Document::GetConnectionsSequenced(uint64_t id, const ConnectionMap& conns, const char* const* classnames, size_t count) const
+std::vector<const Connection*> Document::GetConnectionsSequenced(uint64_t id, bool is_src, const ConnectionMap& conns, const char* const* classnames, size_t count) const
 {
 	ai_assert(classnames);
 	ai_assert(count != 0 && count <= MAX_CLASSNAMES);
@@ -775,7 +792,11 @@ std::vector<const Connection*> Document::GetConnectionsSequenced(uint64_t id, co
 
 	temp.reserve(std::distance(range.first,range.second));
 	for (ConnectionMap::const_iterator it = range.first; it != range.second; ++it) {
-		const Token& key = (*it).second->LazyDestinationObject().GetElement().KeyToken();
+		const Token& key = (is_src 
+			? (*it).second->LazyDestinationObject()
+			: (*it).second->LazySourceObject()
+		).GetElement().KeyToken();
+
 		const char* obtype = key.begin();
 
 		for (size_t i = 0; i < c; ++i) {
@@ -805,10 +826,28 @@ std::vector<const Connection*> Document::GetConnectionsBySourceSequenced(uint64_
 }
 
 
+
+// ------------------------------------------------------------------------------------------------
+std::vector<const Connection*> Document::GetConnectionsBySourceSequenced(uint64_t dest, const char* classname) const
+{
+	const char* arr[] = {classname};
+	return GetConnectionsBySourceSequenced(dest, arr,1);
+}
+
+
+
 // ------------------------------------------------------------------------------------------------
 std::vector<const Connection*> Document::GetConnectionsBySourceSequenced(uint64_t source, const char* const* classnames, size_t count) const
 {
-	return GetConnectionsSequenced(source, ConnectionsBySource(),classnames, count);
+	return GetConnectionsSequenced(source, true, ConnectionsBySource(),classnames, count);
+}
+
+
+// ------------------------------------------------------------------------------------------------
+std::vector<const Connection*> Document::GetConnectionsByDestinationSequenced(uint64_t dest, const char* classname) const
+{
+	const char* arr[] = {classname};
+	return GetConnectionsByDestinationSequenced(dest, arr,1);
 }
 
 
@@ -822,7 +861,7 @@ std::vector<const Connection*> Document::GetConnectionsByDestinationSequenced(ui
 // ------------------------------------------------------------------------------------------------
 std::vector<const Connection*> Document::GetConnectionsByDestinationSequenced(uint64_t dest, const char* const* classnames, size_t count) const
 {
-	return GetConnectionsSequenced(dest, ConnectionsByDestination(),classnames, count);
+	return GetConnectionsSequenced(dest, false, ConnectionsByDestination(),classnames, count);
 }
 
 

code/FBXDocument.h

@@ -73,11 +73,11 @@ public:
 
 public:
 
-	const Object* Get();
+	const Object* Get(bool dieOnError = false);
 
 	template <typename T> 
-	const T* Get() {
-		const Object* const ob = Get();
+	const T* Get(bool dieOnError = false) {
+		const Object* const ob = Get(dieOnError);
 		return ob ? dynamic_cast<const T*>(ob) : NULL;
 	}
 
@@ -86,7 +86,11 @@ public:
 	}
 
 	bool IsBeingConstructed() const {
-		return being_constructed;
+		return (flags & BEING_CONSTRUCTED) != 0;
+	}
+
+	bool FailedToConstruct() const {
+		return (flags & FAILED_TO_CONSTRUCT) != 0;
 	}
 
 	const Element& GetElement() const {
@@ -104,7 +108,13 @@ private:
 	boost::scoped_ptr<const Object> object;
 
 	const uint64_t id;
-	bool being_constructed;
+
+	enum Flags {
+		BEING_CONSTRUCTED = 0x1,
+		FAILED_TO_CONSTRUCT = 0x2
+	};
+
+	unsigned int flags;
 };
 
 
@@ -674,9 +684,18 @@ public:
 		return dest_connections;
 	}
 
+	// note: the implicit rule in all DOM classes is to always resolve
+	// from destination to source (since the FBX object hierarchy is,
+	// with very few exceptions, a DAG, this avoids cycles). In all
+	// cases that may involve back-facing edges in the object graph,
+	// use LazyObject::IsBeingConstructed() to check.
+
 	std::vector<const Connection*> GetConnectionsBySourceSequenced(uint64_t source) const;
 	std::vector<const Connection*> GetConnectionsByDestinationSequenced(uint64_t dest) const;
 
+	std::vector<const Connection*> GetConnectionsBySourceSequenced(uint64_t source, const char* classname) const;
+	std::vector<const Connection*> GetConnectionsByDestinationSequenced(uint64_t dest, const char* classname) const;
+
 	std::vector<const Connection*> GetConnectionsBySourceSequenced(uint64_t source, const char* const* classnames, size_t count) const;
 	std::vector<const Connection*> GetConnectionsByDestinationSequenced(uint64_t dest, const char* const* classnames, size_t count) const;
 
@@ -685,7 +704,7 @@ public:
 private:
 
 	std::vector<const Connection*> GetConnectionsSequenced(uint64_t id, const ConnectionMap&) const;
-	std::vector<const Connection*> GetConnectionsSequenced(uint64_t id, const ConnectionMap&, const char* const* classnames, size_t count) const;
+	std::vector<const Connection*> GetConnectionsSequenced(uint64_t id, bool is_src, const ConnectionMap&, const char* const* classnames, size_t count) const;
 
 private:
 

code/FBXModel.cpp

@@ -89,8 +89,10 @@ Model::~Model()
 // ------------------------------------------------------------------------------------------------
 void Model::ResolveLinks(const Element& element, const Document& doc)
 {
+	const char* const arr[] = {"Geometry","Material"};
+
 	// resolve material
-	const std::vector<const Connection*>& conns = doc.GetConnectionsByDestinationSequenced(ID());
+	const std::vector<const Connection*>& conns = doc.GetConnectionsByDestinationSequenced(ID(),arr, 2);
 
 	materials.reserve(conns.size());
 	geometry.reserve(conns.size());