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

Externals / VideoCommon: update imgui to 1.89.7 (and implot to 0.15) #12065

Merged
merged 2 commits into from Jul 28, 2023

Conversation

iwubcode
Copy link
Contributor

@iwubcode iwubcode commented Jul 23, 2023

This updates imgui to its latest release 1.89.7 and due to breaking changes updates implot to 0.15.

Updating broke our handoff of the IO from QT to ImGui. This review also fixes that and moves us to use AddKeyEvent which is the new recommended way of handling key events.

Finally, I included imgui_demo.cpp as it is pretty handy to be able to see all the demo controls working when doing development. It is left out of compilation with IMGUI_DISABLE_DEMO_WINDOWS but can be enabled if someone wants to reference it.

@iwubcode
Copy link
Contributor Author

@Pokechu22 - sorry for the ping but would you feel comfortable reviewing this? It looks like you've worked on imgui in the past.

@Pokechu22
Copy link
Contributor

This looks reasonable to me. It'd be nice if we could make imgui a submodule, but I'm not sure how practical it would be to do that with the custom config.

@shuffle2 requested that the demos not be added in #11454 due to compile time concerns, though.

@iwubcode iwubcode force-pushed the imgui_update branch 2 times, most recently from 038a562 to 63c7afb Compare July 24, 2023 03:34
@iwubcode
Copy link
Contributor Author

@Pokechu22 - looks like we can use the option IMGUI_DISABLE_DEMO_WINDOWS to disable the compile time of the code.

Comment:

// Message to the person tempted to delete this file when integrating Dear ImGui into their codebase:
// Think again! It is the most useful reference code that you and other coders will want to refer to and call.
// Have the ImGui::ShowDemoWindow() function wired in an always-available debug menu of your game/app!
// Also include Metrics! ItemPicker! DebugLog! and other debug features.
// Removing this file from your project is hindering access to documentation for everyone in your team,
// likely leading you to poorer usage of the library.
// Everything in this file will be stripped out by the linker if you don't call ImGui::ShowDemoWindow().
// If you want to link core Dear ImGui in your shipped builds but want a thorough guarantee that the demo will not be
// linked, you can setup your imconfig.h with #define IMGUI_DISABLE_DEMO_WINDOWS and those functions will be empty.
// In another situation, whenever you have Dear ImGui available you probably want this to be available for reference.
// Thank you,
// -Your beloved friend, imgui_demo.cpp (which you won't delete)

Going to put back in the compilation and use that flag.

…with upgrade; keep the demo code in case someone wants to reference it but don't compile it by enabling 'IMGUI_DISABLE_DEMO_WINDOWS' in config
@iwubcode
Copy link
Contributor Author

@Pokechu22 - Appreciate you looking this over. I added the C++ interface that imgui ships which supports InputText for std::string which I need in my work. Please have a look and sign off if you think it is ready to go.

@iwubcode iwubcode changed the title VideoCommon: update imgui to 1.89.7 (and implot to 0.15) Externals / VideoCommon: update imgui to 1.89.7 (and implot to 0.15) Jul 27, 2023
ImGuiKey_A, ImGuiKey_C, ImGuiKey_V, ImGuiKey_X,
ImGuiKey_Y, ImGuiKey_Z,
};
static_assert(dolphin_to_imgui_map.size() == ImGuiKey_COUNT); // Fail if ImGui adds keys
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this static assert no longer correct? DolphinKey is a Common::EnumMap which doesn't do bounds-checking (but it looks like the more relevant thing is that DolphinKey::Z is the last key in the map, and that's less likely to change).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's no longer correct. While the key still exists, the number of entries is much much larger (compare with original). There was a big change to 1.87 inputs in imgui from what I understand.

My assumption was that this would be ok because both key_map and dolphin_to_imgui_map are the same types and are both bound by DolphinKey::Z as you mention. If DolphinKeyMap changed both of these maps would need to change as well.

Copy link
Contributor

@Pokechu22 Pokechu22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't looked over the changes to imgui itself, but this looks reasonable to me.

@JMC47 JMC47 merged commit a2a6473 into dolphin-emu:master Jul 28, 2023
11 checks passed
@iwubcode iwubcode deleted the imgui_update branch July 28, 2023 21:22
@iwubcode
Copy link
Contributor Author

Looks like this impacted netplay. I will look into reproducing and fix it (from the error guessing it is an unlabeled element).

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