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.

Burgrave

I'd make the vector a record instead of a class, but YMMV. – Odeliaodelinda

Your idea of TVector3D.Normalise implementation requires TVector3D type to be a reference counted interface instead of a class. – Eats

I've told you before, at least once, to make this a record. I cannot understand why you won't take the advice. – Spanking

@DavidHeffernan I recognise that your advice to code this using records is sound and I will move on to doing so. However, recoding this using records would not have helped me understand WHY the above code gave me the results it did. Which, if you read the question, is what I ask here. Now that I know the answer, I will reimplement this with records. – Burgrave

Fair enough. But I'm looking broader than this question. For example, I can see that you create two objects, and yet free four. Because according to your design, nUVNormed and nonUnitVector actually refer to the same object. And likewise for the other pair. So you have access after free, and double free. And those problems will eat you up. So I urge you to start using records ASAP and so avoid these pitfalls. I've supplied you some code to get going. This is code from this program: orcina.com/SoftwareProducts/OrcaFlex – Spanking

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;

Benzvi

+1, I missed that point (begin...end). I focused on the more 'important' issue (=that cannot be classificed as a 'typo'): you never knew if you had one or two instances when the function returned. – Odeliaodelinda

Great answer, thank you! So my headache was caused by the exclusion of a begin..end block! Am I right in thinking that this can be emitted is there is just one line after the else but not if there are multiple lines? – Burgrave

@Trojanian: Yes, the else corresponds to the next line of code. If that line is begin, then the entire begin...end block is included (probably there is some standard terminology here, that I do not know of...). – Odeliaodelinda

This answer scratches at the surface. The entire design here is a disaster. A function that operates in place should be a procedure that returns nothing. No mention that the code in the question accesses memory after freeing it! The whole thing is terminally broken. – Spanking

@Burgrave My workplace coding standard dictates that all blocks must include begin/end. That ensures that such a mistake cannot occur. – Spanking

@Benzvi - there is on e more point - divizions, twhen there should be multiplication, and calling heavy function thrice instead of caching the result. See stackoverflow.com/questions/14461199 and pastebin.ca/2306480 BTW, David elegantly worked this out by introducing Vector/Scalar operation :-) – Billmyre

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);

Spanking

+1 Excellent work! (Some exercises to the reader: make it possible to compute the inner product between two vectors using the * operator; make a normalize procedure; make an AreOrthogonal class function.) – Odeliaodelinda

@AndreasRejbrand There are too many product operators on vectors. So I personally don't implement any with op overloads. I originally wrote Normalise as a procedure, but it's easier to compose functionality using functions. Hence Normalised. But, there are literally oodles of ways to extend this in useful ways. For a start you'll want TMatrix3 and then product operators that operate on matrices, matrix/vector and so on. – Spanking

Indeed, especially the vector (cross) product between vectors would compete (maybe also the tensor product). Matrix times vector seems quite obvious, though. Anyhow, it's kind of fun to design these little simple but useful records, especially since the introduction of advanced records (with operator overloading and stuff). – Odeliaodelinda

@AndreasRejbrand Kind of fun, and also incredibly useful and powerful for real. My company's entire livelihood is based on the work performed by this one little record! – Spanking

@DavidHeffernan Thank you very much for this - it saves me alot of work in coding it up myself. I will have fun extending this. :-) – Burgrave

@Burgrave You are very welcome. My advice is to try to avoid adding methods that modify the state in place. You'll find it more useful to create functions that return new values. That makes composition much easier for the users of the type. – Spanking

@DavidHeffernan Why is it that you use 0.00001 as the limit in the IsUnit function? I know that often zeros aren't really zeros but are they really ever that big? This seems high to me, especially as you are using doubles. Is this limit chosen from experience? – Burgrave

That's based on experience. We typically use that function in assertions where we assert that the result of a calculation is a unit vector. In my view your attempt to test for a vector being unit and so skipping normalisation, will result in slower code. Depends on your usage, but unless you get a lot of hits for the is unit case, you'll just waste your time testing and branching. – Spanking

There also should be operation Matrix * Vector - implementation can be taken from original question stackoverflow.com/questions/14461199 – Billmyre

@David, did u tried, if X*X makes the same code as intrinsic Sqr(X) ? – Billmyre

@Arioch'The It doesn't Sqr is probably better. In my x86 builds it's all x87 code: FLD QWORD PTR [EAX] FMUL ST(0),ST(0) but on x64 it may be quicker to write Sqr. – Spanking

One more point - i think all those functions should be marked inline. At least most of them. Also personally i'd better made X,Y,Z properties rather than members. pastebin.ca/2306480 – Billmyre

@Arioch'The I don't inline them because of the x87 versions. I also don't think it will help much for my code because it would just make the code size much larger. And I think that would come with its own penalty. – Spanking

const ZeroVector3: TVector3=(); would it compile ? If nothing changes that requires explicitly quoting X,m Y and Z values :-) – Billmyre

Using vector / scalar operation instead of explicit caching inside Normalize is elegant, should have thought of that. But your IsUnit is not - u should have check SqrMag instead of Mag. Well, one more thing i think is missed is typecasting Vector -> double mapped to Mag – Billmyre

I prefer an explicit Mag there thank you. You can do it with a cast if you prefer. And yeah, IsUnit could be more efficient I guess. But it's only used for assertions so we don't bother. – Spanking

Answer 3

A few hints:

First, I'd actually make the vector a record instead of a class if I were you, but YMMV. That would simplify a lot, since the compiler will manage the lifetime of every vector (you never need to worry about freeing things). Second,

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

is normally written, syntactically and exactly equivalently,

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

But even so, it is incorrect. Since you are dealing with floating-point numbers, you cannot reliably test equality. Instead, you should see if the magnitude is within some interval of unity, so that 'fuzz' do not interfere. For instance, you could do (uses Math)

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

Third, your Normalize function returns a new vector object if it needs to normalize, and returns the same object if not. That's very confusing. You'd never know how many instances you have! Instead, make this a procedure:

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

Fourth, why use single instead of double or real?

Fifth, as NGLN pointed out (please upvote his answer!), you forgot the begin...end block in the else part of your Normalize function, so the four last lines are always executed! Hence, you always create a new vector instance! Still, my point is very important: your original function 'intends' (if you just add the begin...end block) to return self or create a new instance depending on a condition, which is rather terrible, since then you do not know how many instances you have! (And so, you'll probably begin to leak vectors...)

Odeliaodelinda

Thank you for that, @Andreas! Though would there not be a slight performance loss for unit vectors due to running Normalise when it is not necessary? Why normalise something that is already normalised. Mind you I guess in practice it will speed up as the check is not necessary when the vector is not already normalised. – Burgrave

@Trojanian: That depends on the situation, as you say: what fraction of the vectors will be normalized? Still, that kind of 'micro-optimization' is seldom worth worrying about. [In fact, I'd be more concerned about the difference in precision in the long run.] – Odeliaodelinda

I am using single because in practice I work with so many vectors that I really need the memory and am willing to sacrifice the accuracy. – Burgrave

Then you réally should use records! It saves a fourth. – Benzvi

@Burgrave i still thing you'd better give you data type a special name. Thus you would change it to real or double or even some other type by changing single line. Like it was done at stackoverflow.com/questions/14461199 – Billmyre

@Arioch'The: you mean like TRealNumber = type single and then use TRealNumber as the type in TVector3D? – Odeliaodelinda

@Andreas yes. Later it may be double or some 8-bits Float or some natural ratio record or whatever. Like i did in my record-based implementation at the above link – Billmyre