2

(This is not a duplicate of What is the difference between a field and a property in C# - but a more subtle issue)

I have an immutable class Camera (from this Ray Tracing example) which works fine:

public class Camera
{
    public Vector3D Pos; 
    public Vector3D Forward; 
    public Vector3D Up;
    public Vector3D Right;

    public  Camera(Vector3D pos, Vector3D lookAt)
    {
        Pos = pos;
        Forward = lookAt - pos;
        Forward.Normalize();
        Vector3D Down = new Vector3D(0, -1, 0);
        Right = Vector3D.CrossProduct(Forward, Down);
        Right.Normalize();
        Right *= 1.5;
        Up = Vector3D.CrossProduct(Forward, Right);
        Up.Normalize();
        Up *= 1.5;
    }
}

As this is an immutable class, I wanted to change the fields to read-only properties, which I have done throughout the application with no problem, thus:

public Vector3D Pos { get; private set; } 
public Vector3D Forward { get; private set; }
public Vector3D Up { get; private set; }
public Vector3D Right { get; private set; }

the rest of the code is unchanged - but the application no longer works correctly. Upon investigation, the values of Forward, Up, and Right are no longer normalized. Normalize, called 3 times in the constructor (above) is a void method. It appears that calling such a method on the property rather than on a field (at least, in the constructor) does not cause the property's value to be updated. It almost appears as though the vector (a struct) is being passed by reference rather than by value, if that makes any sense.

The only way I could find to get this to work was to do all my transforms in the constructor before setting the property values, as shown below:

public  Camera(Vector3D pos, Vector3D lookAt)
{
    var forward = lookAt - pos;
    forward.Normalize();
    var down = new Vector3D(0, -1, 0);
    var right = Vector3D.CrossProduct(forward, down);
    right.Normalize();
    right *= 1.5;
    var up = Vector3D.CrossProduct(forward, right);
    up.Normalize();
    up *= 1.5;
    Pos = pos;
    Forward = forward;
    Up = up;
    Right = right;
}

Can anyone explain to me what exactly is going on here? I understand that there is a difference between a property and a field if you are adding any additional behaviour in the get or set, but I'm not.

Community
  • 1
  • 1
Richard Pawson
  • 1,494
  • 1
  • 10
  • 15
  • I would recommend using `public readonly Vector3D Pos;` instead of `properties` if you're going to make it immutable. You can read more about why [here](http://stackoverflow.com/questions/2719699/when-should-use-readonly-and-get-only-properties). Going further with `value types` in properties I would recommend reading [this](http://stackoverflow.com/questions/25171940/change-the-value-of-a-property-of-a-struct-in-c-sharp) – mrogal.ski Feb 14 '17 at 09:05
  • @m.rogalski: The way he initializes the vectors, they are not immutable and therefor can't be `readonly`. – Lukas Körfer Feb 14 '17 at 09:08
  • The issue is much simpler than it seem. `Vector3D` is a mutable struct, so in order to work correctly it should stay a read/write field. Making it readonly field or property will break the mutating methods. – Ivan Stoev Feb 14 '17 at 09:12
  • @lukegv I must've missed the part when he stated that `Vector3D` is mutable. But still. Why do you even make mutable `struct`s? – mrogal.ski Feb 14 '17 at 09:15
  • 2
    @m.rogalski It's not made by OP - https://msdn.microsoft.com/en-us/library/system.windows.media.media3d.vector3d(v=vs.110).aspx – Ivan Stoev Feb 14 '17 at 09:17
  • @Ivan I concur that the issue is to do with it being a mutatable struct. But you are not explaining why there should be a difference for a field and property. If you can provide a good explanation, please give an answer, not a comment. – Richard Pawson Feb 14 '17 at 09:18
  • @RichardPawson Ah, sorry. Because when you access property or readonly field, you are receiving a copy (due to value type semantics), so mutating methods have no effect on the original value. While when using simple field access you are operating on the original (reference semantics). – Ivan Stoev Feb 14 '17 at 09:21

3 Answers3

1

This is one of the many issues with mutable value types and why they should be avoided althogether.

In your first scenario:

public Vector3D Forward; 
Forward.Normalize();

The value stored in the variable Forward is the instance of the Vector3D itself. Therefore, when you call Forward() you are mutating the value stored in Forward and everything works as expected.

However, in the following code:

private Vector3D forward;

public Vector3D Forward
{
     get { return forward; }
     private set { forward = value; }
}

Forward.Normalize();

Normalize is called on a copy of the value stored in the field forward (remember, the default behavior in C# is to pass arguments by value). Because variable values of value types are the value type instances themselves , what you are doing is mutating a copy, not the value stored in forward.

In your case, the property is autogenerated, but the principle is the same. Autogenerated properties also have a backing field, the compiler just saves you the hassle of all the plumbing involved.

InBetween
  • 32,319
  • 3
  • 50
  • 90
  • 1
    Your answer provides a good illustration of why it is not a great idea to make structs mutatable, but Vector3D is a Microsoft type that breaks this principle. – Richard Pawson Feb 14 '17 at 09:32
  • @RichardPawson Yes, and there are more. `Drawing.Point` comes to mind too. – InBetween Feb 14 '17 at 09:33
0

It almost appears as though the vector (a struct) is being passed by reference rather than by value, if that makes any sense.

No, the actual "problem" is, that it is passed by value. You actually call Normalize on the copied Vector3D created by the getter of your property. The original value can only be changed via the setter (e.g. via *=, which contains an assignment).

I understand that there is a difference between a property and a field if you are adding any additional behaviour in the get or set, but I'm not.

That is not correct. There is always a difference. Your "inline property" is only a wrapper for a backing field, which is then called by the getter and setter (if provided) of your property. And if you get a struct or primitive type, you only copy the value.

Lukas Körfer
  • 13,515
  • 7
  • 46
  • 62
0

The reason is that when you operate on a property, you are operating on a copy of the value. Assuming your class looks like this with properties:

public class Camera
{
    public Vector3D Pos { get; private set; }
    public Vector3D Forward { get; private set; }
    public Vector3D Up { get; private set; }
    public Vector3D Right { get; private set; }

    public Camera(Vector3D pos, Vector3D lookAt)
    {
        Pos = pos;
        Forward = lookAt - pos;
        Forward.Normalize();
        Vector3D Down = new Vector3D(0, -1, 0);
        Right = Vector3D.CrossProduct(Forward, Down);
        Right.Normalize();
        Right *= 1.5;
        Up = Vector3D.CrossProduct(Forward, Right);
        Up.Normalize();
        Up *= 1.5;
    }
}

The call in the constructor to the properties is actually calling a method named get_Forward for example. This method returns a copy of the Vector3D on which Normalize() is called. Looking at the IL code for Forward.Normalize():

ldarg.0     
call        UserQuery+Camera.get_Forward
stloc.1     
ldloca.s    01 
call        System.Windows.Media.Media3D.Vector3D.Normalize

The call UserQuery+Camera.get_Forward method is defined as follows:

ldarg.0     
ldfld       UserQuery+Camera.<Forward>k__BackingField
ret      

The instruction ldfld pushes the value of the field onto the stack, and since in this case it is an instance of System.ValueType this value is copied, resulting in a new Vector3D instance that gets the Normalize() method called on it (the stloc.1 and ldloca.s instructions store and load this copy). This means the original value of the field remains unchanged.

codekaizen
  • 26,990
  • 7
  • 84
  • 140