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

Replace CEGUI with ImGui #2374

Closed
wants to merge 43 commits into from
Closed

Replace CEGUI with ImGui #2374

wants to merge 43 commits into from

Conversation

Lpsd
Copy link
Member

@Lpsd Lpsd commented Sep 24, 2021

I'm making this draft PR to track and discuss the potential for switching CEGUI with ImGui.

All CEGUI related code has been removed, including vendor files, and replaced with ImGui.

The entire implementation has been stripped back, so CGUI_Impl is pretty empty right now - it just contains the necessary stuff to "bootstrap" ImGui and a basic example of how to draw to our DX9 device using it.

Everything builds and runs without issue (apart from the obvious fact that CEGUI is missing, along with all the menus).

Upon launching you'll see some basic ImGui examples on top of a (slightly broken) default GTA:SA menu.

Connecting to a server also works fine (tip: use mtasa:// protocol). All of the GUI-related Lua definitions have been disabled for now, so they'll just throw Lua warnings/errors.

Some of the code which makes use of our CGUI implementation (in CServerBrowser, CMainMenu, etc) is just commented out for now, rather than actually being removed - we'll refactor it later.

The plan is to completely rewrite / refactor the internal GUI implementation, it's old, messy and a pain to work with.... Not to mention places like CServerBrowser are directly trying to work with CEGUI structures and classes, which makes having a GUI implementation layer completely useless.

Regardless of whether the GUI library is changed, our GUI implementation and surrounding code needs renewing - it's tied in with CEGUI too closely, the implementation layer is not used effectively enough to decouple CEGUI from MTA code. I'd also like to think after rewriting this we'd be able change the GUI library again in 5 years, without much hassle (no one else should be subjected to the pain I've been in during this whole CEGUI process).

If somehow you missed it, here's the draft CEGUI 0.8.7 upgrade PR for reference -> #1459

Regarding the Lua definitions / API - we would aim to keep these exactly the same, so scripts wouldn't break and their functionality should be pretty much identical - aside from design / style differences, obviously. I can't think of anything which would stop us from replicating the current Lua API.

There's obviously many benefits, and drawbacks to changing our GUI library - I'm not going to list them all here - however, with my experience of GUI, I'm confident ImGui is a better choice for MTA. It's much less bloated, has a lot more "on-the-fly" customization due to its immediate mode interface - and even out of the box it seems way more modern, imo.

One thing I briefly mentioned in the development Discord would be the ability for scripters to define their own "widgets" (gui element types, essentially) that are reusable. This is just one thing that wouldn't really be viable with CEGUI -- example:

local sliderValue = 0
local color = tocolor(255, 255, 255)

local widgetDefinition = guiDefineWidget("widget_unique_name", function()
  ImGui.Text("This is some useful text.");
  ImGui.SameLine();
  ImGui.Checkbox("This is a checkbox");

  ImGui.SliderFloat("float", sliderValue, 0, 1); -- Edit 1 float using a slider from 0.0f to 1.0f
  ImGui.ColorEdit3("color", color); -- Edit 3 floats representing a color
end)

local widget = guiCreateWidget(widgetDefinition or "widget_unique_name", "Widget Title");

Anyway - what are your thoughts? Would you be open to replacing CEGUI with ImGui?

React below with 👍 or 👎

Also, I'd like to hear any feedback or concerns. Fire away!

@patrikjuvonen patrikjuvonen added enhancement New feature or request upstream Related to vendor library labels Sep 25, 2021
@ImSkully
Copy link

ImSkully commented Sep 25, 2021

This is definitely a step in the right direction to modernize the look and feel of MTA, nowadays it is common for developers to use custom DX libraries primarily for the extensive visual customization it provides over the native CEGUI. The addition of widgets would also be a great thing and promote code reusability, not to mention the significant amount of UI development time that could be saved with such a feature.

Would this also open the possibility for servers to create custom imGui themes?

@Lpsd
Copy link
Member Author

Lpsd commented Sep 25, 2021

Would this also open the possibility for servers to create custom imGui themes?

Yeah - they'll be able to change it via Lua functions and we'll also provide our own style scheme (so they can provide a simple .style file or whatever, with their style defs).

