Attach entities at correct position

Attached light entities are now correctly parented to the main entity, and
their attachment offset is encoded into the attached entity's localToParent()
matrix. Some changes were required to Light which has previously always assumed
that its "origin" key IS the light position - we now transform the local origin
by localToWorld() which takes into account both the "origin" key and any
transformation applied to the light entity or its parent.

Attached lights are now appearing in the correct position and tests pass, but
the attached light does not move when the parent entity is dragged.

include/inode.h

@@ -78,10 +78,12 @@ class NodeVisitor
 	virtual void post(const INodePtr& node) {}
 };
 
-/** 
- * greebo: Abstract definition of a Node, a basic element
- * of the scenegraph. All nodes share a certain set of 
- * functionality, like Layer functionality or being a Renderable.
+/**
+ * \brief Main interface for a Node, a basic element of the scenegraph.
+ *
+ * All nodes share a certain set of functionality, such as being placed in
+ * layers, being able to render themselves, and being able to hold and
+ * transform a list of child nodes.
  */
 class INode :
 	public Layered,
@@ -116,7 +118,7 @@ class INode :
 	 * Set the scenegraph this node is belonging to. This is usually
 	 * set by the scenegraph itself during insertion.
 	 */
-	virtual void setSceneGraph(const GraphPtr& sceneGraph) = 0; 
+	virtual void setSceneGraph(const GraphPtr& sceneGraph) = 0;
 
 	/** greebo: Returns true, if the node is the root element
 	 * 			of the scenegraph.
@@ -166,7 +168,7 @@ class INode :
 	virtual bool hasChildNodes() const = 0;
 
 	/**
-	 * greebo: Traverses this node and all child nodes (recursively) 
+	 * greebo: Traverses this node and all child nodes (recursively)
 	 * using the given visitor.
 	 *
 	 * Note: replaces the legacy Node_traverseSubgraph() method.
@@ -187,11 +189,11 @@ class INode :
 
 	/**
 	 * Call the given functor for each child node, depth first
-	 * This is a simpler alternative to the usual traverse() method 
+	 * This is a simpler alternative to the usual traverse() method
 	 * which provides pre() and post() methods and more control about
-	 * which nodes to traverse and. This forEachNode() routine simply 
+	 * which nodes to traverse and. This forEachNode() routine simply
 	 * hits every child node including their children.
-	 * 
+	 *
 	 * @returns: true if the functor returned false on any of the
 	 * visited nodes. The return type is used to pass the stop signal
 	 * up the stack during traversal.
@@ -236,15 +238,21 @@ class INode :
 	// Returns the bounds in world coordinates
 	virtual const AABB& worldAABB() const = 0;
 
-	// Returns the transformation from local to world coordinates
+	/**
+     * \brief Return the transformation from local to world coordinates
+     *
+     * This represents the final transformation from this node's own coordinate
+     * space into world space, including any transformations inherited from
+     * parent nodes.
+     */
 	virtual const Matrix4& localToWorld() const = 0;
 
 	// Undo/Redo events - some nodes need to do extra legwork after undo or redo
 	// This is called by the TraversableNodeSet after a undo/redo operation
 	// not by the UndoSystem itself, at least not yet.
 	virtual void onPostUndo() {}
 	virtual void onPostRedo() {}
-    
+
     // Called during recursive transform changed, but only by INodes themselves
     virtual void transformChangedLocal() = 0;
 };

radiantcore/entity/EntityNode.cpp

@@ -23,7 +23,6 @@ EntityNode::EntityNode(const IEntityClassPtr& eclass) :
 	_shaderParms(_keyObservers, _colourKey),
 	_direction(1,0,0)
 {
-    createAttachedEntities();
 }
 
 EntityNode::EntityNode(const EntityNode& other) :
@@ -44,7 +43,6 @@ EntityNode::EntityNode(const EntityNode& other) :
 	_shaderParms(_keyObservers, _colourKey),
 	_direction(1,0,0)
 {
-    createAttachedEntities();
 }
 
 EntityNode::~EntityNode()
@@ -72,6 +70,9 @@ void EntityNode::construct()
 	addKeyObserver("skin", _skinKeyObserver);
 
 	_shaderParms.addKeyObservers();
+
+    // Construct all attached entities
+    createAttachedEntities();
 }
 
 void EntityNode::constructClone(const EntityNode& original)
@@ -121,6 +122,7 @@ void EntityNode::createAttachedEntities()
     _spawnArgs.forEachAttachment(
         [this](const Entity::Attachment& a)
         {
+            // Check this is a valid entity class
             auto cls = GlobalEntityClassManager().findClass(a.eclass);
             if (!cls)
             {
@@ -130,7 +132,19 @@ void EntityNode::createAttachedEntities()
                 return;
             }
 
-            _attachedEnts.push_back(GlobalEntityModule().createEntity(cls));
+            // Construct and store the attached entity
+            auto attachedEnt = GlobalEntityModule().createEntity(cls);
+            assert(attachedEnt);
+            _attachedEnts.push_back(attachedEnt);
+
+            // Set ourselves as the parent of the attached entity (for
+            // localToParent transforms)
+            attachedEnt->setParent(shared_from_this());
+
+            // Set the attached entity's transform matrix according to the
+            // required offset
+            MatrixTransform& mt = dynamic_cast<MatrixTransform&>(*attachedEnt);
+            mt.localToParent() = Matrix4::getTranslation(a.offset);
         }
     );
 }

