SonyWWS / ATF

Authoring Tools Framework (ATF) is a set of C#/.NET components for making tools on Windows. ATF has been in continuous development in Sony Computer Entertainment's (SCE) Worldwide Studios central tools group since early 2005. ATF has been used by most SCE first party studios to make many custom tools such as Naughty Dog’s level editor and shader editor for The Last of Us, Guerrilla Games’ sequence editor for Killzone games (including the Killzone: Shadow Fall PS4 launch title), an animation blending tool at Santa Monica Studio, a level editor at Bend Studio, a visual state machine editor for Quantic Dream, sound editing tools, and many others.
Apache License 2.0
1.89k stars 262 forks source link

A HasAttribute<T> function in DomNodeAdapter.cs #15

Closed JelleVM closed 9 years ago

JelleVM commented 10 years ago

I have a Dom Node that has optional attributes, like:

<xs:attribute name="someAttribute" type="xs:float" use="optional" />

I would like to be able to check if my node has this attribute at some given time. However, the GetAttribute<T> function returns default(T) when the attribute doesn't exist. Personally I'd just return null, but I guess that's a design decision. In any case, I think a HasAttribute<T> function would be a worthwhile addition to the framework.

I just implemented it as follows in my classes inheriting it:

protected bool HasAttribute<T>(AttributeInfo attributeInfo)
{
    object value = DomNode.GetAttribute(attributeInfo);

    // If value is not null, attempt the cast; an invalid type will then cause
    // an IllegalCastException.
    if (value != null && (T) value != null)
        return true;
     else
        return false;
}
Ron2 commented 10 years ago

Hello, JelleVM,

Good question. You're talking about the DomNodeAdapter's GetAttribute<T>. If you go to the DomNodeAdapter's DomNode, then you can call the DomNode's IsAttributeDefault(AttributeInfo) which I think will do what you want.

        /// <summary>
        /// Gets whether or not the attribute's value (like by calling GetAttribute) is equal to
        /// the default value.</summary>
        /// <param name="attributeInfo">Attribute metadata</param>
        /// <returns>True if the attribute's value is equal to the default value</returns>
        public bool IsAttributeDefault(AttributeInfo attributeInfo)

So, your code would change from:

if (myDomNodeAdapter.HasAttribute<float>(attributeInfo))

to:

if (myDomNodeAdapter.DomNode.IsAttributeDefault(attributeInfo))

Right?

We could add another convenience method to DomNodeAdapter, to save the client from having to call (and discover) the DomNode.IsAttributeDefault() method. I don't think I would call this convenience method "HasAttribute" though, because conceptually, a DomNode "has" all the attributes that are defined for it.

--Ron

JelleVM commented 10 years ago

Hi Ron, thanks for your reply. Thanks for pointing me to the IsAttributeDefault method, I hadn't thought of that. However, I think it doesn't fully solve the problem. (Though I understand what you're saying that "conceptually, a DomNode 'has' all the attributes that are defined.").

What would you do in the following scenario: your DomNode has an optional variable, say of type int. If this variable is not there, you want your programme to compute it. If it is there already, you don't do anything, to avoid unnecessary computation. This would be expressed by the following code:

int m_someVariable; if (HasAttribute(assets.SomeType.myOptionalTypeAttribute)) m_someVariable = GetAttribute(assets.SomeType.myOptionalTypeAttribute); else m_someVariable = ComputeValueOfOptionalAttribute(); // heavy computation

Now the int value of the attribute could be 0. So if we'd use DomNode.IsAttributeDefault() instead and the variable value happens to be equal to the default value (i.e. 0), we will be doing heave extra work for nothing, I think. Am I right?

Could maybe DomNode use a new method:

public bool HasAttribute(AttributeInfo attributeInfo) /* or "IsAttributeNull" or "HasOptionalAttribute" */ { return GetLocalAttribute(attributeInfo) == null; }

Ron2 commented 10 years ago

Hi JelleVM,

I see the problem; thank you for carefully explaining it. I think you're right, that this would be a useful method. Client code can call GetLocalAttribute() and check the result, but that's not very readable.

I need to think about the name for a bit, write unit tests, and ask coworkers before I check this in. (I'm in Japan for a conference this week, announcing that LevelEditor is open source!) The problem I see with HasAttribute(AttributeInfo) is that it might imply that the method is checking if the AttributeInfo has been defined for that DomNode's DomNodeType. How about AttributeWasSet()? Or AttributeHasBeenSet()?

/// <summary>
/// Gets whether or not the attribute has ever been set, even if it was set to its
/// default value.</summary>
/// <param name="attributeInfo">Attribute metadata</param>
/// <returns>True if the attribute has even been set and false otherwise.</returns>
public bool AttributeWasSet(AttributeInfo attributeInfo)
{
    return GetLocalAttribute(attributeInfo) != null;
}

--Ron

JelleVM commented 10 years ago

Hi Ron,

Thanks for considering it! It's indeed quite an uncommon user case, but that's why I brought it up. The name for the function is entirely up to you and your team of course, but I agree that your suggestions are probably clearer for a generic user case. Have fun in Japan -- more open source ATF stuff, yeay! :-)

Ron2 commented 9 years ago

I'll check this new method on DomNode shortly. Thanks again for the good suggestion!

/// <summary>
/// Gets whether or not the attribute has been set, even if it was set to its default value.</summary>
/// <param name="attributeInfo">Attribute metadata</param>
/// <returns>True iff the attribute has been set to anything other than null</returns>
/// <remarks>Setting an attribute to null is special, and makes the attribute look like it
/// was never set.</remarks>
public bool IsAttributeSet(AttributeInfo attributeInfo)
{
    return GetLocalAttribute(attributeInfo) != null;
}