Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ImDrawList::PathArcToAdaptive - path arc that automatically adjust precision according to arc radius #3491

Closed
wants to merge 6 commits into from

Conversation

thedmd
Copy link
Contributor

@thedmd thedmd commented Sep 24, 2020

This PR adds ImDrawList::PathArcToAdaptive(). An implementation of PathArcTo that automatically adjust number of used samples according to used arc radius.

Implementation use 32 element lookup table to fill arc content. On worst case scenario calculate two sampling points when start or end angles are not in lookup table. 32 samples cover radii up to 80 pixels.

To complete this PR decision to call PathArcToAdptive instead of PathArcTo by default in AddXXX functions should be made.

I'm looking for your feedback.

Node: Demo in DO NOT MERGE commit may not work out of the box.

Drawing results

image

Benchmark results

Notice a jump around 80 pixel radius. This is where fallback to PathArcTo kick in.

With PathArcToFast

bench_2_25 bench_2_50
bench_2_75 bench_2_100

With PathArcTo

bench_25 bench_50
bench_75 bench_100

@thedmd
Copy link
Contributor Author

thedmd commented Sep 25, 2020

Edit: It turns out it is easy to make mistake. So I did.

I added PathArcToFast2 which also use lookup table with 32 samples. It outperform PathArcToFast for radii < 10.
PathArcToFast2 isn't outperforming PathArcToFast, but always generate smooth looking output.

image

image image
image image
void ImDrawList::PathArcToFast2(const ImVec2& center, float radius, int a_min, int a_max, int a_step)
{
    if (radius == 0.0f || (a_min >= a_max) || (a_step < 0))
    {
        _Path.push_back(center);
        return;
    }

    if (a_step == 0)
    {
        // Determine step size between samples. Small radii usually allow to skip more than
        // one sample, reduce geometry complexity without harming visual output.
        const float error_for_radius = IM_DRAWLIST_ADAPTIVE_ARC_RADIUS_ERROR_CALC(IM_DRAWLIST_ADAPTIVE_ARC_SAMPLES, radius);
        if (_Data->CircleSegmentMaxError > error_for_radius)
            // Reduce step, but never go lower than 2 samples per quarter of the circle.
            a_step = ImMin((int)ImSqrt(_Data->CircleSegmentMaxError / error_for_radius), IM_DRAWLIST_ADAPTIVE_ARC_SAMPLES / (4 * 2));
    }

    const int  distance = a_max - a_min;
    const bool extra_max_sample = ((a_max - a_min) % a_step) != 0;

    const int samples = (distance) / a_step + (extra_max_sample ? 1 : 0);

    _Path.reserve(_Path.Size + samples);

    for (int a = a_min; a <= a_max; a += a_step)
    {
        const ImVec2& c = _Data->AdaptiveArcVtx[a % IM_ARRAYSIZE(_Data->AdaptiveArcVtx)];
        _Path.push_back(ImVec2(center.x + c.x * radius, center.y + c.y * radius));
    }

    if (extra_max_sample)
    {
        const int a = a_max;
        const ImVec2& c = _Data->AdaptiveArcVtx[a % IM_ARRAYSIZE(_Data->AdaptiveArcVtx)];
        _Path.push_back(ImVec2(center.x + c.x * radius, center.y + c.y * radius));
    }
}

@thedmd
Copy link
Contributor Author

thedmd commented Sep 25, 2020

Few tweaks and PathArcToFast2, integrating adaptive arc with PathArcTo (used when num_segments == 0), bumping number of samples in table to 288 and I think it is a decent replacement.
Note: PathArcToFast is now mapped to PathArcToFast2.

Everything is sampled 10000 times, repeated 10 times and then averaged.

PathArcToFast always output same amount of geometry
PathArcToFast2 adjust amount geometry according to radius, so it will be slower for larger arcs.

Most common radii are around 10.

image image
image image

What is interesting PathArcTo is as performant as PathArcToFast2.

image image

To try it by yourself replace yoursimgui_demo.cpp with one from this branch. You will be able to see performance of current version. Or just build this PR and you will see performance of proposed version. Keep in mind that this version emit more geometry for larger arcs and less for small arcs.

You can checkout spreadsheet with data, there are also more charts inside (file is in ods format):
bench_stats_cmp.zip

Feedback will be appreciated.

@thedmd thedmd force-pushed the feature/draw_path_arc_adaptive branch from 827cf74 to b6a56db Compare September 28, 2020 14:13
@thedmd
Copy link
Contributor Author

thedmd commented Sep 28, 2020

Rebased and squashed to make it easier to view.

@thedmd
Copy link
Contributor Author

thedmd commented Nov 28, 2020

Rebased on master. It is green now.

@ocornut
Copy link
Owner

ocornut commented Dec 1, 2020

Thank you very much. I'm hoping to adopt that shortly after 1.80.

@thedmd thedmd force-pushed the feature/draw_path_arc_adaptive branch from 338fb46 to cab8ab6 Compare December 25, 2020 18:21
@thedmd thedmd force-pushed the feature/draw_path_arc_adaptive branch from cab8ab6 to fc5da7a Compare January 5, 2021 10:38
@ocornut ocornut added this to the v1.81 milestone Jan 5, 2021
@ocornut
Copy link
Owner

