C# Private member shared by all class instances

Question

I am currently working with C# using the Unity3D engine and have come upon the following problem:

I created a class that has two private references to instances of another class which it has to access. Once I create multiple instances of the class and set the references I found out that all instances were using the same variable. I realized this as I was destroying an instance and just before that set the two variables holding the references to null. Immediately after doing that all other instances were throwing NullReferenceExceptions because they were still trying to access the references. The referenced objects are fine, other scripts can still access them.

Here is some pseudo code illustrating the structure:

public class Character
{
    // Character data
}

public class StatusEffect
{
    private Character target;
    private Character originator;

    public void Init(Character _Target, Character _Originator)
    {
        target = _Target;
        originator = _Originator;
    }

    public void Destroy()
    {
        target = null;
        originator = null;
    }
}

In the program it would be called like this:

StatusEffect effect = new StatusEffect();
effect.Init(player1, player2);

// Time goes by

effect.Destroy();

After calling Destroy() every StatusEffect's two references will be null.

This is not only an issue when destroying StatusEffects, but also when creating new ones. As soon as I touch the references from within a new instance all StatusEffects will reference the two Characters specified by the new StatusEffect.

I do not understand why or how I can fix this issue. Can someone enlighten me on this matter?

Cheers, Valtaroth

EDIT:

Here is the real code as requested: I have a container class holding several StatusEffects. As soon as it starts, it initializes all of them.

public class CElementTag
{
    // ..Other data..

    public float f_Duration; // Set in the editor

    private CGladiator gl_target;
    private CGladiator gl_originator;
    private float f_currentDuration;

    public CStatusEffect[] ar_statusEffects;

    // Starts the effect of the element tag
    public void StartEffect(CGladiator _Originator, CGladiator _Target)
    {
        gl_originator = _Originator;
        gl_target = _Target;
        f_currentDuration = f_Duration;

        for(int i = 0; i < ar_statusEffects.Length; i++)
            ar_statusEffects[i].Initialize(gl_originator, gl_target);
    }

    // Ends the effect of the element tag
    public void EndEffect()
    {
        for(int i = 0; i < ar_statusEffects.Length; i++)
        {
            if(ar_statusEffects[i] != null)
                ar_statusEffects[i].Destroy();
        }
    }

    // Called every update, returns true if the tag can be destroyed
    public bool ActivateEffect()
    {
        f_currentDuration -= Time.deltaTime;

        if(f_currentDuration <= 0.0f)
        {
            EndEffect();
            return true;
        }

        for(int i = 0; i < ar_statusEffects.Length; i++)
        {
            if(ar_statusEffects[i] != null && ar_statusEffects[i].Update())
                RemoveStatusEffect(i);
        }

        return false;
    }

    // Removes expired status effects
    private void RemoveStatusEffect(int _Index)
    {
        // Call destroy method
        ar_statusEffects[_Index].Destroy();

        // Remove effect from array
        for(int i = _Index; i < ar_statusEffects.Length - 1; i++)
            ar_statusEffects[i] = ar_statusEffects[i+1];

        ar_statusEffects[ar_statusEffects.Length - 1] = null;
}
}

The actual StatusEffect class is holding the two references as well as some other data it needs to work. It has virtual methods because there are some classes inheriting from it.

public class CStatusEffect
{
    // ..Necessary data..

    // References
    protected CGladiator gl_target;
    protected CGladiator gl_originator;

    virtual public void Initialize(CGladiator _Target, CGladiator _Originator)
    {
        gl_target = _Target;
        gl_originator = _Originator;

        // ..Initialize other necessary stuff..
    }

    virtual public void Destroy()
    {
        gl_target = null;
        gl_originator = null;

        // ..Tidy up other data..
    }

    virtual public bool Update()
    {
        // ..Modifying data of gl_target and gl_originator..
        // Returns true as soon as the effect is supposed to end.
    }
}

That should be all the relevant code concerning this problem.

EDIT2

@KeithPayne I have a static array of ElementTags defined in the editor and saved to xml. At the beginning of the program the static array is loading the xml and stores all element tags. When creating a new element tag to use I utilize this constructor:

// Receives a static tag as parameter
public CElementTag(CElementTag _Tag)
{
    i_ID = _Tag.i_ID;
    str_Name = _Tag.str_Name;
    enum_Type = _Tag.enum_Type;
    f_Duration = _Tag.f_Duration;

    ar_statusEffects = new CStatusEffect[_Tag.ar_statusEffects.Length];
    Array.Copy(_Tag.ar_statusEffects, ar_statusEffects, _Tag.ar_statusEffects.Length);
}

Do I have to use a different method to copy the array to the new tag? I thought Array.Copy would make a deep copy of the source array and stored it in the destination array. If it is in fact making a shallow copy, I understand where the problem is coming from now.

Answer 1

Because you're passing in references to the objects via your Init() method, you're not actually "copying" the objects, just maintaining a reference to the same underlying objects in memory.

If you have multiple players with the same references to the same underlying objects, then changes made by player 1 will effect the objects being used by player 2.

Having said all that, you're not actually disposing the objects in your Destory method. Just setting the local instance references to Null which shouldn't affect any other instances of StatusEffects. Are you sure something else isn't disposing the objects, or that you haven't properly init'd your other instances.

If you do want to take a full copy of the passed in objects, take a look at the ICloneable interface. It looks like you want to pass in a copy of the objects into each Player.

public class Character : ICloneable
{
    // Character data

    //Implement Clone Method
}

public class StatusEffect
{
    private Character target;
    private Character originator;

    public void Init(Character _Target, Character _Originator)
    {
        target = _Target.Clone()
        originator = _Originator.Clone();
    }

Answer 2

From Array.Copy Method (Array, Array, Int32):

If sourceArray and destinationArray are both reference-type arrays or are both arrays of type Object, a shallow copy is performed. A shallow copy of an Array is a new Array containing references to the same elements as the original Array. The elements themselves or anything referenced by the elements are not copied. In contrast, a deep copy of an Array copies the elements and everything directly or indirectly referenced by the elements.

Consider this fluent version of the StatusEffect class and its usage below:

public class StatusEffect
{
    public Character Target { get; private set; }
    public Character Originator { get; private set; }

    public StatusEffect Init(Character target, Character originator)
    {
        Target = target.Clone()
        Originator = originator.Clone();
        return this;
    }
//...
}    

public CElementTag(CElementTag _Tag)
    {
        i_ID = _Tag.i_ID;
        str_Name = _Tag.str_Name;
        enum_Type = _Tag.enum_Type;
        f_Duration = _Tag.f_Duration;

        ar_statusEffects = _Tag.ar_statusEffects.Select(eff => 
            new StatusEffect().Init(eff.Target, eff.Originator)).ToArray();

        // ar_statusEffects = new CStatusEffect[_Tag.ar_statusEffects.Length];
        // Array.Copy(_Tag.ar_statusEffects, ar_statusEffects, _Tag.ar_statusEffects.Length);


    }

Answer 3

The fields aren't shared(static) among other instances. So calling target = null; in Destroy() won't affect other instances.

StatusEffect effect1 = new StatusEffect();
effect1.Init(player1, player2);

StatusEffect effect2 = new StatusEffect();
effect2.Init(player1, player2);

// Time goes by

effect2.Destroy();

// Some more time goes by

// accessing effect1.target won't give a `NullReferenceException` here unless player1 was null before passed to the init.

effect1.Destroy();

I think you did forget the Init(..) on the other instances. Every time you create an instance of StatusEffect, you need to call Init(...).

---

Update:

This line will clear the reference to the effect, but you never recreate it:

ar_statusEffects[ar_statusEffects.Length - 1] = null;

so the next time you call ar_statusEffects[x].Update() or Initialize() etc it will throw a NullReferenceException

If you want to clear out effects within you array, you could create an Enable bool in the effect, this way you only have to set/reset it.

    for(int i = 0; i < ar_statusEffects.Length; i++)
        if(ar_statusEffects[i].IsEnabled)
            ar_statusEffects[i].Update();

Why don't you use a List instead? Arrays will be faster as long you don't have to shuffle in it. (like circulair buffers etc)

Answer 4

Thanks to Keith Payne I figured out where the problem was. I was creating a deep copy of CElementTag, but not of my ar_statusEffects array. I wrongly assumed Array.Copy was creating a deep copy of an array when it actually was not.

I implemented the IClonable interface for my CStatusEffect and use the Clone() method to create a true deep copy for each member of the static array and add it to the new tags ar_statusEffects array. This way I have seperate instances of the effects instead of references to the same static effect.

Thanks to everyone, especially Keith Payne, for their help and support!