Update ImGui and use event-based io

[Rob2309]

This PR updates ImGui to v1.89.1 and exposes the new event-based io functions.

Possibly breaking changes:

• ImageButton now takes an explicit ID, ImageButton::new() changed, added ui.image_button() and ui.image_button_config()
• some variables in ImGuiIO were removed

[Rob2309]

Oops I just saw the other PR implementing this (#685). Leaving this one open in case it is more complete.

[dbr]

This is great, thanks! I didn't get to the event-based API changes in the other PR, so this one is much further along

There's a couple of tweaks to merge this, mainly just running rustfmt and the commented out line. Updating the remaining platform-support code to use the event based API would be nice but feel free to leave it for now, and can be done after the PR is merged

[dbr]

Hmm the fatal runtime error: stack overflow error with the memory are somewhat mysterious to me - I noticed those while working on the other PR, it seems to happen when there are more than a certain number of uses of the assert_field_offset!

[dbr]

Ahh I think this is Gilnaa/memoffset#49 - the issue is the Io struct is now quite large because of the keymap etc

Edit: As a workaround, it seems just splitting the test_io_memory_layout function into two separate functions avoids this for now

[Rob2309]

Ahh I think this is Gilnaa/memoffset#49 - the issue is the Io struct is now quite large because of the keymap etc

Edit: As a workaround, it seems just splitting the test_io_memory_layout function into two separate functions avoids this for now

Yes it seems like offset_of! allocates a new Io struct for every call, which obviously takes a lot of memory in debug mode. The tests work fine in release mode.

[thomcc]

That should be solved by rust-lang/rfcs#3308, and was some part of the motivation for it, for what it's worth (not imgui::Io specifically).

[Rob2309]

What should be done about the changes in ImageButton though? I think it is backwards incompatible as of now, would this be a problem? A workaround could be to just use PushID and always create the ImageButton with an empty string...

[thomcc]

In the mean time you should just run that test on a thread with a lot of stack, perhaps.

[Rob2309]

That should be solved by rust-lang/rfcs#3308, and was some part of the motivation for it, for what it's worth (not imgui::Io specifically).

I see, until then, splitting the checks is probably fine

[Rob2309]

In the mean time you should just run that test on a thread with a lot of stack, perhaps.

that would work too

[Rob2309]

I think I fixed everything now. winit-support now uses the full event system, the tests work by using a thread with a 4MB stack and I ran cargo fmt...

[dbr]

somewhat mysterious

Oh, not mysterious, I just forgot how stack variables work,

Well, when a function gets called, some memory gets allocated for all of its local variables and some other information. This is called a ‘stack frame’, and for the purpose of this tutorial, we’re going to ignore the extra information and only consider the local variables we’re allocating. So in this case, when main() is run, we’ll allocate a single 32-bit integer for our stack frame. This is automatically handled for you, as you can see; we didn’t have to write any special Rust code or anything.

When the function exits, its stack frame gets deallocated. This happens automatically as well.

So makes perfect sense the test function tries to allocate enough space for every offset_of! call, and adding too many in one function eventually falls over the stack size limit

That should be solved by rust-lang/rfcs#3308

Ah excellent, good to know!

Moving the test to a thread is a good idea, bit neater than splitting it over multiple functions

What should be done about the changes in ImageButton though? I think it is backwards incompatible as of now, would this be a problem? A workaround could be to just use PushID and always create the ImageButton with an empty string...

I think there is two options (well, three if we count "just break the API" 😛 )

1. Keep ImageButton::new as is, and mirror the upstream backwards compatible behaviour manually, which is here (which is basically just to push the texture ID as an ID, and empty string for the widget
2. Remove IMGUI_DISABLE_OBSOLETE_FUNCTIONS from cimgui runs, and use the obsolete functions present in bindings until we remove the deprecated method

For this case 1. seems like it should be pretty straight forward

However for future I'm increasingly thinking keeping the deprecated function bindings around is a good idea - as it would make it easier to avoid breaking changes, avoiding redundant reimplementation. That said:

1. It might have some non-obvious downside. Possibly exposing the deprecated methods to people in imgui-sys isn't great, as they would have no easy way to tell if it's deprecated or not.
2. May be worth deferring this to if/when we switch to using the "Dear Bindings" C API generator (not sure how it handles deprecated functions)

[dbr]

Hmm the freetype failure seems to be caused by a change in cimgui,
cimgui/cimgui@d359d90
Don't quite understand the reason behind this, but adding

build.define("CIMGUI_FREETYPE", None);

to imgui-sys/build.rs alongside IMGUI_ENABLE_FREETYPE seems to work fine

[Rob2309]

What should be done about the changes in ImageButton though? I think it is backwards incompatible as of now, would this be a problem? A workaround could be to just use PushID and always create the ImageButton with an empty string...

I think there is two options (well, three if we count "just break the API" 😛 )

1. Keep ImageButton::new as is, and mirror the upstream backwards compatible behaviour manually, which is here (which is basically just to push the texture ID as an ID, and empty string for the widget
2. Remove IMGUI_DISABLE_OBSOLETE_FUNCTIONS from cimgui runs, and use the obsolete functions present in bindings until we remove the deprecated method

For this case 1. seems like it should be pretty straight forward

When we stick to 1, ImageButton cannot take Ui as an argument, meaning it can't be stored in ImageButton. For the newly added ui.image_button(_config) functions, we would need a different builder that stores the Ui. If that is ok I can implement this.

[Gilnaa]

If/when you're willing to bump the MSRV to 1.65 and memoffset to v0.8, you can use offset_of! in a const expression to avoid this issue.

EDIT: ofc it's better to use core::offset_of! when it's available; but I assume that it'll take some time to stabilize

[dbr]

When we stick to 1, ImageButton cannot take Ui as an argument, meaning it can't be stored in ImageButton

Ohhh, I see - yeh the builder backwards-compat with the new() -> build(ui: ...)tonew(ui: ...) -> build()` thing is tricky

With a bulk of these changes in #537 I just resigned to breaking things, as it would involve duplicating a large portion of the code

However with this and the few builders ::new(...) methods still to update, I wonder if we can do better (probably by just duplicating the builder as you say). Thinking out loud:

I initially though we keep ImageButton as-is, and add a new struct for the "build from UI" methods - but naming the new struct is hard (since we couldn't call the new one ImageButton)

Instead I realized we can probably just:

1. Have the now-deprecated ImageButton::new method return a new ImageButtonDeprecated struct which has the old methods like fn build(self, ui: &'ui Ui) -> bool etc
2. Create a new, private constructor for use by ui.image_button etc (which returns ImageButton)
3. Then we can change the build method to ImageButton::build(self) -> bool (as done already in this PR)

That way we:

• later we can cleanly just delete ImageButtonDeprecated along with ImageButton::new
• keep using the "correctly" named ImageButton API for the ui.image_button_config(...).build() usage
• Existing calls to ImageButton::new would (in most cases) keep working without source-code change, even if it returns a different named struct
• I think it is technically still a breaking change - as someone might have a method like fn example(x: ImageButton), and doing example(ImageButton::new(...)) would now break - but it will have a fairly clear error message, and the more common ImageButton::new(...).build(ui) will keep working - so I think this is probably a good compromise

So basically the change would not be "semver compatible", but "probably source compatible" (i.e will work in common cases without library user changing code; and where code changes are needed the old code will error clearly importantly never keep working and cause unexpected runtime behaviour) - which for a GUI library (large API surface) binding to a third-party library (some changes outside our control) is possibly a good target to aim for

Sorry for the long reply! A bit excessive answer for this particular case, mostly thinking of how best to handle these changes in general

[Rob2309]

That would probably work quite well in every "normal" API usage case 👍. I had the idea of making a similar thing work with generics. However, I will probably not have time for this until after christmas...

[Snowiiii]

:c waiting

[Rob2309]

The changes to ImageButton should now be backwards compatible, as long as the type ImageButton is not named directly in user code, which should not be the case most of the time. I went with the solution of new() returning a ImageButtonDeprecated instead of my generics solution as it seemed simpler.

[dbr]

Hope you had a good Christmas! The backwards-compat changes look good, thanks.

I pushed a few minor changes to fix the obvious CI failures, the remaining ones need a bit more looking into 🤔

• The macos builds are failing because they don't know constexpr - possibly just need to enable -std=c++11 (edit: fixed, macos and ubuntu checks now failing consistently)
• The ubuntu builds are failing because ImFontAtlasGetBuilderForStbTruetype cannot be found when both docking+freetype features are enabled

[Rob2309]

The ubuntu build failure seems to be caused by a missing flag to generator.lua. When changing update-cimgui-output.sh to include "internal,freetype" as flags instead of "internal" and regenerating bindings, everything seems to work.
However, since freetype is feature based, we would need a way to regenerate bindings on feature changes.

[dbr]

The ubuntu build failure seems to be caused by a missing flag to generator.lua

Ahh quite right - short of moving the cimgui generator to build-time (which increases the build requirements for imgui-rs to include luajit etc, which I think is unreasonable), I think the solution is just to add a freetype variant to the various generated bindings.rs

Pushed commit with this, and it adds a bit of clutter but nothing too drastic.

[dbr]

This all looks good to me - @Rob2309 are you aware of any outstanding issues, or is it good to merge?

[Rob2309]

I see no major problems.

The only thing we might want to revert is the name change to Io::config_viewports_notask_bar_icon. I changed it to config_viewports_no_task_bar_icon since it seemed like a type, but it breaks compatibility with code that uses the old name with typo. If that is ok I think we could merge.

It might be good to add a test that uses all public variables of the major imgui structs, so that breaking changes would be caught easily.

[dbr]

The only thing we might want to revert is the name change to Io::config_viewports_notask_bar_icon. I changed it to config_viewports_no_task_bar_icon since it seemed like a type, but it breaks compatibility with code that uses the old name with typo. If that is ok I think we could merge.

I think this is okay - it was a typo (in imgui-rs), and I think it's worth changing given:

1. Already some unavoidable breaking changes
2. It's behind the docking feature
3. It's a slightly-obscure config flag relating to the multi-viewport support (one of the more in-progress parts of upstream)
4. If old variant is used, should result a nice clear "did you mean: ..." error

It might be good to add a test that uses all public variables of the major imgui structs, so that breaking changes would be caught easily.

This is a very good point, it's pretty hard to keep track of exactly what is breaking

I suspect a bit of code using everything would be hard to maintain - something like one of the semver-checking tools might be more practical:

• semverver (which doesn't seem to be actively developed and falls over on some of the code in imgui)
• cargo-semver-checks which seems to work quite nicely

The latter outputs the following for the current state of this PR:

Output

   Completed [   8.626s] 33 checks; 28 passed, 5 failed, 0 unnecessary
--- failure auto_trait_impl_removed: auto trait no longer implemented ---
Description:

A public type has stopped implementing one or more auto traits. This can break downstream code that depends on the traits being implemented.

ref: https://doc.rust-lang.org/reference/special-types-and-traits.html#auto-traits

impl: https://github.com/obi1kenobi/cargo-semver-check/tree/v0.15.2/src/queries/auto_trait_impl_removed.ron
Failed in:

type ImageButton is no longer UnwindSafe, in imgui/src/widget/image.rs:78

type ImageButton is no longer Send, in imgui/src/widget/image.rs:78

type ImageButton is no longer Sync, in imgui/src/widget/image.rs:78

type ImageButton is no longer RefUnwindSafe, in imgui/src/widget/image.rs:78
--- failure enum_variant_added: enum variant added on exhaustive enum ---
Description:

A publicly-visible enum without #[non_exhaustive] has a new variant.

ref: https://doc.rust-lang.org/cargo/reference/semver.html#enum-variant-new

impl: https://github.com/obi1kenobi/cargo-semver-check/tree/v0.15.2/src/queries/enum_variant_added.ron
Failed in:

variant Key:LeftCtrl in imgui/src/input/keyboard.rs:24

variant Key:LeftShift in imgui/src/input/keyboard.rs:25

variant Key:LeftAlt in imgui/src/input/keyboard.rs:26

variant Key:LeftSuper in imgui/src/input/keyboard.rs:27

variant Key:RightCtrl in imgui/src/input/keyboard.rs:28

variant Key:RightShift in imgui/src/input/keyboard.rs:29

variant Key:RightAlt in imgui/src/input/keyboard.rs:30

variant Key:RightSuper in imgui/src/input/keyboard.rs:31

variant Key:Menu in imgui/src/input/keyboard.rs:32

variant Key:Alpha0 in imgui/src/input/keyboard.rs:33

variant Key:Alpha1 in imgui/src/input/keyboard.rs:34

variant Key:Alpha2 in imgui/src/input/keyboard.rs:35

variant Key:Alpha3 in imgui/src/input/keyboard.rs:36

variant Key:Alpha4 in imgui/src/input/keyboard.rs:37

variant Key:Alpha5 in imgui/src/input/keyboard.rs:38

variant Key:Alpha6 in imgui/src/input/keyboard.rs:39

variant Key:Alpha7 in imgui/src/input/keyboard.rs:40

variant Key:Alpha8 in imgui/src/input/keyboard.rs:41

variant Key:Alpha9 in imgui/src/input/keyboard.rs:42

variant Key:B in imgui/src/input/keyboard.rs:44

variant Key:D in imgui/src/input/keyboard.rs:46

variant Key:E in imgui/src/input/keyboard.rs:47

variant Key:F in imgui/src/input/keyboard.rs:48

variant Key:G in imgui/src/input/keyboard.rs:49

variant Key:H in imgui/src/input/keyboard.rs:50

variant Key:I in imgui/src/input/keyboard.rs:51

variant Key:J in imgui/src/input/keyboard.rs:52

variant Key:K in imgui/src/input/keyboard.rs:53

variant Key:L in imgui/src/input/keyboard.rs:54

variant Key:M in imgui/src/input/keyboard.rs:55

variant Key:N in imgui/src/input/keyboard.rs:56

variant Key:O in imgui/src/input/keyboard.rs:57

variant Key:P in imgui/src/input/keyboard.rs:58

variant Key:Q in imgui/src/input/keyboard.rs:59

variant Key:R in imgui/src/input/keyboard.rs:60

variant Key:S in imgui/src/input/keyboard.rs:61

variant Key:T in imgui/src/input/keyboard.rs:62

variant Key:U in imgui/src/input/keyboard.rs:63

variant Key:W in imgui/src/input/keyboard.rs:65

variant Key:F1 in imgui/src/input/keyboard.rs:69

variant Key:F2 in imgui/src/input/keyboard.rs:70

variant Key:F3 in imgui/src/input/keyboard.rs:71

variant Key:F4 in imgui/src/input/keyboard.rs:72

variant Key:F5 in imgui/src/input/keyboard.rs:73

variant Key:F6 in imgui/src/input/keyboard.rs:74

variant Key:F7 in imgui/src/input/keyboard.rs:75

variant Key:F8 in imgui/src/input/keyboard.rs:76

variant Key:F9 in imgui/src/input/keyboard.rs:77

variant Key:F10 in imgui/src/input/keyboard.rs:78

variant Key:F11 in imgui/src/input/keyboard.rs:79

variant Key:F12 in imgui/src/input/keyboard.rs:80

variant Key:Apostrophe in imgui/src/input/keyboard.rs:81

variant Key:Comma in imgui/src/input/keyboard.rs:82

variant Key:Minus in imgui/src/input/keyboard.rs:83

variant Key:Period in imgui/src/input/keyboard.rs:84

variant Key:Slash in imgui/src/input/keyboard.rs:85

variant Key:Semicolon in imgui/src/input/keyboard.rs:86

variant Key:Equal in imgui/src/input/keyboard.rs:87

variant Key:LeftBracket in imgui/src/input/keyboard.rs:88

variant Key:Backslash in imgui/src/input/keyboard.rs:89

variant Key:RightBracket in imgui/src/input/keyboard.rs:90

variant Key:GraveAccent in imgui/src/input/keyboard.rs:91

variant Key:CapsLock in imgui/src/input/keyboard.rs:92

variant Key:ScrollLock in imgui/src/input/keyboard.rs:93

variant Key:NumLock in imgui/src/input/keyboard.rs:94

variant Key:PrintScreen in imgui/src/input/keyboard.rs:95

variant Key:Pause in imgui/src/input/keyboard.rs:96

variant Key:Keypad0 in imgui/src/input/keyboard.rs:97

variant Key:Keypad1 in imgui/src/input/keyboard.rs:98

variant Key:Keypad2 in imgui/src/input/keyboard.rs:99

variant Key:Keypad3 in imgui/src/input/keyboard.rs:100

variant Key:Keypad4 in imgui/src/input/keyboard.rs:101

variant Key:Keypad5 in imgui/src/input/keyboard.rs:102

variant Key:Keypad6 in imgui/src/input/keyboard.rs:103

variant Key:Keypad7 in imgui/src/input/keyboard.rs:104

variant Key:Keypad8 in imgui/src/input/keyboard.rs:105

variant Key:Keypad9 in imgui/src/input/keyboard.rs:106

variant Key:KeypadDecimal in imgui/src/input/keyboard.rs:107

variant Key:KeypadDivide in imgui/src/input/keyboard.rs:108

variant Key:KeypadMultiply in imgui/src/input/keyboard.rs:109

variant Key:KeypadSubtract in imgui/src/input/keyboard.rs:110

variant Key:KeypadAdd in imgui/src/input/keyboard.rs:111

variant Key:KeypadEnter in imgui/src/input/keyboard.rs:112

variant Key:KeypadEqual in imgui/src/input/keyboard.rs:113

variant Key:GamepadStart in imgui/src/input/keyboard.rs:114

variant Key:GamepadBack in imgui/src/input/keyboard.rs:115

variant Key:GamepadFaceLeft in imgui/src/input/keyboard.rs:116

variant Key:GamepadFaceRight in imgui/src/input/keyboard.rs:117

variant Key:GamepadFaceUp in imgui/src/input/keyboard.rs:118

variant Key:GamepadFaceDown in imgui/src/input/keyboard.rs:119

variant Key:GamepadDpadLeft in imgui/src/input/keyboard.rs:120

variant Key:GamepadDpadRight in imgui/src/input/keyboard.rs:121

variant Key:GamepadDpadUp in imgui/src/input/keyboard.rs:122

variant Key:GamepadDpadDown in imgui/src/input/keyboard.rs:123

variant Key:GamepadL1 in imgui/src/input/keyboard.rs:124

variant Key:GamepadR1 in imgui/src/input/keyboard.rs:125

variant Key:GamepadL2 in imgui/src/input/keyboard.rs:126

variant Key:GamepadR2 in imgui/src/input/keyboard.rs:127

variant Key:GamepadL3 in imgui/src/input/keyboard.rs:128

variant Key:GamepadR3 in imgui/src/input/keyboard.rs:129

variant Key:GamepadLStickLeft in imgui/src/input/keyboard.rs:130

variant Key:GamepadLStickRight in imgui/src/input/keyboard.rs:131

variant Key:GamepadLStickUp in imgui/src/input/keyboard.rs:132

variant Key:GamepadLStickDown in imgui/src/input/keyboard.rs:133

variant Key:GamepadRStickLeft in imgui/src/input/keyboard.rs:134

variant Key:GamepadRStickRight in imgui/src/input/keyboard.rs:135

variant Key:GamepadRStickUp in imgui/src/input/keyboard.rs:136

variant Key:GamepadRStickDown in imgui/src/input/keyboard.rs:137

variant Key:MouseLeft in imgui/src/input/keyboard.rs:138

variant Key:MouseRight in imgui/src/input/keyboard.rs:139

variant Key:MouseMiddle in imgui/src/input/keyboard.rs:140

variant Key:MouseX1 in imgui/src/input/keyboard.rs:141

variant Key:MouseX2 in imgui/src/input/keyboard.rs:142

variant Key:MouseWheelX in imgui/src/input/keyboard.rs:143

variant Key:MouseWheelY in imgui/src/input/keyboard.rs:144

variant Key:ReservedForModCtrl in imgui/src/input/keyboard.rs:145

variant Key:ReservedForModShift in imgui/src/input/keyboard.rs:146

variant Key:ReservedForModAlt in imgui/src/input/keyboard.rs:147

variant Key:ReservedForModSuper in imgui/src/input/keyboard.rs:148
--- failure enum_variant_missing: pub enum variant removed or renamed ---
Description:

A publicly-visible enum has at least one variant that is no longer available under its prior name. It may have been renamed or removed entirely.

ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove

impl: https://github.com/obi1kenobi/cargo-semver-check/tree/v0.15.2/src/queries/enum_variant_missing.ron
Failed in:

variant Key::KeyPadEnter, previously in file /home/dbr/.cargo/registry/src/github.com-1ecc6299db9ec823/imgui-0.9.0/src/input/keyboard.rs:24
--- failure inherent_method_missing: pub method removed or renamed ---
Description:

A publicly-visible method or associated fn is no longer available under its prior name. It may have been renamed or removed entirely.

ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove

impl: https://github.com/obi1kenobi/cargo-semver-check/tree/v0.15.2/src/queries/inherent_method_missing.ron
Failed in:

ImageButton::frame_padding, previously in file /home/dbr/.cargo/registry/src/github.com-1ecc6299db9ec823/imgui-0.9.0/src/widget/image.rs:124
--- failure method_parameter_count_changed: pub method parameter count changed ---
Description:

A publicly-visible method now takes a different number of parameters.

ref: https://doc.rust-lang.org/cargo/reference/semver.html#fn-change-arity

impl: https://github.com/obi1kenobi/cargo-semver-check/tree/v0.15.2/src/queries/method_parameter_count_changed.ron
Failed in:

imgui::image::ImageButton::build now takes 1 parameters instead of 2, in imgui/src/widget/image.rs:166

Final [  19.148s] semver requires new major version: 5 major and 0 minor checks failed

It raises a good point we should add #[non_exhaustive] to some enums where they might gain extra variants (e.g Keys although now fairly exhaustive I can imagine still gaining extra keys)

[Rob2309]

The changelog should probably be updated.

[Snowiiii]

finally its merged yay

[dbr]

Merged! Thank you for all the work @Rob2309

I've split #695 and #694 to keep track of the remaining points (and updated changelog in main branch)