radiantcore/entity/light/Light.cpp

@@ -616,7 +616,11 @@ AABB Light::lightAABB() const
     }
     else
     {
-        return AABB(_originTransformed, m_doom3Radius.m_radiusTransformed);
+        // AABB ignores light_center so we can't call getLightOrigin() here.
+        // Just transform (0, 0, 0) by localToWorld to get the world origin for
+        // the AABB.
+        return AABB(_owner.localToWorld().transformPoint(Vector3(0, 0, 0)),
+                    m_doom3Radius.m_radiusTransformed);
     }
 }
 
@@ -637,8 +641,13 @@ Vector3 Light::getLightOrigin() const
     }
     else
     {
-        // AABB origin + light_center, i.e. center in world space
-        return _originTransformed + m_doom3Radius.m_centerTransformed;
+        // Since localToWorld() takes into account our own origin as well as the
+        // transformation of any parent entity, just transform a null origin +
+        // light_center by the localToWorld matrix to get the light origin in
+        // world space.
+        return _owner.localToWorld().transformPoint(
+            /* (0, 0, 0) + */ m_doom3Radius.m_centerTransformed
+        );
     }
 }
 

test/Entity.cpp

@@ -9,6 +9,7 @@
 
 #include "render/NopVolumeTest.h"
 #include "string/convert.h"
+#include "transformlib.h"
 
 namespace test
 {
@@ -380,6 +381,65 @@ TEST_F(EntityTest, ModifyEntityClass)
     EXPECT_NE(light->getWireShader(), origWireShader);
 }
 
+TEST_F(EntityTest, LightLocalToWorldFromOrigin)
+{
+    auto light = createByClassName("light");
+
+    // Initial localToWorld should be identity
+    EXPECT_EQ(light->localToWorld(), Matrix4::getIdentity());
+
+    // Set an origin
+    const Vector3 ORIGIN(123, 456, -10);
+    light->getEntity().setKeyValue("origin", string::to_string(ORIGIN));
+
+    // localToParent should reflect the new origin
+    auto transformNode = std::dynamic_pointer_cast<ITransformNode>(light);
+    ASSERT_TRUE(transformNode);
+    EXPECT_EQ(transformNode->localToParent(), Matrix4::getTranslation(ORIGIN));
+
+    // Since there is no parent, the final localToWorld should be the same as
+    // localToParent
+    EXPECT_EQ(light->localToWorld(), Matrix4::getTranslation(ORIGIN));
+}
+
+TEST_F(EntityTest, LightTransformedByParent)
+{
+    // Parent a light to another entity (this isn't currently how the attachment
+    // system is implemented, but it should validate that a light node can
+    // inherit the transformation of its parent).
+    auto light = createByClassName("light");
+    auto parentModel = createByClassName("func_static");
+    parentModel->addChildNode(light);
+
+    // Parenting should automatically set the parent pointer of the child
+    EXPECT_EQ(light->getParent(), parentModel);
+
+    // Set an offset for the parent model
+    const Vector3 ORIGIN(1024, 512, -320);
+    parentModel->getEntity().setKeyValue("origin", string::to_string(ORIGIN));
+
+    // Parent entity should have a transform matrix corresponding to its
+    // translation
+    EXPECT_EQ(parentModel->localToWorld(), Matrix4::getTranslation(ORIGIN));
+
+    // The light itself should have the same transformation as the parent (since
+    // the method is localToWorld not localToParent).
+    EXPECT_EQ(light->localToWorld(), Matrix4::getTranslation(ORIGIN));
+
+    // Render the light to obtain the RendererLight pointer
+    RenderFixture renderF(true /* solid */);
+    renderF.renderSubGraph(parentModel);
+    EXPECT_EQ(renderF.nodesVisited, 2);
+    EXPECT_EQ(renderF.collector.lights, 1);
+    ASSERT_FALSE(renderF.collector.lightPtrs.empty());
+
+    // Check the rendered light's geometry
+    const RendererLight* rLight = renderF.collector.lightPtrs.front();
+    EXPECT_EQ(rLight->getLightOrigin(), ORIGIN);
+    EXPECT_EQ(rLight->lightAABB().origin, ORIGIN);
+    EXPECT_EQ(rLight->lightAABB().extents, Vector3(320, 320, 320));
+}
+
 TEST_F(EntityTest, RenderUnselectedLightEntity)
 {
     auto light = createByClassName("light");