Problems returning as a result of a function

Question

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

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 great for non-proprietary vecors, i.e. IsUnitVector returns false. But for unit vectors such as (1,0,0) , instead of returning, I get a result with very low non-zero numbers, where there used to be a non-zero number, for example (8.47122...E-38,0,0) .

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

I do not understand why this is happening. Why this is happening and how best to change my Normalise function to avoid it.

Answer 1

There are some issues in your current implementation of TVector3D.Normalise :

• The last 4 lines are always executed, because you did not use the begin-end block after else ,
• Therefore, the procedure never returns Self , but always a new instance
• The returned instance memory may be leaked because you lost ownership of it after calling the function,
• When IsUnitVector returns True , the MagnitudeFactor assignment will be skipped and it will be a random value (currently present at this memory address), which explains why you get rubies. You are also warned by the compiler: the Variable MagnitudeFactor can not be initialized.

Instead, I would rewrite the procedure 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 that is a reference type. Instead, you need to make your vector a value type. This means use record .

In your code, even when you fix the problem identified by @NGLN, you still destroy all instances of your class by the time the WriteLn call starts.

If you do not understand this problem in the near future, I am afraid that you will have problems. Switching to using a value type will make your coding trivially simple compared to your current approach.

Here you can start:

 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 := -VX; Result.Y := -VY; Result.Z := -VZ; 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*VX; Result.Y := D*VY; Result.Z := D*VZ; end; class operator TVector3.Multiply(const D: Double; const V: TVector3): TVector3; begin Result.X := D*VX; Result.Y := D*VY; Result.Z := D*VZ; 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 code base. I use Double , but if you really prefer to use Single , you can easily change it.

Using operator overloading makes the code you write much more readable. Now you can write V3 := V1 + V2 , etc.

Here is what your test code looks like with this entry:

 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 a bit:

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

Answer 3

A few tips:

First, I would make the vector a record instead of a class, if I were you, but YMMV. This would make things a lot easier, since the compiler would control the lifetime of each vector (you don't have to worry about freeing things up). Secondly,

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

usually written syntactically and exactly equivalent

 function TVector3D.IsUnitVector; begin result := GetMagnitude = 1 end; 

But even so, this is not true. Since you are dealing with floating point numbers, you cannot reliably verify equality. Instead, you should see if the value is within a certain range of unity, so that "fluff" does not interfere. For example, you can do ( uses Math )

 function TVector3D.IsUnitVector; begin result := IsZero(GetMagnitude - 1) end; 

Thirdly, your Normalize function returns a new vector object if it needs to be normalized, and returns the same object if not. This is very confusing. You will never know how many copies you have! Instead, complete the following procedure:

 procedure TVector3D.Normalize; var norm: single; begin norm := GetMagnitude; x := x / norm; y := y / norm; z := z / norm; end; 

Fourth, why use a single instead of double or real ?

Fifth, as NGLN pointed out (please confirm your answer!), You forgot the begin...end block in the else part of your Normalize function, so all four lines are always executed! Therefore, you always create a new vector instance! However, my point of view is very important: your original function "intends" (if you just added a begin...end block) to return self or create a new instance depending on the state, which is pretty terrible, since then you are not know how many copies you have! (So, you will probably start flowing vectors ...)