-
-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
Texture-based paths for thick anti-aliased lines #3245
Comments
Here are my test results for ImPlot: Computer
Test Setup Description
Test Codefloat RandomRange(float min, float max) {
float scale = rand() / (float)RAND_MAX;
return min + scale * (max - min);
}
struct BenchmarkItem {
BenchmarkItem() {
float y = RandomRange(0, 1);
Data = new ImVec2[1000];
for (int i = 0; i < 1000; ++i) {
Data[i].x = i * 0.001f;
Data[i].y = y + RandomRange(-0.01f, 0.01f);
}
Col = ImVec4(RandomRange(0, 1), RandomRange(0, 1), RandomRange(0, 1), 1);
}
~BenchmarkItem() { delete Data; }
ImVec2* Data;
ImVec4 Col;
};
static void ShowImPlotBenchmark() {
ImGui::SetNextWindowPos(ImVec2(0, 0), ImGuiCond_Always);
ImGui::SetNextWindowSize(ImVec2(1400, 850));
ImGui::Begin("ImPlot Benchmark");
static const int n_items = 100;
srand(0);
static BenchmarkItem items[n_items];
ImGui::BulletText("%d lines with %d points each @ %.3f FPS.", n_items, 1000, ImGui::GetIO().Framerate);
ImPlot::SetNextPlotLimits(0, 1, 0, 1, ImGuiCond_Always);
if (ImPlot::BeginPlot("##Bench", NULL, NULL, ImVec2(-1, -1), ImPlotFlags_Default)) {
char buff[16];
ImPlot::PushStyleVar(ImPlotStyleVar_LineWeight, 1);
for (int i = 0; i < 100; ++i) {
sprintf(buff, "item_%d", i);
ImPlot::PushStyleColor(ImPlotCol_Line, items[i].Col);
ImPlot::PlotLine(buff, items[i].Data, 1000);
ImPlot::PopStyleColor();
}
ImPlot::PopStyleVar();
ImPlot::EndPlot();
}
ImGui::End();
} ResultsI made the comparison between You can overlay the images below in Photoshop and see that there is a very tiny difference between the two in some areas, but not enough to concern me.
|
Thanks (again) for your amazing feedback :)
Have you tried disabling |
I can see some differences, in the following code: static void DrawZigZag( ImDrawList* draw, const ImVec2& wpos, double start, double end, double h, uint32_t color, float thickness = 1.f )
{
const auto spanSz = end - start;
if( spanSz <= h * 0.5 )
{
draw->AddLine( wpos + ImVec2( start, 0 ), wpos + ImVec2( start + spanSz, round( -spanSz ) ), color, thickness );
return;
}
const auto p = wpos + ImVec2( 0.5f, 0.5f );
const auto h05 = round( h * 0.5 );
draw->PathLineTo( p + ImVec2( start, 0 ) );
draw->PathLineTo( p + ImVec2( start + h05, -h05 ) );
start += h05;
const auto h2 = h*2;
int steps = int( ( end - start ) / h2 );
while( steps-- )
{
draw->PathLineTo( p + ImVec2( start + h, h05 ) );
draw->PathLineTo( p + ImVec2( start + h2, -h05 ) );
start += h2;
}
if( end - start <= h )
{
const auto span = end - start;
draw->PathLineTo( p + ImVec2( start + span, round( span - h*0.5 ) ) );
}
else
{
const auto span = end - start - h;
draw->PathLineTo( p + ImVec2( start + h, h05 ) );
draw->PathLineTo( p + ImVec2( start + h + span, round( h*0.5 - span ) ) );
}
draw->PathStroke( color, false, thickness );
} Example 1Some pixels are missing, doesn't really matter here. Example 2Thickness Other than that, everything looks the same. |
Yes, I have. My problem was that I couldn't use
My code is here, the first half using |
@wolfpld:
Could you possibly make two calls to the function, or would the normal artefact at the point of jointure be too much of a problem? I wonder if we can refactor some of that code to facilitate reusing chunks of the code with less of the fixed cost (I don't know if (By the way any reason you still need to enable 32-bit indices?) |
The idea crossed my mind but I haven't tried it yet. The main issue was the visual artifacts (small slivers shooting off screen) I saw in AddPolyline, presumably at the mitereed corners. I think it may have only pertained to the AA code however.
Yes, I have optimizations and inlining enabled. Indeed, PrimReserve should only have an effect the first frame. The biggest gain might be from avoiding the if
For the test above, I saw artifacting if it wasn't enabled. This was done isnide of your test engine with the default backend enabled. |
Yes, there's no visible change there.
Yes, the fuzziness is quite obvious. Attaching un-zoomed comparison below.
Hardcoding this value has the same effect as adjusting fractional part in the |
I am working on a fix for exactly this problem. I plan to submit it a bit later this week. |
To clarify, thickness below <1.0f are and were always treated the same as =1.0f Forgot to mention in the initial post that this comes with a restriction:
(Added to top-most post now) |
No differences here as far as I can tell |
Curious if #3163 fixes it for you? |
@wolfpld Sorry for delayed reaction. We're not sure what's the best approach here. If we should make it automatic or optional to "degrade" to the existing polygon path when thickness are non-integer (worst case we are degrading to existing code, but the various paths are a little bothering.. |
Yeah, I'd really like to have a better solution for fractional line widths - geometry-based edges tend to be reasonably "clear" because the geometry is drawn with separate line/anti-aliasing widths, but as you've observed textured edges are more fuzzy as essentially the code just "crushes" the nearest integer-width texture into a smaller space, effectively uniformly scaling (and blending together) all the elements. We may be able to apply some tweaks (introducing intermediate textures at 0.5 increments for small widths?) to try and minimise that, but the issue I keep coming back to is that regardless of any of that the differences in sampling behavior between geometry edges and texture edges means that any solution based on sometimes drawing lines one way and sometimes drawing them the other is extremely likely to exhibit inconsistencies - we can kinda get away with a bit of that with integer coordinates/widths because (a) in theory the sizes are "right" either way, (b) you have a full pixel worth of step between valid values and (c) they're much more amenable to hand-tuning, but as soon as you have a scenario where there's a cut-off point - e.g. "everything <4px is a texture and everything >=4px is geometry" then there's potential for someone to try and draw a shape that involves a 3.99px (texture) wide line meeting a 4.0px (geometry) wide line with a visible discontinuity. The least-worst solution I've come up in terms of consistency with thus far is to never allow line edges to be geometry, and always ensure that they are textures even if that means adding extra polys in the middle to "fill out" the line... but in terms of the "fuzziness" problem that's actually a step backwards because it would unify everything onto the texture path... :-( |
Works fine on my end. Performance wise i am seeing an increase. No obvious visual differences for me. Rendering takes a roughly half the time as far as i can see, which probably really helps on weaker machines, but i don't have one on had right now :/ |
Ben pushed a tweak to make this branch "non-regressing": texture based lines are currently only used when thickness is an integer value, so we are able to merge it. (edit: clarification, this is now indeed merged) The change would also technically break modification of AA_SIZE as done by thedmd@39cde5c for scaling, so those patches will need to add a Also merged separately of the applicable renaming/comments/shallow stuff introduced with this branch, then rebased over, so the merged branch is as lean as possible. This relatively small changes are not overly exciting right now but they will largely facilitate use of thick borders, and will be one (among many) of the building blocks toward redesigning and improving the style system (thanks to NVIDIA @xteo for their contribution on this and many other upcoming changes aligned with this vision). @epezent ImPlot may be able to make use of the new UV data provided in |
Minor amend, stripped out dead-code since are only using the texture-bsaed based for integer width: 9801c8c |
Closed as implemented/solved. |
We have various changes coming in the low-level drawing pipelines (mostly implemented by Ben aka @ShironekoBen) and for some of them we'll be looking for feedback.
The first one is fairly simple and available in the
features/tex_antialiased_lines
branch:https://github.com/ocornut/imgui/tree/features/tex_antialiased_lines
We made it that lines up to a certain thickness are relying on texture data instead of extra polygon.
Essentially:
Added in style:
bool style.AntiAliasedLinesUseTex
Added in font atlas:
ImFontAtlas: ImFontAtlasFlags_NoAntiAliasedLines
(disable the whole thing which takes about 64x64 pixels or pixel space, for very low memory footprint devices)Restriction:
Feedback wanted:
style.AntiAliasedLinesUseTex = !ImGui::GetIO().KeyShift
as a way to easily compare for difference.This is most probably breaking PR #2964 which we will rework accordingly.
This should merge in
docking
with 1 minor conflict which should be obvious to fix when merging (conflicting comments in nearby lines).Taking the liberty to tag people who I suspect may be interested (writers of node editors or plot widgets):
@rokups @thedmd @inflex @wolfpld @epezent @r-lyeh @soulthreads @mkalte666 @Nelarius
Attaching snapshot of some @epezent earlier tests were they stated:
The text was updated successfully, but these errors were encountered: