AddCircle{Filled} - Use _PathArcToFastEx directly to emit circle vert…

[thedmd]

…ices for 12 segment case (#4419)

Before introduction of adaptive arcs PathArcToFast() were operating on 12 vertices table.
Now it is replaced by internal _PathArcToFastEx() doing same job but with greater granularity,
PathArcToFast was left as a fallback.

[ocornut]

Hello @thedmd,
Thanks for this.
I am not sure I understand your fix. It makes the AddCircle() call with an explicit requests for 12 segment render with 48 segments.

[ocornut]

Actually, the bug reported in #4419:

Comes from the fact that the num_segments parameter is not correctly honored by AddCircle() in the first place.

• It is correct that PathArcToFast() uses "auto tesselation" since we upgraded this API and it never had a "num_segments" parameters.
• But AddCircle() with explicit num_segments should use the explicit count.

Removing the if/else and just calling PathArcTo() yields correct visual result, but lookup tables are never used.

[thedmd]

Hi. This function will calculate appropriate step basing on radius, so it will skip as many vertices as it can.
Cannot heck how big exactly it is, because I'm not near my PC.

[ocornut]

Unless I'm misunderstanding something, It's even worse than I thought:

• AddCircle() with num_segments == 0 calculate the segment count and then pass it to PathArcTo(), which immediately calls _PathArcToN(), so the fast paths are never taken (??)

[thedmd]

12 case was there for compatibility, maybe a misjudged one.

In the end, even without if code will most likely end up using lookup table anyway. Only just a bit latter.

[ocornut]

It actually doesn't:

    // Obtain segment count
    if (num_segments <= 0)
    {
        // Automatic segment count
        num_segments = _CalcCircleAutoSegmentCount(radius);
    }
    else
    {
        // Explicit segment count (still clamp to avoid drawing insanely tessellated shapes)
        num_segments = ImClamp(num_segments, 3, IM_DRAWLIST_CIRCLE_AUTO_SEGMENT_MAX);
    }

    // Because we are filling a closed shape we remove 1 from the count of segments/points
    const float a_max = (IM_PI * 2.0f) * ((float)num_segments - 1.0f) / (float)num_segments;
    if (num_segments == 12)
        PathArcToFast(center, radius - 0.5f, 0, 12 - 1);
    else
        PathArcTo(center, radius - 0.5f, 0.0f, a_max, num_segments - 1);
    PathStroke(col, ImDrawFlags_Closed, thickness);
}

num_segments is calculated so PathArcTo() is called with that value, and PathArcTo() does

    if (num_segments > 0)
    {
        _PathArcToN(center, radius, a_min, a_max, num_segments);
        return;
    }

Which calls cos/sin.
We broke something at some point.

e45847d already had this in PathArcTo(), and AddCircle() already calls PathArcTo().

TL;DR; AddCircleXXX() never uses the optimizations apart accidentally (and incorrectly) in the num_segments==12 case (which is the reported bug that got us to notice this).

[thedmd]

Digging trough the code commits (up to 2015)

1. The original code
   AddCircle{Filled}() was always implemented with PathArcTo().
   One with ImCos and ImSin to be precise. Number of segments (N) was always
   expected to be provided by the called. When N was <= 2, and nothing was drawn.
   N was defaulted to 12 in header.

2. Adaptive segment calculation
   (2020-01-23) 051ce07 "AddCircle, AddCircleFilled: Add auto-calculation of circle segment counts"
   Ben introduced radius based adaptive segment calculation, which run for N <= 0.
   Number of segments were precalculaded in table for some radii. Function
   still use PathArcTo() to emit vertices.

AddNgon{Filled} were introduced which are exact copy of AddCircle from point 1.
Renamed, and without default N set in header.

3. N == 12 optimization
   (2020-01-23) 5363af7 "AddCircle, AddCircleFilled: Add auto-calculation of circle segment counts (amends)"
   (2020-07-09) 89685b3 "ImDrawList: Fixed minor bug introduced in 1.75 where AddCircle() with 12 segments would generate an extra unrequired vertex."

Adaptive segment calculation were followed by a patch that used PathArcToFast()
when user requested 12 segments. Since PathArcToFast() was configured to
use 12 element lookup table, this code worked fine and gave identical results
as slower path.

4. Enabling adaptive segmment calculation by default
   (2020-07-16) e223bd8 "ImDrawList: changed AddCircle(), AddCircleFilled() default num_segments from 12 to 0."

Default argument N == 12 changed to 0, which enabled adaptive segment calculation
for circles.

