#5546: attached light entities now correctly coloured

Rather than rely exclusively on the possibly-inherited editor_color spawnarg,
entity classes now check this spawnarg only at their own level, then
recursively delegate to their parent class's getColour() method. This ensures
that colour overrides (which are implemented in terms of setColour/getColour)
propagate to child classes.

include/ieclass.h

@@ -168,7 +168,7 @@ class EntityClassAttribute
      */
     EntityClassAttribute(const std::string& type_,
                          const std::string& name_,
-                         const std::string& value_, 
+                         const std::string& value_,
                          const std::string& description_ = "")
     : _typeRef(new std::string(type_)),
       _nameRef(new std::string(name_)),
@@ -180,13 +180,13 @@ class EntityClassAttribute
     /**
      * Construct a inherited EntityClassAttribute with a true inherited flag.
      * The strings are taken from the inherited attribute.
-     * Note: this is not a copy-constructor on purpose, to allow STL assignments to 
+     * Note: this is not a copy-constructor on purpose, to allow STL assignments to
      * copy the actual instance values.
      */
     EntityClassAttribute(const EntityClassAttribute& parentAttr, bool inherited_)
     : _typeRef(parentAttr._typeRef),    // take type string,
       _nameRef(parentAttr._nameRef),    // name string,
-      _valueRef(parentAttr._valueRef),  // value string 
+      _valueRef(parentAttr._valueRef),  // value string
       _descRef(parentAttr._descRef),    // and description from the parent attribute
       inherited(inherited_)
     {}
@@ -201,7 +201,7 @@ typedef std::shared_ptr<const IEntityClass> IEntityClassConstPtr;
 
 /**
  * \brief Entity class interface.
- * 
+ *
  * An entity class represents a single type of entity that can be created by
  * the EntityCreator. Entity classes are parsed from .DEF files during startup.
  *
@@ -262,15 +262,24 @@ class IEntityClass
     /**
      * Return a single named EntityClassAttribute from this EntityClass.
      *
-     * @param name
+     * \param name
      * The name of the EntityClassAttribute to find, interpreted case-insensitively.
      *
-     * @return
+     * \param includeInherited
+     * true if attributes inherited from parent entity classes should be
+     * considered, false otherwise.
+     *
+     * \return
      * A reference to the named EntityClassAttribute. If the named attribute is
      * not found, an empty EntityClassAttribute is returned.
      */
-    virtual EntityClassAttribute& getAttribute(const std::string& name) = 0;
-    virtual const EntityClassAttribute& getAttribute(const std::string& name) const = 0;
+    virtual EntityClassAttribute&
+    getAttribute(const std::string& name, bool includeInherited = true) = 0;
+
+    /// Get a const EntityClassAttribute reference by name
+    virtual const EntityClassAttribute&
+    getAttribute(const std::string& name,
+                 bool includeInherited = true) const = 0;
 
     /**
      * Enumerate the EntityClassAttibutes in turn.
@@ -300,7 +309,7 @@ class IEntityClass
     virtual const std::string& getSkin() const = 0;
 
 	/**
-	 * Returns true if this entity is of type or inherits from the 
+	 * Returns true if this entity is of type or inherits from the
 	 * given entity class name. className is treated case-sensitively.
 	 */
 	virtual bool isOfType(const std::string& className) = 0;
@@ -428,7 +437,7 @@ class IEntityClassManager :
      */
     virtual void reloadDefs() = 0;
 
-    /** 
+    /**
      * greebo: Finds the model def with the given name. Might return NULL if not found.
      */
     virtual IModelDefPtr findModel(const std::string& name) = 0;

radiantcore/eclass/Doom3EntityClass.cpp

@@ -115,18 +115,21 @@ void Doom3EntityClass::setColour(const Vector3& colour)
 
 void Doom3EntityClass::resetColour()
 {
-    // (Re)set the colour
-    const EntityClassAttribute& colourAttr = getAttribute("editor_color");
-
-    if (!colourAttr.getValue().empty())
-    {
-        setColour(string::convert<Vector3>(colourAttr.getValue()));
-    }
-    else
+    // Look for an editor_color on this class only
+    const EntityClassAttribute& attr = getAttribute("editor_color", false);
+    if (!attr.getValue().empty())
     {
-        // If no colour is set, assign the default entity colour to this class
-        setColour(DefaultEntityColour);
+        return setColour(string::convert<Vector3>(attr.getValue()));
     }
+
+    // If there is a parent, use its getColour() directly, rather than its
+    // editor_color attribute, to take into account overrides through the
+    // EClassColourManager
+    if (_parent)
+        return setColour(_parent->getColour());
+
+    // No parent and no attribute, all we can use is the default colour
+    setColour(DefaultEntityColour);
 }
 
 const Vector3& Doom3EntityClass::getColour() const {
@@ -255,7 +258,15 @@ void Doom3EntityClass::resolveInheritance(EntityClasses& classmap)
         _colourTransparent = true;
     }
 
+    // Set up inheritance of entity colours: colours inherit from parent unless
+    // there is an explicit editor_color defined at this level
     resetColour();
+    if (_parent)
+    {
+        _parent->changedSignal().connect(
+            sigc::mem_fun(this, &Doom3EntityClass::resetColour)
+        );
+    }
 }
 
 bool Doom3EntityClass::isOfType(const std::string& className)
@@ -279,13 +290,18 @@ std::string Doom3EntityClass::getDefFileName()
 }
 
 // Find a single attribute
-EntityClassAttribute& Doom3EntityClass::getAttribute(const std::string& name)
+EntityClassAttribute& Doom3EntityClass::getAttribute(const std::string& name,
+                                                     bool includeInherited)
 {
-    return const_cast<EntityClassAttribute&>(std::as_const(*this).getAttribute(name));
+    return const_cast<EntityClassAttribute&>(
+        std::as_const(*this).getAttribute(name, includeInherited)
+    );
 }
 
 // Find a single attribute
-const EntityClassAttribute& Doom3EntityClass::getAttribute(const std::string& name) const
+const EntityClassAttribute&
+Doom3EntityClass::getAttribute(const std::string& name,
+                               bool includeInherited) const
 {
     StringPtr ref(new std::string(name));
 
@@ -294,8 +310,9 @@ const EntityClassAttribute& Doom3EntityClass::getAttribute(const std::string& na
     if (f != _attributes.end())
         return f->second;
 
-    // If there is no parent, this is the end of the line: return an empty attribute
-    if (!_parent)
+    // If there is no parent or we have been instructed to ignore inheritance,
+    // this is the end of the line: return an empty attribute
+    if (!_parent || !includeInherited)
         return _emptyAttribute;
 
     // Otherwise delegate to the parent (which will recurse until an attribute

radiantcore/eclass/Doom3EntityClass.h

@@ -77,9 +77,9 @@ class Doom3EntityClass
     // greebo: I've changed the EntityAttributeMap key type to StringPtr, to save
     // more than 130 MB of string data used for just the keys. A default TDM installation
     // has about 780k entity class attributes after resolving inheritance.
-    // However, the memory saving comes with a performance cost since we need 
+    // However, the memory saving comes with a performance cost since we need
     // to convert the incoming std::string queries to StringRefs before passing
-    // them to std::map::find(). During a regular DarkRadiant startup 
+    // them to std::map::find(). During a regular DarkRadiant startup
     // there are about 64k find() operations, a single idLight
     // creation costs about 30-40 find() operations, which is ok I guess.
 
@@ -165,8 +165,11 @@ class Doom3EntityClass
     void resetColour();
     const std::string& getWireShader() const override;
     const std::string& getFillShader() const override;
-    EntityClassAttribute& getAttribute(const std::string& name) override;
-    const EntityClassAttribute& getAttribute(const std::string& name) const override;
+    EntityClassAttribute& getAttribute(const std::string&,
+                                       bool includeInherited = true) override;
+    const EntityClassAttribute&
+    getAttribute(const std::string&,
+                 bool includeInherited = true) const override;
     void forEachClassAttribute(std::function<void(const EntityClassAttribute&)>,
                                bool) const override;
 

test/Entity.cpp

@@ -98,6 +98,10 @@ TEST_F(EntityTest, EntityClassInheritsAttributes)
     // Inherited but overridden on 'light_extinguishable' itself
     EXPECT_EQ(cls->getAttribute("editor_displayFolder").getValue(),
               "Lights/Base Entities, DoNotUse");
+
+    // Lookup without considering inheritance
+    EXPECT_EQ(cls->getAttribute("editor_color", false).getValue(), "");
+    EXPECT_EQ(cls->getAttribute("spawnclass", false).getValue(), "");
 }
 
 TEST_F(EntityTest, CannotCreateEntityWithoutClass)