Suggestion for TGLZHmgPlane

[dicepd]

  TGLZHmgPlane = record
     // Computes the parameters of a plane defined by a point and a normal.
     procedure Create(constref point, normal : TGLZVector); //overload;
     function  Distance(constref point : TGLZVector) : Single;  overload;
     function  Distance(constref Center : TGLZVector; constref Radius:Single) : Single; overload  

     case Byte of
       0: (V: TGLZVector4fType);         // should have type compat with other vector4f
       1: (A, B, C, D: Single);          // Plane Parameter access
       2: (AsNormal3: TGLZAffineVector); // super quick descriptive access to Normal as Affine Vector.
       3: (AsVector: TGLZVector);
       4: (X, Y, Z, W: Single);          // legacy access so existing code works
  end;         

[dicepd]

Ok more reading and I am talking bollocks, dumb post removed, we seem to store unit plane from the word go, or at least we should be. So that is how I will do the unit testing. TGLZHmgPlane should be a unit plane by the time it hits the testing framework, if not then it is a fault in one of the create routines.

[dicepd]

Ok, I have just set up a test harness for this and the first impressions for any chances in using SSE to create a unit plane are not good. I ended up going back to basics with the following code for plane create 3 vectors (basic maths)

  A := p1.y * (p2.Z - p3.Z) + p2.Y * (p3.Z - p1.Z) + p3.Y * (p1.Z - p2.Z);
  B := p1.Z * (p2.X - p3.X) + p2.Z * (p3.X - p1.X) + p3.Z * (p1.X - p2.X);
  C := p1.X * (p2.Y - p3.Y) + p2.X * (p3.Y - p1.y) + p3.X * (p1.y - p2.Y);
  D := -(p1.X * (p2.Y * p3.Z - p3.Y * p2.Z) + p2.X * (p3.Y * p1.Z - p1.y * p3.Z) + p3.X * (p1.y * p2.Z - p2.Y * p1.Z));                

with tests of

  // create from three points
  ph1.Create(vt1,vt2,vt3);
  // all points should satisfy
  // plane.A*Point.X + plane.B*Point.Y + plane.C*Point.Z + PlaneD = 0
  fs1 := ph1.A * vt1.X + ph1.B * vt1.Y + ph1.C * vt1.Z + ph1.D;
  AssertTrue('TGLZHmgPlane:Create3Vec:sub1 Point 1 does not lie on plane', IsEqual(fs1,0, 1e-3));                 

As you can see the Epsilon to get the tests to pass are quite large. We may need to go to doubles just in the creation of the unit plane??? and this is before we normalize the plane coefficients. Looking at the code all the single calcs have been done in SSE registers as we have the compiler flags for that.

I will do some double tests to see what comes out.

[dicepd]

Ok Created a routine for checking doubles

procedure PlaneCof(p1x,p1y,p1z,p2x,p2y,p2z,p3x,p3y,p3z: double; var A, B, C, D: double);
begin
  A := p1y * (p2Z - p3Z) + p2Y * (p3Z - p1Z) + p3Y * (p1Z - p2Z);
  B := p1Z * (p2X - p3X) + p2Z * (p3X - p1X) + p3Z * (p1X - p2X);
  C := p1X * (p2Y - p3Y) + p2X * (p3Y - p1y) + p3X * (p1y - p2Y);
  D := -(p1X * (p2Y * p3Z - p3Y * p2Z) + p2X * (p3Y * p1Z - p1y * p3Z) + p3X * (p1y * p2Z - p2Y * p1Z));
end;

This also used the xmm registers because of compiler switches but use double variants

            mulsd  %xmm11,%xmm9
            addsd  %xmm10,%xmm9
            movsd  %xmm9,(%rsi)
            movapd %xmm4,%xmm9
            subsd  %xmm8,%xmm9

This allowed me to remove the forced epsilon and it passed results fine.

[dicepd]

One last thought before I head out for the day, it is probably important to have planes as accurate as possible for mesh manipulation, intersecting planes not quite intersecting with other planes and being a cause of holed meshes and tears.

[dicepd]

Ok, so checkin cccbec4 is the root cause of all issues with plane so far.

This does however raise a question on behavior of a plane 'object' when the two creation methods exhibit wildly different behaviors. Currently, and also in GLScene, create with three points creates a unit plane, whereas create with a norm and point allows the definition of a non unit plane.

With the non unit plane certain functions will still work ok such as IsPointOnPlane, IsInHalfSpace etc basically any function that does a > < <= >= = <> comparison with the plane equation.

Distance functions will fail miserably with a non unit plane.

Untested but I suspect intersection [point|line] values would also require a unit plane to return the correct point.

So do we

1. Support both type as is and let a user shoot themselves in the foot.
2. Support UnitPlane only.
3. Support both type each with its create method made clear (Create, CreateNorm)

[dicepd]

Ok I have a cut of this done for Pascal with unit tests but not reorganised the asm yet. Shall I create a branch or just checkin when I have something that is working?

[jdelauney]

I don't know very well this routines (never used) so do like you want. I thinks perhaps it will be better to have new unit for plane and also other records type if need we can create a unit named "VectorGeometry" " VectorMathAddons" or whatever, and place your suggestions. In the same i thought something similar for "Raycast"

Type 
  TGLZRay = class
 private
    aRay : TGLZVector4f
public
    function IntersectWithPlane
    function IntersectWithBBox
    function IntersectWithBSphereBox
    function IntersectWithLine
    ........

One things i'm sure our lib work here with the unit test. But outer it have some bad issue with assembler code. I've made an animation of Craig Reynold's Boids. With native code no problem (used TGLZVector4f and 2f) . But with SSE enabled, the application frozed at start. I'm need check more things.

[dicepd]

If outer code somehow uses and forces fpu routines then all sorts of nasties will happen as we have not protected anything yet with EMMS instruction.

[dicepd]

Also I would issue a EMMS just before you call any GL functions.

[dicepd]

One other thing you can try Jerome is

  movaps xmm0, [RDI]     // xmm0 = Plane->d | Plane->c | Plane->b | Plane->a
{$ifdef SAFECALL}
  movups xmm1, [POINT]   // xmm1 = ???????? | Point->z | Point->y | Point->x
{$else}
  movaps xmm1, [POINT]   // xmm1 = ???????? | Point->z | Point->y | Point->x
{$endif}     

Only do this for parameters which are not self, that should be fine, and don't bother with movups on consts as they will be aligned.

If you are just using two classes atm this should not be too big a job.

[jdelauney]

Also I would issue a EMMS just before you call any GL functions.

EMMS is just for MMX not ?

{$ifdef SAFECALL}
  movups xmm1, [POINT]   // xmm1 = ???????? | Point->z | Point->y | Point->x
{$else}
  movaps xmm1, [POINT]   // xmm1 = ???????? | Point->z | Point->y | Point->x
{$endif}     

good idea just one question what does the abbreviation atm mean?

[dicepd]

atm --> At the moment, (now)

[dicepd]

Ok this is now checked in, it may not seem like a lot of work for the time but I have some interesting results in CreateVec3, most of my time was spent in seeing if eliminating "andps constant" made a difference, and where there are a few used it is certainly so. Shufps seems cheaper than mem access.

Avx variants done as well, will now try to track down the det bug and get a full set of green balls before I continue with functional tests. THmgPlane has a good starting set of where I would like to see the functional tests get to.

[dicepd]

I now have a complete set of green balls and all tests pass for all variants of SSE in Unix64.

Matrix was a few shuffles and using NoW mask where it was not needed.

AVX now although from all my tests on my older hardware CPU Info: AMD FX(tm)-8120 Eight-Core Processor CPU Features: 3DNow / 3DNowExt / MMX / EMMX / SSE / SSE2 / / / / / SSE4A / / / FMA4 SSE3 seems to be the quickest option for me.

[jdelauney]

For me all test are now green except 1 :

THmgPlaneFunctionalTest.TestNormalizeSelf ==> TGLZHmgPlane:NormalizeSelf:Sub2 planes do not match

Dont't say why yet

[dicepd]

If you check in your 64bit code I will take a look later

[jdelauney]

i've just make copy of your's Unix64 to Win64 SSE TGLZHmgPlane. I've checked the s file but see nothing wrong. I'll check more deeper tomorrow

[jdelauney]

Thanks Peter i had eyes closed yesterday, when i'm showing the little change i'v forgetted :) Now all test are Green 👍

[dicepd]

You will have to make some local changes in the new extrafpc.cfg file I just checked in. Deatils in the check in comment e143991

[dicepd]

Ok you may need to issue the following for this not to be tracked local on your machine

git update-index --assume-unchanged VectorClassesUnitTest/extrafpc.cfg

sometime git is a pain!!!!

taken from https://stackoverflow.com/questions/18570926/do-git-update-index-assume-unchanged-rules-propagate-to-clients-on-pull seems there is no simple way to force this behavior from the central repo.