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

*Update DearImGui lib to 1.88 WIP version #31

Merged
merged 4 commits into from
Jun 26, 2022

Conversation

lemantisee
Copy link
Contributor

Remove direct access to obsolete key arrays for version starts from 1.87
Replace ImGui opengl loader by glad

Signed-off-by: Kostiantyn kos.chernenok@gmail.com

Remove direct access to obsolete key arrays for version starts from 1.87
Replace ImGui opengl loader by glad

Signed-off-by: Kostiantyn <kos.chernenok@gmail.com>
@lemantisee
Copy link
Contributor Author

Purpose to update ImGui library to latest version 1.88 WIP.
Starting from 1.87 ImGui suggest don't use key arrays in ImGuiIO struct and marked it as obsolete. So I remade keybord processing with suggested methods in ImGuiIO struct. Add back compability for versions less than 1.87.
Also file imgui_impl_opengl3_loader.h now is internal file and ImGui lib suggests to use own opengl loader. I add glad for opengl 3.0 Core.

@sammyfreg
Copy link
Owner

Cool, I'll have a look this weekend. I had initially decided to wait a bit to integrate 1.88, to make sure the big key change in Dear ImGui didn't have any unforeseen issue.

@sammyfreg
Copy link
Owner

sammyfreg commented May 29, 2022

I am not done checking the PR, but its looking good, with a few changes needed.

  1. We only need Dear ImGui version check on code used by Client (Code\Client\*). Anything else is only using a predetermined version stored under ThirdParty, so can just replace the code without ifdef.
  2. The CmdPacket classes can be used by Client with any Dear ImGui Version and a different Server Version. We cannot used ifdef in the class member declaration, (could lead to a different class size between client / server). What needs to happen here is that the input command needs data that can be used to update client 1.87+ and 1.86-. The cmdinput cpp can have ifdef that knows how to take the common input and feed it properly to any client version.
  3. With the change to the CmdInput data, the Command version (CmdVersion::eVersion) will need to be incremented to prevent error between mismatching Client/Server data exchange.

@@ -112,6 +119,11 @@ struct alignas(8) CmdInput
bool mCompressionUse = false; // Server would like client to compress the communication data
bool mCompressionSkip = false; // Server forcing next client's frame data to be uncompressed
uint8_t PADDING[4] = {};

#if IMGUI_VERSION_NUM >= 18700
Copy link
Owner

Choose a reason for hiding this comment

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

Cannot have Command Packets data structure that vary depending on ImGui version. The server needs to support both Imgui clients that are pre 1.87 and post 1.87. It should export the information needed by both, then the client api code should consume the one it needs.

Copy link
Owner

Choose a reason for hiding this comment

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

This class is used by both Client and Server to communicate.
Its values are written by the Server application and read by the Client.

The Server is always compiled with the same Dear ImGui version (stored under ThirdParty/DearImgui) so you can rely on having a predictable code used too fill the data, that uses the new 1.87 (still need to use the version define since still client includes the same code, without compiling it).

The Client has its own Dear ImGui version that can be any version. So the command packet needs to be able to fill the Dear Imgui client inputs in any format that it can supports.

Copy link
Contributor Author

@lemantisee lemantisee Jun 8, 2022

Choose a reason for hiding this comment

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

Hmm. Do we need back compability if imgui introduces this breaking changes?) It's seems quite complicated to support versions before and after keyboard chages. I think it can be done in that way - we implement own enums for keys and mouse buttons and getter/setter for keys state which uses this enums. But it will be hardly to convert legacy imgui KeysDown array to our enums.

Copy link
Owner

Choose a reason for hiding this comment

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

I haven't looked in detail about the input changes Dear ImGui introduced, but there are a lot of people pre Dear ImGui 1.87. Game projects tend to minimize changes until the game is shipped, so plenty of people will still be relying on the older input system. On my side, I would like the NetImgui Server application to continue being able to communicate with multiple clients using different Dear ImGui versions simultaneously.

Copy link
Owner

Choose a reason for hiding this comment

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

As for how to support it properly, I guess we could copy the new ImGui input enum into NetImgui and only enable it when the version is 1.87, so the enum is always valid. Instead of using windows virtual keys override, we now can use this same enum instead. This is just a quick idea, without having research the problem yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Added copy of ImGuiKey_ enum to NetImgui_CmdPackets.inl. Hide implementation for getting and setting keyboard keys. Also moved setup of imgui key map to CmdPackets. With this changes we can increase encapsulation and hide all keyboard routine.

@@ -8,6 +8,7 @@
#if HAL_API_PLATFORM_GLFW_GL3

#ifdef _MSC_VER
#include <Windows.h>
Copy link
Owner

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@lemantisee lemantisee Jun 8, 2022

Choose a reason for hiding this comment

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

Can you explain a little bit more about the problem with Windows.h? As I understood from twitt including Windows.h increasing compile time. But we need Windows.h for GetCommandLineA() function. It's not trivial to include header processenv.h which contains definition of GetCommandLineA(). And Windows.h widely used in dx11 imgui backend.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, it is just the concern of additional compile time and everything else that windows.h pulls. Do you need the GetCommandlineA? main(...) already contains the calling parameters, no?

Copy link
Owner

Choose a reason for hiding this comment

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

Unless it's because one of the newly added backend file is trying to use that function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I can combine cmd args to string without windows functions. Maybe there is some reason to use GetCommandlineA? Because it's legacy code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -415,6 +415,7 @@ void Client::CaptureImguiInput()

if( ImGui::IsWindowFocused() )
{
#if IMGUI_VERSION_NUM < 18700
Copy link
Owner

Choose a reason for hiding this comment

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

Do not need the version check define, since this is server only code, using only the internally provided ImGui. Only code used by client needs to have version check (everything under Code\Client\. Can just remove the old code here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -69,6 +69,7 @@ void HAL_Shutdown()

}

#if IMGUI_VERSION_NUM < 18700
Copy link
Owner

Choose a reason for hiding this comment

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

Remove code entirelly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -48,8 +48,10 @@ namespace NetImguiServer { namespace App
bool HAL_Startup(const char* CmdLine);
// Prepare for shutdown of application, with platform specific code
void HAL_Shutdown();
#if IMGUI_VERSION_NUM < 18700
Copy link
Owner

Choose a reason for hiding this comment

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

Remove declaration entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@lemantisee
Copy link
Contributor Author

Thank you for review. I will try to fix it on this week.

@sammyfreg
Copy link
Owner

sammyfreg commented Jun 8, 2022 via email

… client

Signed-off-by: Kostiantyn <k.chernenok@frontpictures.com>
@sammyfreg
Copy link
Owner

sammyfreg commented Jun 10, 2022

Nice! Thank you for this PR. I will finish looking at it and testing it locally next weekend (in ~ 9 days).

@sammyfreg
Copy link
Owner

I just realized, I think your PR integrated the main branch of Dear ImGui. The branch I need submitted into the ThirdParty\DearImgui needs to be from the dock branch, since the NetImgui Server needs the docking.

Signed-off-by: Kostiantyn <kos.chernenok@gmail.com>
@lemantisee
Copy link
Contributor Author

I have checked ThirdParty\DearImgui. It's looks like docking branch. imgui.h has docking flags and other docking routine.
But to be sure on 100% I have updated lib to last version from docking branch.

@sammyfreg
Copy link
Owner

Thanks, I will try to have a look this Sunday. Since this is a significant change, I will pull the changelist and try it locally.

@sammyfreg
Copy link
Owner

I have pulled the PR locally, and can see that there are various compile issues happening.

  • The OpenGL server backend has been left active by default, should be put back to DX11 since that's the only one that compiles in both debug/release version (I didn't include some OpenGL debug library when first adding support for it)
  • Every project should be able compile using 'Build->Batch build->Select all and build'. Right now, various projects do not build.
  • I also have regression test with older version of Dear Imgui, call 'Build\GenerateCompatibilityTest.bat' once, before generating the solutions, to fetch them and have them included in the build
  • Some of the errors are warnings needing to be fixed. Since I have -Wall active

@lemantisee
Copy link
Contributor Author

lemantisee commented Jun 19, 2022

To fix compile errors in PR I trying to compile current dev branch to setup clang. And can't compile under VS 2022 + clang 13.01. There is a lot of errors in imgui.h such as '_CmdBuffer' is reserved because it starts with '_' followed by a capital letter. Clang build requires some additional setups?

@sammyfreg
Copy link
Owner

sammyfreg commented Jun 20, 2022 via email

- Fix CmdInput for compatibility tests
- Add linking for glfw debug lib
- Update glfw to version 3.3

Signed-off-by: Kostiantyn <k.chernenok@frontpictures.com>
@lemantisee
Copy link
Contributor Author

lemantisee commented Jun 20, 2022

  • Fix compatibility test
  • Fix glfw error linking for debug build
  • Fix some warnings

Since we cannot include imgui opengl backend source files anymore (or maybe I didn't find good solution) I added permanent linking of opengl and glfw lib (both for win11 and opengl version). Maybe it will better to split server to two targets?

@sammyfreg
Copy link
Owner

Ok, I was able to compile it locally, but it seems that the Clang build doesn't work (missing GLFW libs which are not available on Glfw.org). Thus, NetImguiServer will need to have it's conditional lib/cpp include restored. For now, could you move this PR to a new branch I created instead (Task_InputRefactor) so we can finally have it in. Afterward, I will be able to fix the Clang support, and modify my samples project to support multiple Dear ImGui version. This input change has the potential to break some things and I would like to isolate and test it further before moving it to the dev branch.

Thank you.

@sammyfreg sammyfreg changed the base branch from dev to Task_InputRefactor June 26, 2022 05:08
@sammyfreg
Copy link
Owner

Thank you, I have pulled this into a input branch and will look at fixing the opengl lib issue in clang and make sure there's not issues.

@sammyfreg sammyfreg merged commit 49b688e into sammyfreg:Task_InputRefactor Jun 26, 2022
sammyfreg added a commit that referenced this pull request Jul 18, 2022
* Fixed a few issues

-Prevent client crash when Dear Imgui has 0 draws
-Fixed 32bits Client rendering issues
-Added Dear ImGui 1.87 compatibility test

* Make sure new client connection RT is cleared

Prevents seeing previous client content until 1st DrawCommand is received

* Skip preamble when parsing command line host;port (#25)

* Bug fix for command line parsing including the executable location.

* Add some safety to command line parsing.

* Coverity warning fixes

Fixes some issues found by Coverity static analyser

* Fix to null check & change vertex UV range

* * Fix data race between reading and writing pending textures (#26) (#28)

Signed-off-by: Kostiantyn <kos.chernenok@gmail.com>

* VS2022 warning fixes

* Fix accept thread not getting properly signalled to stop on certain platforms (#33)

* Version update

* *Update DearImGui lib to 1.88 WIP version (#31)

* *Update DearImGui lib to 1.88 WIP version

Remove direct access to obsolete key arrays for version starts from 1.87
Replace ImGui opengl loader by glad

Signed-off-by: Kostiantyn <kos.chernenok@gmail.com>

* *Add back compatibility for keyboard keys transfer between server and client

Signed-off-by: Kostiantyn <k.chernenok@frontpictures.com>

* * Update DearImgui lib to 1879 version

Signed-off-by: Kostiantyn <kos.chernenok@gmail.com>

* * Add debug libs for glfw

- Fix CmdInput for compatibility tests
- Add linking for glfw debug lib
- Update glfw to version 3.3

Signed-off-by: Kostiantyn <k.chernenok@frontpictures.com>

* Some OpenGl build fixes

Also reimported NetImguiServer_App_win32dx11.cpp from latest dear imgui sample main.cpp, makign sure it fits the current backend

* Added NoBackend sample

New Sample demonstrating use of Dear Imgui without any Backend.
-Useful for hardward without display/input.
Refactored the Dear Imgui compatibility test to now be console application trying to connect to NetImguiServer (using NoBackend sample code)
-This allows making sure older Dear ImGui works with current NetImgui (instead of only testing that it compiles)

* Support of the new Dear ImGui in put api

Fixed compatibility with older Dear ImGui version

* Added Gamepad support

* Updated to Dear ImGui 1.88 (docking branch)

* Visual Studio 2022 LLVM build fix

Runtime library switched to DLL

* Center server popup

Center server popup on parent Window when they first appear
Bug fix that prevent server asset copy in post build step

* readme update

Co-authored-by: Jeff <96199164+rtjeffk@users.noreply.github.com>
Co-authored-by: Kostiantyn Chernenok <44094399+lemantisee@users.noreply.github.com>
Co-authored-by: Víctor Popovici <557049+TheAnswer@users.noreply.github.com>
sammyfreg added a commit that referenced this pull request Jul 18, 2022
* Fixed a few issues

-Prevent client crash when Dear Imgui has 0 draws
-Fixed 32bits Client rendering issues
-Added Dear ImGui 1.87 compatibility test

* Make sure new client connection RT is cleared

Prevents seeing previous client content until 1st DrawCommand is received

* Skip preamble when parsing command line host;port (#25)

* Bug fix for command line parsing including the executable location.

* Add some safety to command line parsing.

* Coverity warning fixes

Fixes some issues found by Coverity static analyser

* Fix to null check & change vertex UV range

* * Fix data race between reading and writing pending textures (#26) (#28)

Signed-off-by: Kostiantyn <kos.chernenok@gmail.com>

* VS2022 warning fixes

* Fix accept thread not getting properly signalled to stop on certain platforms (#33)

* Version update

* *Update DearImGui lib to 1.88 WIP version (#31)

* *Update DearImGui lib to 1.88 WIP version

Remove direct access to obsolete key arrays for version starts from 1.87
Replace ImGui opengl loader by glad

Signed-off-by: Kostiantyn <kos.chernenok@gmail.com>

* *Add back compatibility for keyboard keys transfer between server and client

Signed-off-by: Kostiantyn <k.chernenok@frontpictures.com>

* * Update DearImgui lib to 1879 version

Signed-off-by: Kostiantyn <kos.chernenok@gmail.com>

* * Add debug libs for glfw

- Fix CmdInput for compatibility tests
- Add linking for glfw debug lib
- Update glfw to version 3.3

Signed-off-by: Kostiantyn <k.chernenok@frontpictures.com>

* Some OpenGl build fixes

Also reimported NetImguiServer_App_win32dx11.cpp from latest dear imgui sample main.cpp, makign sure it fits the current backend

* Added NoBackend sample

New Sample demonstrating use of Dear Imgui without any Backend.
-Useful for hardward without display/input.
Refactored the Dear Imgui compatibility test to now be console application trying to connect to NetImguiServer (using NoBackend sample code)
-This allows making sure older Dear ImGui works with current NetImgui (instead of only testing that it compiles)

* Support of the new Dear ImGui in put api

Fixed compatibility with older Dear ImGui version

* Added Gamepad support

* Updated to Dear ImGui 1.88 (docking branch)

* Visual Studio 2022 LLVM build fix

Runtime library switched to DLL

* Center server popup

Center server popup on parent Window when they first appear
Bug fix that prevent server asset copy in post build step

* readme update

Co-authored-by: Jeff <96199164+rtjeffk@users.noreply.github.com>
Co-authored-by: Kostiantyn Chernenok <44094399+lemantisee@users.noreply.github.com>
Co-authored-by: Víctor Popovici <557049+TheAnswer@users.noreply.github.com>
sammyfreg added a commit that referenced this pull request Aug 7, 2022
* Fixed a few issues

-Prevent client crash when Dear Imgui has 0 draws
-Fixed 32bits Client rendering issues
-Added Dear ImGui 1.87 compatibility test

* Make sure new client connection RT is cleared

Prevents seeing previous client content until 1st DrawCommand is received

* Skip preamble when parsing command line host;port (#25)

* Bug fix for command line parsing including the executable location.

* Add some safety to command line parsing.

* Coverity warning fixes

Fixes some issues found by Coverity static analyser

* Fix to null check & change vertex UV range

* * Fix data race between reading and writing pending textures (#26) (#28)

Signed-off-by: Kostiantyn <kos.chernenok@gmail.com>

* VS2022 warning fixes

* Fix accept thread not getting properly signalled to stop on certain platforms (#33)

* Version update

* *Update DearImGui lib to 1.88 WIP version (#31)

* *Update DearImGui lib to 1.88 WIP version

Remove direct access to obsolete key arrays for version starts from 1.87
Replace ImGui opengl loader by glad

Signed-off-by: Kostiantyn <kos.chernenok@gmail.com>

* *Add back compatibility for keyboard keys transfer between server and client

Signed-off-by: Kostiantyn <k.chernenok@frontpictures.com>

* * Update DearImgui lib to 1879 version

Signed-off-by: Kostiantyn <kos.chernenok@gmail.com>

* * Add debug libs for glfw

- Fix CmdInput for compatibility tests
- Add linking for glfw debug lib
- Update glfw to version 3.3

Signed-off-by: Kostiantyn <k.chernenok@frontpictures.com>

* Some OpenGl build fixes

Also reimported NetImguiServer_App_win32dx11.cpp from latest dear imgui sample main.cpp, makign sure it fits the current backend

* Added NoBackend sample

New Sample demonstrating use of Dear Imgui without any Backend.
-Useful for hardward without display/input.
Refactored the Dear Imgui compatibility test to now be console application trying to connect to NetImguiServer (using NoBackend sample code)
-This allows making sure older Dear ImGui works with current NetImgui (instead of only testing that it compiles)

* Support of the new Dear ImGui in put api

Fixed compatibility with older Dear ImGui version

* Added Gamepad support

* Updated to Dear ImGui 1.88 (docking branch)

* Visual Studio 2022 LLVM build fix

Runtime library switched to DLL

* Center server popup

Center server popup on parent Window when they first appear
Bug fix that prevent server asset copy in post build step

* readme update

* NetImgui 1.8 version update

* Version update to 1.8.1 wip

* Added ASAN to debug build

* Generate Project changes for GitAction

Co-authored-by: Jeff <96199164+rtjeffk@users.noreply.github.com>
Co-authored-by: Kostiantyn Chernenok <44094399+lemantisee@users.noreply.github.com>
Co-authored-by: Víctor Popovici <557049+TheAnswer@users.noreply.github.com>
@lemantisee lemantisee deleted the keyboard_PR branch June 1, 2023 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants