Updating backend ImGui/ImPlot/ImNodes to latest versions

[SamuMazzi]

---

name: Updating backend ImGui/ImPlot/ImNodes
about: Updated backends
title: Updating backend ImGui/ImPlot/ImNodes
assignees: ''

---

Description

This PR aims to update the Im* backends of DPG to way more recent versions.
Currently DPG relies on ImGui 1.83 (last commit around May 2021), ImPlot 0.11 (always around that date) and ImNodes (which is the less updated). With this PR we'll have ImGui 1.90.6 and ImPlot 0.17 (for the major updates see below).

Background: ImGui is currently sponsored by big tech companies like Adobe, Blizzard, Activision, etc. so we can expect a lot of improvements with the update (there are already ton of them) and we can expect a lot of people working on the really ugly/hard things like graphical backend libraries.

Why?

In my workplace we are about to ship a product based on DPG. Because of my job, I had to edit the backend and then I found out that what I edited was already modified in the new ImGui and then I also found out that the ImGui version we were relying on in DPG was really old.
We want to stick with this library but I honestly told them that I don't see the point of relying on an almost dead (okay, no, maintenance mode) library, so we decided to either upgrade it or drop it and replace it with something else.

I know that this is quite a huge change, and that several problems can arise but I also think that it can be definitely worth it. Also with this huge community all of these changes can be "easily" tested.

Changes

If you want to take a look to the original releases you can find them here for ImGui and here for ImPlot
You'll probably find better explanations (and images/videos) of what I'm gonna tell you below.
(Keep in mind that many other improvements have been made, but these are the most important ones in my opinion)

OpenGL

(This has been on of the hardest problem to solve actually)
Currently, for the build, DPG relies on gl3w in the examples folder of ImGui. This doesn't sound like a really stable thing and indeed a couple of releases later, that's been removed, so I added gl3w directly in the thirdparty folder.
Also ImGui decided to make some heavy changings in order to be more cross-platform. One of these, which is also my biggest concern about all of this work, is about the buttons of the keyboard. These are now less than before, and they have been all mapped to predefined values in ImGui. So if someone used them heavily now they could have some problem.

Issues solved (afaik)

• Fix destroy_context doesn't clean up the window #2173, Window freezes when using stop_dearpygui #2013, Viewport stuck after closing when executing blocking instructions afterwards on MacOS #1593 (last 2 issues are closed but not solved, but they are all related)
• Fix add_text_point raises exceptions even when used with only required inputs #2108
• Fix dpg.set_axis_limits just lock the axis min limit and let the max limit be free (+vice versa) #2016

New Features

• For all the Drag* widgets there are new parameters delayed, no_cursor, no_fit, no_inputs
• Now there can be 3 X axes other than the currently supported 3 Y axes
• Plots: now you can decide a _mod button to enable certain actions only when that button is pressed (i.e. select, zoom, override, etc...) and you can also decide the zoom rate
• New StackToolWindow (there's still an empty window opening, but it'll be solved soon)
• Plots: many parameters have been added to each kind of plot
• Plots: new series like Infinite line series (just a wrapper for Vertical and Horizontal line series), Digital Series, Group Bar Series
• Plots: New way to query the plots (take a look at the demo)
• Plots: new functions set_axis_limits_constraints() and set_axis_zoom_constraints()
• Plots scale: there are currently four scales for the axes' plots, but more will be added and in the roadmap there's the goal to implement, like in ImGui, a custom callback scale.
• dpg.add_plot_tag(): now there can be tags attached to the axes
• New widget: SeparatorText()
• Several updates to tables (some of them still have to been implemented)
• Added function set_decimal_point()
• CTRL + Tab navigation across windows
• New params for InputText
• New params for dpg.add_window()
• New params to dpg.add_legend()
• More params for ColorMapScale
• Add recursive mode to filter_set (beta)
• New param "disabled" for dpg.add_group() (equivalent of new feature ImGui::BeginDisabled())

Breaking changes