@Allerek
Copy link
Contributor

Allerek commented Sep 26, 2021

Its awesome idea, we're trying to update CEGUI, while we can just replace it with ImGUI and create compatibility layer for it, so scripts will work as usual, updating CEGUI is just waste of time in general.

@Dutchman101
Copy link
Member

If this is ever about to get implemented, we should have all of the concerns raised in dev discord, investigated.. see below discussion

https://imgur.com/a/wEeJmcd

@prnxdev
Copy link

prnxdev commented Nov 8, 2021

Dope! I didn't check out their documentation but is there easy way to style widgets (buttons, inputs etc.)? If yes then remove that CEGUI right now!

// EDIT

#2374 (comment) - ok, I have an answer but can you provide some documentation for styling? I can't find one

@prnxdev
Copy link

prnxdev commented Nov 8, 2021

Do you have some binary to test it out on server or should I build it on my own (honestly - i don't want to)?

@Lpsd
Copy link
Member Author

Lpsd commented Nov 8, 2021

ok, I have an answer but can you provide some documentation for styling? I can't find one

ImGui doesn't have "normal" documentation, mostly everything you need to know is part of an issue topic on their GitHub - however in this case you're better off searching for "imgui style" on Google. Here's a good source for some style configs using ImGui's built in style scheme: ocornut/imgui#707

Styling is also partly down to us, as we'll be extending classes via our interface, meaning we'll essentially have our own style scheme.

Do you have some binary to test it out on server or should I build it on my own (honestly - i don't want to)?

No, as this is a draft PR there are no builds generated automatically. It's also not ready to be used via Lua or in servers (although you can still load into a server via the mtasa:// protocol, there will be no GUI as all the Lua definitions are temporarily commented out).

Currently you can load MTA and view the main menu using ImGui (which looks exactly the same as in CEGUI). You can hover over menu items and they animate, as normal, however clicking them will either crash or have no effect - as I'm currently working on input stuff (which is a huge task, as native input support in ImGui isn't great).

ImGui MTA Menu

don't mind the font, I've not implemented font support yet :P

@prnxdev
Copy link

prnxdev commented Nov 8, 2021

If you need some tester - I'm ready :D

@github-actions github-actions bot added the stale Inactive for over 90 days, to be closed label Feb 7, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Feb 7, 2022

This draft pull request is stale because it has been open for at least 90 days with no activity. Please continue on your draft pull request or it will be closed in 30 days automatically.

@Pirulax
Copy link
Contributor

Pirulax commented Feb 13, 2022

I don't know how to feel about it.
It is definitely going to be slower compared to CEGUI, no matter how bloated the latter is - ImGUI needs a lot more calls from Lua to C than CEGUI, and that's where the overhead lies.
I agree ImGUI is really nice and easy to work with, but while in C++ it's overhead is pretty limited, in our case it will be huge.
Take for example ImGui::Text - Each time the text is updated you will need to copy it to Lua, as Lua doesn't accept string references, but instead makes hard copies and iterns them - It would just strain the GC and waste memory.

If that wasn't enough, if we want to provide a compatibility layer we'll end up with a mess, as there would be gui functions and ImGui stuff as well.

I feel like it may be a nice experiment, but I don't see it getting merged. The time spent on this should instead be spent on getting the new CEGUI version merged instead, as it has some quite nice features (including the per-element styling which is really like).

ImGUI would probably be just a tiny bit faster compared to a widget system written in dx on the Lua side, it's not worth the effort.

@github-actions github-actions bot removed the stale Inactive for over 90 days, to be closed label Feb 14, 2022
@github-actions
Copy link
Contributor

This draft pull request is stale because it has been open for at least 90 days with no activity. Please continue on your draft pull request or it will be closed in 30 days automatically.

@github-actions github-actions bot added the stale Inactive for over 90 days, to be closed label May 15, 2022
@github-actions github-actions bot closed this Jun 14, 2022
@github-actions
Copy link
Contributor

This draft pull request was closed because it has been marked stale for 30 days with no activity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request stale Inactive for over 90 days, to be closed upstream Related to vendor library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants