-
Notifications
You must be signed in to change notification settings - Fork 149
feat(gameclient): Introduce imgui framework #2127
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
base: main
Are you sure you want to change the base?
feat(gameclient): Introduce imgui framework #2127
Conversation
|
| Filename | Overview |
|---|---|
| Core/Libraries/Source/ImGui/dx8_backend/imgui_impl_dx8.cpp | Custom DX8 backend implementation with MIT license. Previous comments addressed, lock result checks added. |
| Core/Libraries/Source/ImGui/dx8_backend/imgui_impl_dx8.h | Header file for DX8 backend with MIT license, clean API declarations. |
| Core/Libraries/Source/ImGui/wrapper/ImGuiFrameManager.cpp | Simple frame management wrapper, properly gated with frame state tracking. |
| Generals/Code/GameEngine/Source/GameClient/GameClient.cpp | ImGui integration in game update loop with proper frame begin/end calls at appropriate points. |
| GeneralsMD/Code/GameEngine/Source/GameClient/GameClient.cpp | Zero Hour variant with identical ImGui integration pattern to Generals version. |
Sequence Diagram
sequenceDiagram
participant App as Game/WorldBuilder
participant GC as GameClient
participant FM as FrameManager
participant DX8 as DX8Wrapper
participant Backend as ImGui_DX8Backend
participant Device as D3DDevice8
Note over App,Device: Initialization Phase
App->>DX8: Init(HWND, Width, Height)
DX8->>Backend: ImGui_ImplWin32_Init(HWND)
DX8->>Backend: ImGui_ImplDX8_Init(Device)
Backend->>Device: AddRef()
Note over App,Device: Render Loop
GC->>FM: BeginFrame()
FM->>Backend: ImGui_ImplDX8_NewFrame()
FM->>Backend: ImGui_ImplWin32_NewFrame()
FM->>FM: ImGui::NewFrame()
GC->>GC: ImGui::ShowDemoWindow()
GC->>FM: EndFrame()
FM->>FM: ImGui::Render()
GC->>DX8: TheDisplay->DRAW()
DX8->>Device: BeginScene()
DX8->>Backend: ImGui_ImplDX8_RenderDrawData()
Backend->>Device: SetRenderState()
Backend->>Device: CreateStateBlock()
Backend->>Device: DrawIndexedPrimitive()
Backend->>Device: ApplyStateBlock()
DX8->>Device: EndScene()
Note over App,Device: Device Reset
DX8->>Backend: ImGui_ImplDX8_InvalidateDeviceObjects()
Backend->>Device: Release buffers
DX8->>Backend: ImGui_ImplDX8_CreateDeviceObjects()
Backend->>Device: CreateTexture/CreateDepthStencilSurface()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
14 files reviewed, 4 comments
Greptile found no issues!From now on, if a review finishes and we haven't found any issues, we will not post anything, but you can confirm that we reviewed your changes in the status check section. This feature can be toggled off in your Code Review Settings by deselecting "Create a status check for each PR". |
ab35d8a to
742f8be
Compare
|
relates to : #387 and #2084 (comment) |
de59853 to
d7e3765
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
14 files reviewed, 1 comment
d7e3765 to
4e08d7e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
14 files reviewed, 4 comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
14 files reviewed, 3 comments
Core/CMakeLists.txt
Outdated
| ) | ||
| # link imgui if needed | ||
| if (RTS_BUILD_OPTION_IMGUI) | ||
| target_link_libraries(corei_always INTERFACE lib_imgui) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think imgui is not suitable for _always lib. _always lib is meant to be included in all targets. But there are build targets that have no rendering and never will.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't that still mean that it is added to all targets through corei_always ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw that corei_always doesnot mean gi_always and zi_always these are lower down the chain ?
instead of having them in corei_always once. it is added twice once to gi_always and zi_always because multiple targets depend on it from both generals and zero hours, specifically guiedit (x2), worldbuilder (x2) and GameClient (x2), so it is only visible to those targets that link gi_always or zi_always i removed it from corei_always so not all targets can see it . GuiEdit for example can because it links against gi_always (or zi_always) . An alternative would be to go further down the chain and have imgui lib linked to the individual targets that need it but that means i would have to add it in 4*2 places depending on how granular we want to go.
THe problem is that imgui_lib is needed in multiple locations (WinMain, dx8wrapper, GameClient and wb3dclient) so the lower we go the more places we have to add it . Any suggestions other than zi_always and gi_always ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
target_link_libraries(corei_always INTERFACE
core_config
core_utility
corei_libraries_include
resources
)
i think only targets that need generals or zerohour codebase link against gi_always and zi_always but you are obviously more knowleadgeable about the codebase. So i could be wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you search for d3d8lib in cmake files then you find all targets that link Direct X. I suggest put imgui right there for all necessary targets. I think Game exe and World Builder exe are good for starters. W3DView and GUIEdit are not important to have it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i have moved it where it is needed at the deepest possible component. gameclient is part of the gameengine so that is where i hadded it. we are lucky also that the gameengine is used by worldbuilder so that's good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imgui is failing to render after this change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UPDATE: multiple regressions happened . ImGui failing to render after these compile changes. apparently many more targets need to link to libimgui to get it work, will investigate when i have more time. This is curently a major showstopper as imgui nolonger shows in Worldbuilder (both of them).
86dec82 to
d72b80f
Compare
This reverts commit d469040. Reason : Build failure with HRESULT IDirect3DDevice8::SetPixelShader(DWORD)': cannot convert argument 1 from 'nullptr' to 'DWORD
now linking against gi_always and zi_always to ensure rendering targets have access
d579452 to
21171d1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
21 files reviewed, 1 comment
Thanks. i did exactly that but given the large differences between both backends, some braces might slip the net while i am scanning. i hope i got them all now |
after moving link location multiple targets fail to render imgui. this has been fixed in ZH and generals but is yet to be fixed in worldbuilder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
25 files reviewed, 1 comment
Note : to test this please configure in Debug mode before building with
-A Win32 -DRTS_BUILD_OPTION_DEBUG=ON -DRTS_BUILD_OPTION_IMGUI=Onall the code is gated behind a RTS_IMGUI_ENABLED perprocessor define for debugging purposes
What it does : Integrates the Dear Imgui framework and loads a console and demo window overlay
in debug mode (generalsv, generalszh, WorldBuilderV and WorldBuilderZH)
What is missing
Some useful command or Ideas what it should look like: this PR will just focus on build and runtime integration via ImGui DemoCurrently it is permanently enabled when the game is builtwith the imgui option onGated behind RTS_BUILD_OPTION_IMGUICode cleanup and SuperHackers Comments etc...doneBackporting to Generalsdone