[resolved] Divide by zero error with convexshape

[Vanilla]

I think I found a bug, didn't it in the issues list or on the forum.  sfml version 2.3.2

The short version:
If you have a convex shape with 3 or more points, but only one of the points has value (other than zero) then shape.updateOutline() will attempt to divide by zero and crash.  Ex: a shape with points (3,4) (0,0) (0,0) will cause a crash.

Well, that's not much of a shape, it's really just a line segment with an extra point.  The trigger is that convexshape.setpoint() immediately calls shape.update(), which in turn calls the updateOutline function.

So the code that looks like this (pascal bindings, but that doesn't matter):

var
testShape : TSfmlConvexShape;  //variable of type convex shape

begin
testShape := TSfmlConvexShape.Create;      //calls convexshape constructor
testShape.PointCount := 3;                               //calls convexshape.setpointcount
testShape.Point[0] := SfmlVector2f(3,4);       // calls convexshape.setpoint
...

crashes and burns when executing that last line.

I went through the source code and this is what happens:

When the point count is set to three, convexshape.m_points is resized and filled with zeroes. It looks like this:
(0,0) (0,0) (0,0)

When convexshape.setpoint() is called, m_points is set to this
(3,4) (0,0) (0,0)

and immediately calls shape.update().  Looking through shape.update() then.  The vertex vector in shape will have five vertices.  It takes the three points above and puts them at indices 1,2, and 3 respectively.  Index 4 repeats index 1 and index 0 gets the center of the shape.  So m_vertices looks like this:
(1.5, 2) (3,4) (0,0) (0,0) (3,4)

Code execution proceeds to updateOutline which is where the crash will happen.  UpdateOutline() starts with the three points and calculates the normals.  On the first pass through the loop:
p0 = (0,0)
p1=(3,4)
p2 = (0,0)

n1 = (-0.8, +0.6)  //calculated from p1 and p0 per the compute normal function
n2 = (+0.8, -0.6) //same, but with p1 and p2

Then there is a dot product check to verify the normal direction. This calculates the dot product between each normal and a vector from the point p1 to the center of the shape (m_vertices[0] ).  Since this shape is only a line segment, the normals will be 90 degrees from the vector (point to center) and the dot product will always be zero.  So the dot product check does nothing to the normal values above.

The function then computes a variable called factor which is 1 + (n1.x * n2.x + n1.y *n2.y).  This evaluates to zero, and the next line divides by factor, which causes a divide by zero error.

I'm not sure of a fix.  A simple check to verify factor != 0 would work, but I don't know what the function should do in that case.  Pulling the call to update() out from the convexshape setpoint function would also work, but then folks would have to remember to call update() manually after all the points have been loaded and any time the point positions are set again.   


Resolved:
It turns out the code does perform a divide by zero.  But the values are all floats, and floating point divide by zero is represented by a floating point NaN, or +Inf, or -Inf.  The C programming language doesn't treat this as an exception (C99 and later standards), and I assume C++ doesn't either.  Pascal, on the other hand, does treat this as an exception and halts program execution.  The workaround is to disable this exception in the compiler (may not be available in all compilers, but not relevant to this issue).  The Pascal bindings probably need a fix or at least a note to this effect.

Hope this helps,
Vanilla

[eXpl0it3r]

I unfortunately can't reproduce this with the master branch, but maybe I didn't fully understand how exactly you managed to create the problem. So complete (C++) code example would be useful.

Here's what I've run.

#include <SFML/Graphics.hpp>

int main()
{
    sf::RenderWindow window({800, 600}, "Test");
    window.setFramerateLimit(60);

    sf::ConvexShape polygon;
    polygon.setPointCount(3);
    polygon.setPoint(0, sf::Vector2f(0, 0));
    polygon.setPoint(1, sf::Vector2f(0, 10));
    polygon.setPoint(2, sf::Vector2f(25, 5));
    polygon.setOutlineColor(sf::Color::Red);
    polygon.setOutlineThickness(5);
    polygon.setPosition(10, 20);

    while(window.isOpen())
    {
        sf::Event event;
        while(window.pollEvent(event))
        {
            if(event.type == sf::Event::Closed)
                window.close();
        }

        window.clear();
        window.draw(polygon);
        window.display();
    }
}
 

Even setting every point deliberately to (0, 0) doesn't cause a crash.

[Vanilla]

Thanks you for the help in trying to reproduce the error.

If my analysis was correct, it wouldn't trigger by setting a point to (0,0) but it should trigger on the first non-zero point that is set.  Point (0,10) in your code.  Since you do not encounter the error, it is more likely something wrong on my end. 

I'm off to dig into this further.

Vanilla

[eXpl0it3r]

I set it to a non-zero value, but I still am not experiencing any crash. You said you're using SFML 2.3.2 while I'm using SFML master, is there a chance you could test SFML master?

[Vanilla]

Sorry, I just edited the first post with the resolution rather than posting again.

The updateOutline() function does perform a floating point divide by zero, but it turns out C (and C++) won't raise an exception.  Floating point registers have enough bits to represent NaN, +Inf, and -Inf.   C++ programs will just keep going.  Pascal treats a floating point divide by zero as an exception and will halt execution.

A user will typically write set several points in a row and each time a point is set the whole outline is recalculated, so the NaN will be overwritten once two or three points are set.   The Pascal bindings should be probably updated to disable that exception or disable access to the ConvexShape code.

Vanilla