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

Anti-aliased rendering #133

Closed
phicore opened this issue Feb 13, 2015 · 34 comments
Closed

Anti-aliased rendering #133

phicore opened this issue Feb 13, 2015 · 34 comments

Comments

@phicore
Copy link

phicore commented Feb 13, 2015

It would really be nice if imgui would use nanovg as a backend for drawing widgets.
As it seems now the backend already receive a list of triangle to render. I suppose then the integration with nanovg would need to be done at an upper layer.

What do you think ?

I already use nanovg for an user interface but I use pure C for the imgui and it is less rich than your library but it definitively give nice result.

@ocornut
Copy link
Owner

ocornut commented Feb 13, 2015

Can you clarify, to what end would you want to use NanoVG ?

If it is to render the triangle lists output by ImGui. You can do already do this since the rendering of those lists is your responsibility. However there would be no point rendering the triangles via NanoVG, they won't look any better. But for the sake of integration with an existing codebase it could be nice to provide stock function that uses NanoVG or bgfx (this function would be 30 lines of code).

If what you desire are anti-aliasing shapes such as the rounded rectangles, it would need to be done at a lower level.

I intend to implement more consistently anti-aliased primitives in ImGui but the implementation for that would need to stay very performant. Naively adding NanoVG in the middle of ImDrawList would affect performance, and although possible my intuition is that reimplementing anti-aliased primitives based on techniques used by NanoVG would be better. (In fact Mikko the author of NanoVG has done so in an ImGui branch).

So again I am curious to know exactly to what end you desire to include NanoVG in the loop.

@phicore
Copy link
Author

phicore commented Feb 13, 2015

The intent is to use imgui rich and clear API to render antialiased vectorized GUI. If there is already a fork using nanovg i will look at it. Thanks a lot.

@phicore
Copy link
Author

phicore commented Feb 13, 2015

Indeed mikko's fork gives already nice results
https://github.com/memononen/imgui

image

On my side I did not observe a real performance hit (in opengl)

Do you intend adding mikko's change in your repository ?

@ocornut
Copy link
Owner

ocornut commented Feb 13, 2015

I won't merge them as is, but something similar and based on this will be integrated eventually. For rounded rectangles we'll use textures to reduce vertex count, which will also enable us to use rounded frames as the default.

There is a performance hit. It's not major but it exists and I need to measure it more carefully before taking on further step. It's just that it's not at the top of priority list right now. Mikko sent me an e-mail with infos because he knows about those stuff well but I haven't had time to dig on any of it yet.

However I see no harm in using this branch if you want the anti-aliased lines right away (preferably kept up to date, thats a rather old base it is using at the moment).

@ocornut ocornut changed the title Integration with nanovg Smooth rendering (was: Integration with nanovg) Mar 5, 2015
@ocornut
Copy link
Owner

ocornut commented Mar 6, 2015

I have merged and moved Mikko changes into a new branch until they are sorted out
https://github.com/ocornut/imgui/tree/2015-03-antialiased-primitives

@adamdmoss
Copy link
Contributor

IMHO this is pleasant, most noticeable in apps that use graphs and lines a lot (or probably almost any app on a low-DPI display).

One downside is that it seems pretty consistently about 5% slower than master imgui. That's not a big deal considering how fast imgui is anyway :) but it's a bit surprising that it's even measurable given how fast my* GPU is.

(* or any modern)

@ocornut
Copy link
Owner

ocornut commented Mar 22, 2015

We need to turn all the rendering into using indexed vertices, it'll save on both front.

@ocornut
Copy link
Owner

ocornut commented Apr 9, 2015

I have changed the rendering to output indexed primitives. Reduced vertices by about 35%.
Unfortunately this causes a fair amount of breakage in the "user provided" render loop.

https://github.com/ocornut/imgui/tree/2015-04-indexed-rendering

So I'll only merge that into trunk along with bigger changes such as the AA rendering or new design, and we'll take the opportunity to change the signature of the render function and provide more information/tools to the user so that code can be smaller.

ImGui could flatten the vertices/indices into a contiguous buffer for you. Considering just doing that to make it easier for the user. The reason I'm not doing it is that some user may want to change the vertex format and then it becomes 2 cpu passes over the vertices instead of one.. so that becomes a bit of a waste.

@ocornut ocornut changed the title Smooth rendering (was: Integration with nanovg) Anti-aliased rendering Apr 9, 2015
@ocornut
Copy link
Owner

ocornut commented Apr 9, 2015

Did some more fixes to the AA branch and added a quick hack to easily toggle with and without AA for measurement (but all path building logic still runs).

Some quick measurement. Not super thorough but gives some useful info.
Test consist in opening all demo windows + the graph pane + the app examples panes. Measurement with the DX11 runtime (screenshot shows DX9, but gets you an idea).
all
That's about 10-13 windows open including child windows. They aren't doing a lot of stuff tho.
The time are for the entire application loop, including polling messages, clearing, rendering, swapping.

Release Build
all 0.57 ms - trunk
all 0.58 ms - indexed-rendering branch
all 0.61 ms - AA branch
all 0.57 ms - AA branch with AA disabled by runtime flag

For reference, with only 1 imgui window opened the time with trunk code is 0.42 ms, it seems to be roughly the maximum framerate I can get on my computer with DX11 at this window size (resizing the main window smaller/bigger).

Debug Build (DX11)
all 1.80 ms - trunk    (render func only ~0.25 ms), 11900 vtx
all 1.55 ms - indexed-rendering branch (render func only ~0.25 ms),     7822 vtx, 11900 idx
all 2.23 ms - AA branch   (render func only ~0.37)    16527 vtx
all 1.85 ms - AA branch with AA disabled by runtime flag,   11500 vtx

What does that mean:

  • With AA enabled I think the code is little a little too slow in debug but it's not a deal breaker and it's slower in every branches (I need to put more work into optimising debug builds).
  • With AA disabled at runtime the code appears to be fast enough. That's good news it means that the AA branch could be merged into master pretty soon, it can be off by default until some optimisations and testing / tweaks are done but that's a good step forward.

And it will get better when we can merge indexed rendering into the AA branch (it will benefit greatly from it).

I still need to sort out some issues with lines alignment, some things are off by a pixel.

@ocornut
Copy link
Owner

ocornut commented Jun 30, 2015

I have merged the indexed rendering branch into the anti-aliased branch, and spent some more time doing optimisations, now rather satisfied with the result.

anti aliased

@ocornut
Copy link
Owner

ocornut commented Jul 6, 2015

Done some further optimisations. Measuring in Debug and testing with some fairly loaded screens, the AA branch with AA enabled is now slightly faster than Master at rendering.
Of course it is biased because I haven't done as much of the finer optimisations on Master.

Opening all the windows from the demo, within the AA branch:

  • In Debug builds (msvc 2010 with stack checking, etc.) with a very charged screen, AA enabled adds about 14% cpu cost (+0.25 ms)
  • In Optimised builds the difference is negligible.
  • Enabling frame rounding has a rather heavy cost typically +20-30% for charged screen (was always the case before). It could probably be zeroed by drawing those rounded shapes in the texture atlas so there's room for optimisation here, later.

Note that those costs don't grow with clipped objects (outside of screen, outside of scrolled view), a list with thousands of items won't be affected. It only grows up to how much can fit on the screen.

Again those worse cases numbers are better than the current Master, so nothing to worry about.

ocornut added a commit that referenced this issue Jul 6, 2015
… neater and future proof breaking of the render API (#133 #254)
ocornut added a commit that referenced this issue Jul 6, 2015
@ocornut
Copy link
Owner

ocornut commented Jul 6, 2015

I have committed the render API breaking changes that should now be more future proof.
(It was already broken in this branch, but now it is broken in a way that makes more sense for future updates)

If you are feeling like testing the anti-aliased branch now, please do it and give me feedback!
I'd prefer to give you no further information and see if any of the changes are confusing!

I would like to know if the breakage makes sense, what's your experience with updating, etc. thanks!

@ocornut
Copy link
Owner

ocornut commented Jul 7, 2015

While I'm breaking this I may just rename all the fields in ImDrawList to use PascalCase to be consistent with the other types. It always bothered me that they used a lower_case_with_underscore because ImDrawList is a proper class with functions and it makes.
It's now or never so I'd rather do it now.

@emoon
Copy link
Contributor

emoon commented Jul 7, 2015

A few questions:

  1. When do you think this will go into master?
  2. Is the AA stuff all or nothing? Meaning is it still possible to use non-AA in places where one doesn't want it?

@Pagghiu
Copy link
Contributor

Pagghiu commented Jul 7, 2015

I see a couple of runtime style flags (AntiAliasedLines and AntiAliasedShapes) that I assume do what their name suggests.

@emoon
Copy link
Contributor

emoon commented Jul 7, 2015

Ah nice :)

@ocornut
Copy link
Owner

ocornut commented Jul 7, 2015

It's pretty much done, I may decide to do the UpperCase renaming of all fields in ImDrawData, ImDrawList but other than that not much, need some testing on people apps mainly.

  1. So probably within 1 or 2 weeks.
  2. See above it is part of the style (missing the push/pop enums yet). I'm doing a few things like axis-aligned filled rectangles don't do through AA path already, etc. shaving a bit of time for charged screen.

@emoon
Copy link
Contributor

emoon commented Jul 7, 2015

Sound good! 👍

ocornut added a commit that referenced this issue Jul 8, 2015
…mDrawData to match the rest of our coding convention (#133)
@ocornut
Copy link
Owner

ocornut commented Jul 8, 2015

OK i went and done the renaming I wanted to do. This is mildly annoying to the user but it's the final touch to the breaking changes of that branch. Most programmers should be able to fix everything in 5 minutes. HOPEFULLY. I can still revert that change if a riot start.

d03b046

 - 2015/07/07 (1.42) - switched rendering data to use indexed rendering. this is saving a fair amount of CPU/GPU and enables us to get anti-aliasing for a marginal cost.
                       this necessary change will break your rendering function! the fix should be very easy. sorry for that :(
                     - if you are using a vanilla copy of one of the imgui_impl_XXXX.cpp provided in the example, you just need to update your copy and you can ignore the rest.
                     - the signature of the io.RenderDrawListsFn handler has changed! 
                            ImGui_XXXX_RenderDrawLists(ImDrawList** const cmd_lists, int cmd_lists_count)
                       became:
                            ImGui_XXXX_RenderDrawLists(ImDrawData* draw_data). 
                              argument   'cmd_lists'        -> 'draw_data->CmdLists'
                              argument   'cmd_lists_count'  -> 'draw_data->CmdListsCount'
                              ImDrawList 'commands'         -> 'CmdBuffer'
                              ImDrawList 'vtx_buffer'       -> 'VtxBuffer'
                              ImDrawList  n/a               -> 'IdxBuffer' (new)
                              ImDrawCmd  'vtx_count'        -> 'ElemCount'
                              ImDrawCmd  'clip_rect'        -> 'ClipRect'
                              ImDrawCmd  'user_callback'    -> 'UserCallback'
                              ImDrawCmd  'texture_id'       -> 'TextureId'
                     - each ImDrawList now contains both a vertex buffer and an index buffer. For each command, render ElemCount/3 triangles using indices from the index buffer.
                     - if you REALLY cannot render indexed primitives, you can call the draw_data->DeIndexAllBuffers() method to de-index your buffer. This is slow and a waste of CPU/GPU. Prefer using indexed rendering!
                     - refer to code in the examples/ folder or ask on the GitHub if you are unsure of how to upgrade. please upgrade!
                     - removed the 'thickness' parameter from ImDrawList::AddLine().

With that I'm pretty much ok to merge that in master after 2-3 people have tested it (I'll integrate it in a big codebase I have access to first).

Maybe provide a monospaced AA font as well ?

I may need to re-add the thickness feature of AddLine(), or direct it to non-anti-aliased path for now. It's not hard to add in the AA path but quite a fair amount of extra code. Edit Done

@unpacklo
Copy link

unpacklo commented Jul 8, 2015

I just tested it on my end and it was a very easy and trivial change! I am getting some warnings on my Linux machine, though:

clang++ -I../../ `pkg-config --cflags glfw3` -Wall -c -o main.o main.cpp
clang++ -I../../ `pkg-config --cflags glfw3` -Wall -c -o imgui_impl_glfw.o imgui_impl_glfw.cpp
clang++ -I../../ `pkg-config --cflags glfw3` -Wall -c -o ../../imgui.o ../../imgui.cpp
../../imgui.cpp:11149:33: warning: cast to 'void *' from smaller integer type 'int' [-Wint-to-void-pointer-cast]
                ImGui::TreePush((void*)i);
                                ^
../../imgui.cpp:11190:37: warning: cast to 'void *' from smaller integer type 'int' [-Wint-to-void-pointer-cast]
                if (ImGui::TreeNode((void*)i, "Child %d", i))
                                    ^
2 warnings generated.
clang++ -o imgui_example main.o imgui_impl_glfw.o ../../imgui.o -I../../ `pkg-config --cflags glfw3` -Wall `pkg-config --static --libs glfw3`
Build complete for Linux
g++ -I../../ `pkg-config --cflags glfw3` -Wall -c -o main.o main.cpp
g++ -I../../ `pkg-config --cflags glfw3` -Wall -c -o imgui_impl_glfw.o imgui_impl_glfw.cpp
g++ -I../../ `pkg-config --cflags glfw3` -Wall -c -o ../../imgui.o ../../imgui.cpp
../../imgui.cpp: In member function ‘void ImDrawList::AddPolyline(const ImVec2*, int, ImU32, bool, bool)’:
../../imgui.cpp:9211:9: warning: variable ‘start’ set but not used [-Wunused-but-set-variable]
     int start = 0, count = points_count;
         ^
../../imgui.cpp: In function ‘void ImGui::ShowTestWindow(bool*)’:
../../imgui.cpp:11149:40: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
                 ImGui::TreePush((void*)i);
                                        ^
../../imgui.cpp:11190:44: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
                 if (ImGui::TreeNode((void*)i, "Child %d", i))
                                            ^
g++ -o imgui_example main.o imgui_impl_glfw.o ../../imgui.o -I../../ `pkg-config --cflags glfw3` -Wall `pkg-config --static --libs glfw3`
Build complete for Linux

ocornut added a commit that referenced this issue Jul 8, 2015
@thevaber
Copy link
Contributor

thevaber commented Jul 8, 2015

I also tested this today - necessary changes were extremely straightforward and quick (except maybe for a function I use to draw linear gradient quads which uses internal ImDrawList stuff, but that's very clearly marked as internal, so that doesn't really count), everything looks great, didn't notice any issues at all and there seems to be zero performance impact even in debug builds - great work!

@ocornut
Copy link
Owner

ocornut commented Jul 8, 2015

Nice. Today I have merged in SDL and Allegro 5 sample to it was reassuring to see it all work. Allegro 5 doesn't support 16-bit indices or 32-bit colors but then it's a rather old and clunky library. Easy to convert the data.

Perhaps I should re-add the PrimVtx and PrimIdx functions for that sort of custom use?

@thevaber
Copy link
Contributor

thevaber commented Jul 8, 2015

These could indeed be useful to avoid breaking changes in the future, maybe together with overloads (or something like "PrimVtxs" and "PrimIdxs") for arrays of ImDrawVerts and ImDrawIdxs; not sure how critical that is though (it's quite simple to update even when the internals change)

@ocornut
Copy link
Owner

ocornut commented Jul 8, 2015

I have re-added PrimVtx() plus lower-level helpers now. Don't want to surchage this so I won't add array versions but yeah it should be trivial.

ocornut added a commit that referenced this issue Jul 15, 2015
@ocornut
Copy link
Owner

ocornut commented Jul 15, 2015

Now merged in and we have a more flexible font API: oversampling, horizontal sub-pixel positioning, ability to merge glyphs from multiple fonts into one font and various extra options.

I'll let it sit in master for a few days I assume there will be reactions/issues since it's quite a breaking change.

@Pagghiu
Copy link
Contributor

Pagghiu commented Jul 15, 2015

In the next days I will branch my app applying this update on flexible fonts api and AA / Indexed rendering changes, reporting here any problems.

@ApoorvaJ
Copy link

FYI: Sean Barrett pushed stb_truetype.h v1.06 today, with significantly improved rasterization with AA. Might be pertinent here.

ocornut added a commit that referenced this issue Jul 15, 2015
@ocornut
Copy link
Owner

ocornut commented Jul 15, 2015

Updated!

@ocornut
Copy link
Owner

ocornut commented Jul 16, 2015

Will release this as 1.43 tomorrow morning and finally close this topic!

@Pagghiu
Copy link
Contributor

Pagghiu commented Jul 16, 2015

Updating my apps with latest master works with one exception. Stb_truetype 1.06 crashes on one assertion when adding font awesome.
Manually Replacing 1.05 solves the crash.

@ocornut
Copy link
Owner

ocornut commented Jul 16, 2015

Do you have a repro? or - which setting are you using? I tried various combination and didn't get an assert firing.

On updating to indexed rendering - did you run into any issue or confusing thing that you think could have been easier? I need to keep in mind that many users are not as proficient in C/C++ as we may be, some are just learning programming so any confusion that anyone here has is mulitplied by 100 with a junior programmer and I'd like to smooth them out.

@ocornut
Copy link
Owner

ocornut commented Jul 17, 2015

Closing this!

@ocornut ocornut closed this as completed Jul 17, 2015
@ocornut ocornut reopened this Jul 17, 2015
@ocornut
Copy link
Owner

ocornut commented Jul 17, 2015

Re-opening this just so I can close it AGAIN.

@ocornut ocornut closed this as completed Jul 17, 2015
@emoon
Copy link
Contributor

emoon commented Jul 17, 2015

Double close just to be safe! :)

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

No branches or pull requests

8 participants