• get_plot_query_area() -> get_plot_query_rects()
• no_mouse_pos -> no_mouse_text
• dpg.add_plot(): pan_button -> pan (and like this, other parameters of this widget)
• New mvStyleVar options
• In the Drag* widgets show_label param has been removed
• When adding new axes now you need to specify which ones (i.e. dpg.add_plot_axis(mvXAxis2))
• Now all the lines will automatically have AA (so the parameter anti-aliased has been dropped)
• Added several params to dpg.add_child_window()
• mvVLineSeries and mvHLineSeries -> mvInfLineSeries

Discontinued

• dpg.add_area_series() has been removed. Not a huge thing because of the shaded_series and the new shaded param in the add_line_series()

Changes expected in the future from ImGui

• Test engine
• Window shadows
• Shortcuts API

Major concerns

• As I said earlier, now there are less keyboard buttons than before, I think that they'll add more in the future, but I don't know how to solve this problem.
• In my workplace we have never used ImNodes so we haven't tested that a lot. There haven't been any huge updates during these years so there shouldn't be a lot (or any) problems. If someone's willing to help and take a look, they'd be welcome.

Notes

Everything was tested with Valgrind in order to be sure that there wasn't any kind of accidental memory leak (or at least not more than those who were already there).

Don't look at the README.md and TODO.md, it's nothing but development garbage, it'll be removed when merged. In case you'll read the TODO you'll see that there are still many (not too many) things to do. These are all new features and I'd say, almost all little ones, or difficult to implement, so they can actually be implemented later in the future.

My main goal here was to have a real feedback from the most expert users on this work.

I hope I was clear enough!
I'd like to improve this library because I think it's really great, so any kind of suggestion will be well accepted.

[v-ein]

Do we really need this new argument? An empty label (label="" or just omitting it) has the same effect (even handled by the same if within ImPlot), and it works for all other kinds of series, too. Is there anything special in custom_series that it needs a separate flag for this?

[v-ein]

Do we need an explicit argument for this?

The old version controls legend visibility by showing or hiding the plot_legend widget; if there's no add_plot_legend, the legend is hidden by default - which looks reasonable. The new version also switches ImPlotFlags_NoLegend when plot_legend gets shown or hidden. I'd say config.show on plot_legend is sufficient, whereas having more than one way to control this ImPlotFlags_NoLegend flag is excessive and will eventually lead to inconsistency.

[v-ein]

I'd suggest that we use the standard enabled keyword instead of adding disabled - this way it will be more consistent with the rest of the API.

Also, regarding the help text for disabled: mvThemeCol_TextDisabled is not used by many widgets in the disabled state; they use mvThemeCol_Text instead. Some even use mvThemeCol_TextDisabled for other stuff, like input_text renders the hint with that color. Add to this that in DPG, "enabled" and "disabled" styles are configured separately via theme components with different enabled_state.

I'd suggest that we leave only mvStyleVar_DisabledAlpha in this comment - and maybe explain that it is applied to all colors within the disabled group.

[v-ein]

delete_rect - a number of concerns with this argument:

• ❗❗ delete_rect is a rather confusing name. Like, I don't know what to expect when I see dpg.plot(delete_rect=False).

• Thinking about the purpose/use case for this flag: right now, if we specify delete_rect=False, this means the user will not be able to delete query rects, but will still be able to create them. Suppose a plot has a query rect from the very start, and the user by mistake creates one more query rect. If deletion is disabled, how would the user cancel the mistaken action?
  
  My feeling is that to properly customize this behavior, the option should disable both "add" and "delete" actions. That is, either the user can freely add and delete rectangles, or they can't do either of those (but will still be able to drag the rectangles).

  • Thinking further: if the user can add/delete rects, it might be useful to restrict the number of rects both from above and from below.
    
    In the end, it looks like a better way to do it might be to mimic the node editor and how it adds links via a separate callback. Like, instead of adding a rect to an internal list, we just call the user's callback and then the application can add a drag_rect on its own.
    
    The downside is that it would be totally incompatible with the old query feature. We can probably support both?

• With the default button assignment (SelectCancel is the left button), the double-click behavior of delete_rect conflicts with double-clicks on DragRect: you can double-click the edge to make it snap to the view boundary. With delete_rect=True, it looks really weird: the rect first expands and then disappears. Also, that edge expansion is a nice feature that we probably don't want to lose.

• Double-click deletes the last rect no matter what item was clicked, whereas intuitively the user would expect that they double-click a rect and exactly that rect gets deleted.

In the end, this argument, as well as the entire query/drag_rect change, needs more work to make it more consistent.

[v-ein]

query_button, query_mod, query_toggle_mod:
It took me a great deal of research to understand what happened to the query feature. Here are a couple of observations.

First of all, the old way to draw query rects was way more flexible in terms of specifying the button and the modifiers. The updated version suggests a hardcoded "click one mouse button while holding the other" method, which, being itself an interesting gesture, is not very intuitive. Moreover, for some users it might even be difficult to perform, and might cause the mouse to move slightly and affect the end result.

I do not see an easy way to preserve query_button and query_mod, so we'll have to say good-bye to these two. I wonder though whether we should add them back to the parser - with the DEPRECATED_REMOVE_KEYWORD_ARG flag. This way the existing scripts won't crash but will instead spit warnings, and might be able to work normally except for plot queries.

For query_toggle_mod, I think we can retain that. I'm going to add a patch that implements it. Unfortunately it uses kind of a hack on ImPlot to prevent it from messing up the query rect; however, the current solution with SelectCancel click is actually a hack as well - it only works because ImPlot processes SelectCancel on mouse up, while mvPlot does that on mouse down (i.e. earlier). I'm not blaming anyone (especially since that SelectCancel code came from ImPlot demo), just trying to say that we can easily exchange one hack to another.

Another problem is that all the standard modifiers (Ctrl, Alt, and Shift) are already used by default in ImPlot, with Ctrl now assigned to OverrideMod. I honestly think that ImPlot could do a better job on handling OverrideMod, e.g. check it on mouse down, so that the same modifier could be pressed during selection to change the semantics. In my patch, I'll be hacking OverrideMod on the fly to achieve that.

In future, I hope ImPlot provides a better API to handle user input for queries. I've opened an issue in ImPlot repo to address that, see epezent/implot#566.

[v-ein]

Here's a branch (in my fork of DPG) that implements query_toggle_mod on the updated ImPlot.
https://github.com/v-ein/DearPyGui-fixes/compare/porting...v-ein:DearPyGui-fixes:feature/query-toggle-mod?expand=1

You can probably cherry-pick commits or at least copy-paste the changes.

[v-ein]

no_child: Let's keep an entry with DEPRECATED_REMOVE_KEYWORD_ARG in the parser.

[v-ein]

A note on deprecated keywords:

I've suggested DEPRECATED_REMOVE_KEYWORD_ARG in a couple of places. For all these cases, we need to document:

• why the keyword is being removed;
• what should the app developer do to migrate to the new version (e.g. use a keyword in a different object, or simply remove the argument, or something else).

Not sure where to keep this doc, maybe just a comment in this PR? In the end, it should go to the release notes.

[v-ein]

A couple of words on declaring arguments deprecated. Right now, DPG has two options to deprecate an argument:

• DEPRECATED_RENAME_KEYWORD_ARG will issue a warning and pass the argument to C++ with a new name.
• DEPRECATED_REMOVE_KEYWORD_ARG will issue a warning and won't pass the argument at all.

I'd suggest that we add one more option, where a warning will be issued but otherwise the argument will be passed into the C++ part the way it did before, with its original name. This can be used for arguments that we want to retain for backward compatibility (maybe delete later), but also want to point the user to a newer, better way to do the same thing. We can name it simply DEPRECATED_KEYWORD_ARG. It will also need an extra message coded in argument definition, so that we can say something like "use argument X instead".

[v-ein]

item_count and group_count: We already have the total length of the values list, and can therefore deduce one of these arguments from another. I suggest to replace them with a single value specifying the number of bars in a group, and name it group_size (see also comments on the current group_size argument below). Then then number of items can be calculated from the list size.