5. Adaptive arcs
   (2021-02-13) e45847d "Add version of PathArcTo() and PathArcToFast() with adaptive rendering quality. (ImDrawList::PathArcToAdaptive - path arc that automatically adjust precision according to arc radius #3491)"
   _PathArcToFastEx() were introduced.
   PathArcToFast() and PathArcTo() were implemented in terms of _PathArcToFastEx().
   PathArcToFast() honor 0-12 input and map them to _PathArcToFastEx() which in result gave
   smooth arc on bigger radii and around same result on commont radii.

At this point AddCircle{Filled} became smooth for N == 12, except the closing segmennt,
because PathArcToFast(0, 11) got tesselated and step from 11 to 12 was a straight line.

5. Last fix attempt
   (2021-08-11) 499a108 "AddCircle{Filled} - Use _PathArcToFastEx directly to emit circle vertices for 12 segment case ( end segment of circle arc scissoring #4419)"
   PathArcToFast() in AddCircle{Filled} were replaced by _PathArcToFastEx() which
   yeild whole circle tesselated, including last segment.

---

AddCircle{Filled}

AddCircle() N == 12 is broken, because it acts like adaptive version N == 0.
PathArcToFast() generate tesselated geometry

Proposed solution

Force step size in _PathArcToFastEx() call to emit 12 vertices and still use
fast path.

---

PathArcToFast()

PathArcToFast() is used across ImGui code base, but it also user-facing API.

Proposed solution

Rename PathArcToFast() to _PathArcToFast12() and convert ImGui internal calls
to it.

Add PathArcToFast() that always up emit 12 vertices.

[thedmd]

I did an update to PR, where to make AddCircle use _PathArcToFastEx() for adaptive path, and added two fast paths.

I suspect there will be some questions or changes that can be made to further improve this routine.

Note: I made _PathArcToFastEx() to make full circle with begin/end overlapping and then I pop the last vertex. This way circle is vertices will be symmetric.

void ImDrawList::AddCircle(const ImVec2& center, float radius, ImU32 col, int num_segments, float thickness)
{
    if ((col & IM_COL32_A_MASK) == 0 || radius <= 0.0f)
        return;

    if (num_segments <= 0)
    {
        // Use arc with automatic segment count
        _PathArcToFastEx(center, radius - 0.5f, 0, IM_DRAWLIST_ARCFAST_SAMPLE_MAX, 0);
        _Path.pop_back();
    }
#if IM_IS_DIVISIBLE_BY(IM_DRAWLIST_ARCFAST_SAMPLE_MAX, 12)
    else if (num_segments == 12)
    {
        // If IM_DRAWLIST_ARCFAST_SAMPLE_MAX is dividable by 12, we can use _PathArcToFastEx
        // with constant step size.
        _PathArcToFastEx(center, radius - 0.5f, 0, IM_DRAWLIST_ARCFAST_SAMPLE_MAX, IM_DRAWLIST_ARCFAST_SAMPLE_MAX / 12);
        _Path.pop_back();
    }
#endif
#if IM_IS_DIVISIBLE_BY(IM_DRAWLIST_ARCFAST_SAMPLE_MAX, 24)
    else if (num_segments == 24)
    {
        // If IM_DRAWLIST_ARCFAST_SAMPLE_MAX is dividable by 12, we can use _PathArcToFastEx
        // with constant step size.
        _PathArcToFastEx(center, radius - 0.5f, 0, IM_DRAWLIST_ARCFAST_SAMPLE_MAX, IM_DRAWLIST_ARCFAST_SAMPLE_MAX / 24);
        _Path.pop_back();
    }
#endif
    else
    {
        // Explicit segment count (still clamp to avoid drawing insanely tessellated shapes)
        num_segments = ImClamp(num_segments, 3, IM_DRAWLIST_CIRCLE_AUTO_SEGMENT_MAX);

        // Because we are filling a closed shape we remove 1 from the count of segments/points
        const float a_max = (IM_PI * 2.0f) * ((float)num_segments - 1.0f) / (float)num_segments;
        PathArcTo(center, radius - 0.5f, 0.0f, a_max, num_segments - 1);
    }

    PathStroke(col, ImDrawFlags_Closed, thickness);
}

[ocornut]

Merged the main part of the fix for now, thanks!
We'll think together about whether the optimization for divisable by 12 are worth it.

[rollraw]

So, for now we can't use 12 segments count "magic" for cached PathArc?

[thedmd]

Does adaptive version work for you? Is will use fast path and also emit enough vertices to produce smooth result.

[rollraw]

Does adaptive version work for you? Is will use fast path and also emit enough vertices to produce smooth result.

yes it's working, and i already understood that 12 segments count now is just 12 segments count, and 0 is used for both automatic / cached segments circle