Problems returning self as a function result

Question

I have a very simple class definition for 3D Vectors, TVector3D, and a few methods used to implement the TVector3D.Normalise function. If I pass the Normalise function a vector that is already normalised, I want it to return the vector I passed it. Here I have used Result := Self but I am having some crazy returns.

The console application:

program Project1;

{$APPTYPE CONSOLE}

{$R *.res}

uses
  System.SysUtils;

type
  TVector3D = Class
  public
    x : Single;
    y : Single;
    z : Single;
    constructor Create(x : Single;
                       y : Single;
                       z : Single);
    function GetMagnitude() : Single;
    function IsUnitVector() : Boolean;
    function Normalise() : TVector3D;
  end;

  constructor TVector3D.Create(x : Single;
                               y : Single;
                               z : Single);
  begin
    Self.x := x;
    Self.y := y;
    Self.z := z;
  end;

  function TVector3D.GetMagnitude;
  begin
    Result := Sqrt(Sqr(Self.x) + Sqr(Self.y) + Sqr(Self.z));
  end;

  function TVector3D.IsUnitVector;
  begin
    if Self.GetMagnitude = 1 then
      Result := True
    else
      Result := False;
  end;

  function TVector3D.Normalise;
  var
    x : Single;
    y : Single;
    z : Single;
    MagnitudeFactor : Single;
  begin
    if IsUnitVector then
      Result := Self
    else
      MagnitudeFactor := 1/(Self.GetMagnitude);
      x := Self.x*MagnitudeFactor;
      y := Self.y*MagnitudeFactor;
      z := Self.z*MagnitudeFactor;
      Result := TVector3D.Create(x,
                                 y,
                                 z);
  end;

  procedure TestNormalise;
  var
    nonUnitVector : TVector3D;
    unitVector : TVector3D;
    nUVNormed : TVector3D;
    uVNormed : TVector3D;
  begin
  //Setup Vectors for Test
    nonUnitVector := TVector3D.Create(1,
                                      1,
                                      1);
    unitVector := TVector3D.Create(1,
                                   0,
                                   0);
  //Normalise Vectors & Free Memory
    nUVNormed := nonUnitVector.Normalise;
    nonUnitVector.Free;
    uVNormed := unitVector.Normalise;
    unitVector.Free;
  //Print Output & Free Memory
    WriteLn('nUVNormed = (' + FloatToStr(nUVNormed.x) + ', ' + FloatToStr(nUVNormed.y) + ', ' + FloatToStr(nUVNormed.z) + ')');
    nUVNormed.Free;
    WriteLn('uVNormed = (' + FloatToStr(uVNormed.x) + ', ' + FloatToStr(uVNormed.y) + ', ' + FloatToStr(uVNormed.z) + ')');
    uVNormed.Free;
  end;

begin
  try
    TestNormalise;
    Sleep(10000);
  except
    on E: Exception do
      Writeln(E.ClassName, ': ', E.Message);
  end;
end.

Normalise works fine for non-unit vecors, i.e. IsUnitVector returns false. But for unit vectors, such as (1,0,0), instead of returning itself I get a result with very low nonzero numbers wherever there was a nonzero previously, such as (8.47122...E-38,0,0).

If I run this through the debugger with a breakpoint on the line Result := Self set to evaluate Self, Self is (1,0,0) yet result becomes (Very Low Number,0,0). Where Very Low Number changes each time I run the programme but always seems to be around E-38/E-39.

I do not understand why this happens. Why does it happen and how is it best to alter my Normalise function to avoid it.

Answer 1

Your current TVector3D.Normalise implementation has some issues:

• The last 4 lines are always executed, because you have not used a begin-end block after the else,
• So the routine never returns Self, but always a new instance,
• The returned instance's memory is propably leaked because you lost ownership of it after the function call,
• When IsUnitVector returns True, then the assignment of MagnitudeFactor will be skipped, and it will be a random value (currently present at that memory's address), which explains why you get rubish. You are also warned by the compiler for this: Variable MagnitudeFactor might not have been initialized.

Instead, I would rewrite the routine as follows:

function TVector3D.Normalise: TVector3D;
begin
  if not IsUnitVector then
  begin
    x := x / GetMagnitude;
    y := y / GetMagnitude;
    z := z / GetMagnitude;
  end;
  Result := Self;
end;

Answer 2

The root of all your problems is that you are using a class, which is a reference type. Instead you need to make your vector be a value type. That means use a record.

In your code, even when you fix the problem identified by @NGLN, you have still destroyed all instances of your class by the time you start calling WriteLn.

Unless you grasp this issue soon, I fear that you will continue having problems. Switching to using a value type will make your coding trivially easy in comparison to your current approach.

Here's something to get you started:

type
  TVector3 = record
  public
    class operator Negative(const V: TVector3): TVector3;
    class operator Equal(const V1, V2: TVector3): Boolean;
    class operator NotEqual(const V1, V2: TVector3): Boolean;
    class operator Add(const V1, V2: TVector3): TVector3;
    class operator Subtract(const V1, V2: TVector3): TVector3;
    class operator Multiply(const V: TVector3; const D: Double): TVector3;
    class operator Multiply(const D: Double; const V: TVector3): TVector3;
    class operator Divide(const V: TVector3; const D: Double): TVector3;
    class function New(const X, Y, Z: Double): TVector3; static;
    function IsZero: Boolean;
    function IsNonZero: Boolean;
    function IsUnit: Boolean;
    function Mag: Double;
    function SqrMag: Double;
    function Normalised: TVector3;
    function ToString: string;
  public
    X, Y, Z: Double;
  end;

const
  ZeroVector3: TVector3=();

class operator TVector3.Negative(const V: TVector3): TVector3;
begin
  Result.X := -V.X;
  Result.Y := -V.Y;
  Result.Z := -V.Z;
end;

class operator TVector3.Equal(const V1, V2: TVector3): Boolean;
begin
  Result := (V1.X=V2.X) and (V1.Y=V2.Y) and (V1.Z=V2.Z);
end;

class operator TVector3.NotEqual(const V1, V2: TVector3): Boolean;
begin
  Result := not (V1=V2);
end;

class operator TVector3.Add(const V1, V2: TVector3): TVector3;
begin
  Result.X := V1.X + V2.X;
  Result.Y := V1.Y + V2.Y;
  Result.Z := V1.Z + V2.Z;
end;

class operator TVector3.Subtract(const V1, V2: TVector3): TVector3;
begin
  Result.X := V1.X - V2.X;
  Result.Y := V1.Y - V2.Y;
  Result.Z := V1.Z - V2.Z;
end;

class operator TVector3.Multiply(const V: TVector3; const D: Double): TVector3;
begin
  Result.X := D*V.X;
  Result.Y := D*V.Y;
  Result.Z := D*V.Z;
end;

class operator TVector3.Multiply(const D: Double; const V: TVector3): TVector3;
begin
  Result.X := D*V.X;
  Result.Y := D*V.Y;
  Result.Z := D*V.Z;
end;

class operator TVector3.Divide(const V: TVector3; const D: Double): TVector3;
begin
  Result := (1.0/D)*V;
end;

class function TVector3.New(const X, Y, Z: Double): TVector3;
begin
  Result.X := X;
  Result.Y := Y;
  Result.Z := Z;
end;

function TVector3.IsZero: Boolean;
begin
  Result := Self=ZeroVector3;
end;

function TVector3.IsNonZero: Boolean;
begin
  Result := Self<>ZeroVector3;
end;

function TVector3.IsUnit: Boolean;
begin
  Result := abs(1.0-Mag)<1.0e-5;
end;

function TVector3.Mag: Double;
begin
  Result := Sqrt(X*X + Y*Y + Z*Z);
end;

function TVector3.SqrMag: Double;
begin
  Result := X*X + Y*Y + Z*Z;
end;

function TVector3.Normalised;
begin
  Result := Self/Mag;
end;

function TVector3.ToString: string;
begin
  Result := Format('(%g, %g, %g)', [X, Y, Z]);
end;

This is extracted from my own codebase. I'm using Double, but if you really prefer to use Single, then you can readily change it.

The use of operator overloading makes the code you write so much more readable. Now you can write V3 := V1 + V2 and so on.

Here's what your test code looks like with this record:

var
  nonUnitVector: TVector3;
  unitVector: TVector3;
  nUVNormed: TVector3;
  uVNormed: TVector3;

begin
  //Setup Vectors for Test
  nonUnitVector := TVector3.New(1, 1, 1);
  unitVector := TVector3.New(1, 0, 0);

  //Normalise Vectors
  nUVNormed := nonUnitVector.Normalised;
  uVNormed := unitVector.Normalised;

  //Print Output
  WriteLn('nUVNormed = ' + nUVNormed.ToString);
  WriteLn('uVNormed = ' + uVNormed.ToString);
  Readln;
end.

Or if you want to compress it somewhat:

WriteLn('nUVNormed = ' + TVector3.New(1, 1, 1).Normalised.ToString);
WriteLn('uVNormed = ' + TVector3.New(1, 0, 0).Normalised.ToString);