What's more, I think we need to add more safety around list sizes. Right now, if labels is shorter than the number of groups, DPG will segfault. Since both values and labels are positional args (i.e. both will for sure be passed in), we can check that their sizes are correct (e.g. values is a multiple of group_size, and labels is no longer than len(values)/group_size), and raise an exception if they aren't.

[SamuMazzi]

Where do you suggest to put these checks? I was thinking in the draw function: maybe it's not the ideal place to put them, but otherwise I should put them in set_positional_configuration, set_configuration and fill_configuration_dict and I should replicate code and I don't love it tbh.

[v-ein]

The drawback of checking them in draw_group_bar_series is that we can't raise an exception there. I'd prefer to signal the error right away. This means we need to check these sizes in set_positional_configuration and set_configuration, but not in fill_configuration_dict. To prevent code duplication, let's add a helper function like ValidateBarGroupConfig(mvGroupBarSeriesConfig& outConfig) and call it from both set_positional_configuration and set_configuration. You can even declare it static so that it's not visible outside of mvPlotting.cpp.

To raise an exception, use mvThrowPythonError. If I remember correctly, there's no need to return anything to Python in this case. Don't look at how DPG does it - in many cases throughout DPG code, it returns None after mvThrowPythonError - this is actually wrong and it crashes in debug mode (documented in #2097).

[SamuMazzi]

Okay, I was implementing this and then I recognized that if I use the function as you told me (with mvGroupBarSeriesConfig& outConfig as parameter) I can check the state only when it's already been set, then if there's some error I should restore back to how it was.
Or mvThrowPythonError takes care of it by itself? (But I don't think so)

[v-ein]

mvThrowPythonError will just store exception info inside of Python so that when it gets back from C++, it raises the exception.

Yes, looks like we need to either back up the old state, or to have a function like SetBarGroupData(mvGroupBarSeriesConfig& outConfig, some-other-parms) that would store values, labels, and group_size in the config in a single atomic operation (first checking that new values are valid). "Some-other-parms" here could be three PyObject* obtained from keyword args or from positional args, and SetBarGroupData would parse them into local variables, replacing NULLs with values from the config. It can then validate the values and store in the config.

[v-ein]

no_undo_redo help text - there's no such thing as ClearActiveID in DPG.

Moreover, I'm not sure that a custom undo/redo stack can be implemented on DPG at all. My attempts at custom filtering (like decimal/hexadecimal but with another set of characters) ultimately failed. Custom undo/redo is a pretty close thing and will suffer from similar issues. I'll be happy to hear that it is doable (seriously, I have a use case for it). Otherwise maybe remove the entire sentence about ownership and undo/redo stack.

[v-ein]

x, y: Changing these to list-type and at the same time asking the user to only pass one element (rather than enforcing it via type system) looks really weird. What's the reason behind this?

[SamuMazzi]

I think it was because of consistency with all the other series... I'm pretty sure I tried to accept single elements but I couldn't. Anyway I'll try again and I'll tell you if and where I get stuck

[v-ein]

While this condition might makes tooltip rendering more efficient in some cases (not in the current version of ImGui though 😁), it will also lead to item state "getting stuck" like it was on menu widgets in #2245. We need to update the state even when BeginTooltip returns false. Obviously state.visible will be false in this case; rectSize should probably be (0, 0); not sure about content region - probably (0, 0) too.

[v-ein]

It's coming! The Holy War! Tabs vs. spaces!

We'd be better off having 4 spaces in this line, like in the rest of this function.

[SamuMazzi]

I should have fixed it.. pls check it in the next commit

[v-ein]

Mx: Changing Mx to Style.LineWeight is a breaking change that effectively cancels the feature request #1094. The guy who asked for it will definitely be upset.

I see that Mx is no longer there in the ImPlot context. We can still simulate it this way:

auto Mx = ImPlot::GetPlotSize().x / ImPlot::GetPlotLimits().Size().x;

This gives exactly the same scaling factor as ImPlot::PlotToPixels uses; I've tested it, and the drawings look identical to what they were in the old version.

Regarding Style.LineWeight, I don't think that adding it in the drawings' outline is a good idea. The old version didn't do that, so if somebody customized mvPlotStyleVar_LineWeight on their plots, they will get unexpected results with the new version.

So my proposal is to compute Mx as shown above, and multiply _thickness by Mx only (not by LineWeight), as it was in the old version.

BTW, do you have a use for non-scaled line thickness (as in your updated version) in your own app? If so, we probably need to add it as an option; not sure where to put the corresponding flag. Adding it to each draw primitive (like dpg.draw_arrow(pixel_thickness=True)) seems to be the most straightforward and the least efficient way. But maybe you're not using the drawing primitives in plots, and we can simply drop the subject?..

[v-ein]

Adding this as a quickpath is fine, but the comment seems to be a bit misleading. I couldn't recreate a crash on UUID 0.

import dearpygui.dearpygui as dpg

dpg.create_context()
dpg.create_viewport(title="Test", width=700, height=750)

with dpg.window() as wnd:
    dpg.add_button(label="Click me", callback=lambda: dpg.configure_item(0, show=False))

# dpg.configure_item(0, show=False)

dpg.setup_dearpygui()
dpg.show_viewport()
dpg.start_dearpygui()
dpg.destroy_context()

Neither of the configure_item calls segfaults (or did you mean a Python exception and not a segfault?) - I tested both in old and in new versions, and they produce identical messages.

In fact, whenever the code finds a zero in cachedContainersID, the corresponding cachedContainersPTR contains null, keeping it from the initialization in mvItemRegistry constructor. This can be broken by calling CacheItem and passing it an item whose uuid field is zero - not sure whether such code path exists; I couldn't quickly find one. Looks like CacheItem is only called in places where the item already has a nonzero uuid.

That said, the comment probably needs correction.

Thinking more about it, how often does GetItem get called with UUID of zero? Won't that extra check just slow down most calls for no visible benefit? (sure, the slowdown will be negligible because GetItem is already slow as hell... still it looks to me like degrading 99.99% cases for the benefit of the remaining 0.01%).

[v-ein]

A note on key modifiers in the plot (this seems to be the only place where we can pass modifiers):

While it might look natural to pass dpg.mvKey_None to disable the corresponding modifier, mvKey_None doesn't actually work. ImPlot uses ImHasFlag to check if the modifier is pressed. The way ImHasFlag is implemented, ImHasFlag(keys, 0) will always return true, not false. To disable the modifier, one can pass -1 instead of mvKey_None, which is totally non-obvious. We probably need to document this somewhere.

It would be even better to modify the code so that mvKey_None leads to false in modifier tests, but that means making changes to ImPlot or even ImGui, which is a long way to go.

[v-ein]

mvCleanupViewport: Calling it here creates a race condition where onCloseCallback competes with the rest of destroy_context for DPG and ImGui objects. This is a known issue with destroy_context and I have a fix for it; however, right now let's not aggravate it by adding mvCleanupViewport in the mix.

It should be much safer to call it after GContext->future.get(). Let's just move the call to occur right before delete GContext->viewport (below), in the same if block.

[SamuMazzi]

Actually I put that line there because otherwise it used to throw an error. I tried again now with what you said and still it's giving me that error.
python: ./dearpygui/thirdparty/imgui/imgui.cpp:3739: void ImGui::Shutdown(): Assertion '(g.IO.BackendPlatformUserData == __null) && "Forgot to shutdown Platform backend?"' failed.

Notice that you need assertions enabled for that, but even if you disable them it gives me a segmentation fault.

[v-ein]

I see. mvCleanupViewport calls platform-specific cleanup in ImGui (which is not called at all in the old version), so it needs to go before ImGui::DestroyContext(). Let's leave it where it is in your version then. I'm going to fix that in future. Unfortunately the fix is not trivial and therefore I'd prefer not to include it directly into this update; I'll open a separate PR later.