ocornut commented Feb 8, 2021

Quick question:
you added in PathArcTo():

    if (a_min >= a_max)
        return;

What's the reasoning for it?

@ocornut
Copy link
Owner

ocornut commented Feb 8, 2021

if (a_min >= a_max)
return;

That matches the equivalent test in PathArcToFast(). But it's technically an API breakage which I don't mind if necessary but seems like we could just swap values? Otherwise need to document as breaking.

@thedmd
Copy link
Contributor Author

thedmd commented Feb 8, 2021

Values can be swapped. It was a guard to prevent invalid input values.

@thedmd thedmd force-pushed the feature/draw_path_arc_adaptive branch from fc5da7a to 7ff027d Compare February 13, 2021 18:45
@thedmd
Copy link
Contributor Author

thedmd commented Feb 13, 2021

PR was rebased on #3808, cleaned up and grew a need for feedback.

At the moment I introduced PathArcTo2 and PathArcToFast2, leaving current implementation untouched for benchmarking purposes. Pushing math work in #3808 was beneficial and allowed me to simplify implementation and get rid of "work on stardust" factor.

One thing I do want to address and need the feedback the most is the bool anti_aliased function parameter. Anti-Aliased geometry use smaller max error value to produce smoother shapes. This is also true for arcs. PathXXX primitives does not have knowledge how they're about to be used. AddXXX functions on the other hand do.
The question is, how can I get rid of this parameter? Should I use same max error value everywhere? Should AA flags in ImDrawList be unified into one? Some other solution? Please share your thoughts.

Also updated benchmark results. There will be more after technical issued get addressed.
image

@thedmd thedmd force-pushed the feature/draw_path_arc_adaptive branch 2 times, most recently from 3190c38 to 8be757e Compare February 16, 2021 21:16
@thedmd
Copy link
Contributor Author

thedmd commented Feb 16, 2021

I made a test using ImDrawList::PathRect(). Made a copy of it called PathRect2 and replaced calls to PathArcToFast with PathArcToFast2 which is an adaptive version. PathRect call arc function four times. Rounding radius is always maximum possible value, which is worst case scenario for rounded rect. This also make them look like circles:

The Benchmark

Each PathXXX function was run 10000 times, after that PathStroke or PathFill were called also 10000. After that was done, whole operation was repeated 50 times and results averaged. So for each radius test each function was called half a milion times. Benchmark is restricted to single core. And there are the results:

Results

We have four graphs in this order:

Graph Comment
PathRect2 - using _PathArcToFastEx Outlined, Adaptive rounding
PathRect - current Outlined, Each corner emit 4 points
PathRect2 - using _PathArcToFastEx Filled, Adaptive rounding
PathRect - current Filled, Each corner emit 4 points
(Sorry for not naming them differently.)

Worst case

Worst case scenario: rounding radius = 0.25 * rect_width
image
(Image is rotated to avoid excessive scrolling).
image

More realistic case

Less cruel example, where rounding is halved. rounding radius = 0.25 * rect_width
image
image

Conclusions

Overall, adaptive arcs are on pair with current implementation. They are faster for small radii, on pair with most commonly used ones and became slower as number of vertices grow. Please take a note that we're in microseconds (1e-6) realm while reading graphs.

Arc generation to PathStroke/PathConvexFill ratio is 1:2. And it as number of vertices grow ratio approach 1:5, this is for radius 116.

Summary

I'm open for any feedback you can have. Branch currently have a benchmark code embedded in image_demo.cpp. You can build it and test it yourself. There are more test cases for arc in the code, but disabled. Demo can generate CSV file for enabled benchmarks.

After few days waiting for feedback I will move forward to wrap up this PR by replacing current implementation.

Thanks!

Edit:
One extra perf graph for 90° arc length. Most commonly used by rounded rect.
image

@thedmd thedmd force-pushed the feature/draw_path_arc_adaptive branch from 86c58a0 to cfa7862 Compare February 17, 2021 13:41
@thedmd
Copy link
Contributor Author

thedmd commented Mar 2, 2021

Commits 42923c6 and d836189 are the essence of this PR. Remaining two are benchmark harness and fixup after replacing original implementation.
First two can be squashed together.

I think, PR is ready to be reviewed.

ocornut added a commit that referenced this pull request Mar 11, 2021
…dius < 0.0f. (#3491) + changed poor-man ceiling in _CalcCircleAutoSegmentCount() to use 0.999999f to reduce gaps

Previously it sorts of accidentally worked but would lead to counter-clockwise paths which and have an effect on anti-aliasing.
ocornut pushed a commit that referenced this pull request Mar 11, 2021
@ocornut
Copy link
Owner

ocornut commented Mar 11, 2021

Merged (had lots of privates discussions leading to this), thanks for your amazing work.

I'll close this. As you know part of the demo/test bed may be worth including in either Demo or our Test Facilities in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants