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

Upgrade CEGUI to 0.8.7 #1459

Closed
wants to merge 99 commits into from
Closed

Upgrade CEGUI to 0.8.7 #1459

wants to merge 99 commits into from

Conversation

Lpsd
Copy link
Member

@Lpsd Lpsd commented May 23, 2020

This draft PR is intended to gather feedback/input on the big task of upgrading CEGUI to 0.8.7

The current version of CEGUI is 0.4.0, released in 2005. Version 0.8.7 was released in 2016 - so as you can imagine, there's major changes to classes/methods (a lot of which have been refactored and don't even exist anymore).

At the time of writing this, the CEGUI vendor files (including dependencies) have been upgraded to version 0.8.7, and the CEGUI project can be built successfully.

The remaining task is to re(write) the actual CEGUI implementation in MTA itself, which I've already started with the constructors/destructors in CEGUI_Impl.

The reason I started this was due to a big change to the scheme manager in CEGUI 0.8.X, which allows you to set a skin for each individual GUI element (rather than only being able to have a single skin/scheme for the whole of CEGUI, without ugly workaorunds). This will allow servers to define their own CEGUI skin to make their server feel more unique, whilst also keeping the client's skin choice for the MTA menus / server browser etc.

The latest version of CEGUI has many more benefits, such as more optimized (refactored) code, lots more properties & more. I'm yet to look into all the changes since 0.4.0 (there's a lot!).

Please feel free to share your thoughts, especially if you have any ideas or concerns about upgrading.

Thanks!

@qaisjp
Copy link
Contributor

qaisjp commented May 23, 2020

Well done on getting the CEGUI project to compile!

You mentioned a compatibility layer for certain properties (before: "Carat", after: "Caret"). What are your thoughts and plans on that?

@Lpsd
Copy link
Member Author

Lpsd commented May 23, 2020

I had a think - our Lua API pretty much acts as that compatibility layer, any "breaking" changes can be rectified there (or in the GUI project) and the end-user won't know any different.

Regarding properties, if there's simply been a name change, we could just manually track these from 0.4.0 to 0.8.7 and have a "lookup table" for guiSet/GetProperty (so when you're setting/getting "CaratIndex", you're actually setting/getting "CaretIndex"). It may be worth displaying a warning to let them know about this though.

If the property now requires a different value, we may be able to do a conversion on the value. For example, if it used to require 0 or 1, and now requires true or false, we can handle that in the Lua function definition. Otherwise, there's not much we can do.

I'm hoping (not just limited to properties) that most of the changes in CEGUI from 0.4.0 to 0.8.7 had backwards compatibility in mind, so we can match everything exactly how it was before.

@qaisjp
Copy link
Contributor

qaisjp commented May 23, 2020

I'm thinking it would be nice to have an API version in meta.xml or something. So that resources without an API version can use that, and resources with an API version can just use the latest and most correct attributes.

This 'api version' thing (c/sh)ould be submitted as a separate issue

@StrixG StrixG added enhancement New feature or request upstream Related to vendor library labels May 23, 2020
@Pirulax
Copy link
Contributor

Pirulax commented Jun 11, 2020

The API version thing seems like a good idea, but implementing it would be a mess.

@Lpsd
Copy link
Member Author

Lpsd commented Jun 11, 2020

I agree, it would be a clusterfuck of overloads and wrapping code in if statements to check for CEGUI API version - can't think of a "clean" way of implementing it.

@qaisjp
Copy link
Contributor

qaisjp commented Jun 11, 2020

CEGUI API version

FWIW I was suggesting a version for MTA's Lua API overall, not CEGUI specific version

@darkdreamingdan
Copy link
Member

darkdreamingdan commented Oct 11, 2020

Modifications we made to CEGUI that I'm aware of:

  • Allow images to be placed in a gridlist and combobox elements (for the server browser locked icon, and for the search players and search servers)
  • Numerous speed and refresh optimizations, which probably are deprecated with a new CEGUI
  • Added Unicode support, which again should be deprecated
  • Unicode Glyph Caching (again should be deprecated in newer CEGUI)
  • Unicode Glyph substitution using unifont. Hopefully new CEGUI has some functionality for glyph substitution?

@AlexTMjugador
Copy link
Member

By the way, if this PR gets merged, I think this issue would be resolved too: #283

@StrixG StrixG linked an issue Oct 12, 2020 that may be closed by this pull request
@Lpsd
Copy link
Member Author

Lpsd commented Oct 15, 2020

Just as a note (after some discussion about how to approach this monstrosity of a PR) - this draft pull request is basically just my training ground now for figuring out every part of CEGUI and seeing if I can get a 1:1 match with the current version of CEGUI (looks and functionality wise).

Once that's done I'll be recreating pretty much all the code and submitting smaller pull requests (for example, a PR just for updating the binaries/CEGUI source), then a pull request containing commits re-factoring each type of CGUI*_Impl.

something along those lines, anyway, otherwise this is just gonna be a complete mess to review.

@Lpsd

This comment has been minimized.

@Lpsd Lpsd closed this Nov 17, 2020
@Lpsd
Copy link
Member Author

Lpsd commented Nov 17, 2020

Thanks to @qaisjp for letting me know how to "merge" the new branch with this one. Everything seems good.

Note: follow commits starting from 3dd292a (Add CEGUI-0.8.7 vendor) - everything before that is the "old" branch and isn't relevant now.

Copy link
Member

@darkdreamingdan darkdreamingdan left a comment

Choose a reason for hiding this comment

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

Can we just share datafiles instead of duplicating them between cegui versions?

@Lpsd Lpsd added this to In progress in Vendor upgrades via automation Mar 25, 2021
@Pirulax
Copy link
Contributor

Pirulax commented Apr 18, 2021

So basically, all we need now is a ton of unit tests?
Or should we just test it briefly (crashes, etc), and let it out?

@Lpsd
Copy link
Member Author

Lpsd commented Apr 20, 2021

There's still some things left to fix. Gridlists don't work fully, sliders are broken, there's minor issues with relative positioning and sizing (after testing a few default resources, and some from the community), and probably a lot more undiscovered.

Everyone seems happy to drop support for the current themes, as long as a suitable alternative is provided, which makes it much easier. I've decided to go with "GWEN" as a base, it needs some tweaking but it's already much better than some of the default themes shipped with CEGUI. (https://github.com/benelot/CEGUI-GWEN-Skin)

You can see a few examples of the skin by clicking the spoiler below:

Examples of GWEN skin

image
image
image

(don't worry, I was also working on a dark theme)

Once I'm happy that the last few remaining "big" issues are fixed, we should just test. Anyone can help out, just clone the branch, compile it and load all your GUI scripts. We should have a dedicated place to track issues for this.

We can also slowly start refactoring the classes as we go along fixing issues - I've tried to keep everything as close to the original code as possible, however there's a lot I couldn't avoid at least partly re-writing.

Honestly, the code is shit. It was shit then, it's still shit now, and whoever reviews this needs to accept that it's shit. The best we can do is slowly test and refactor as we go, and at some point once we're happy the main issues have been addressed we'll release it into nightly for wider scale testing. Rinse and repeat.

If you don't already know, we already have the option to choose the CEGUI version, with the use_new_cegui cvar (0 or 1). This requires a restart of the client once set, and switches out the GUI module - so as for stability in nightly we should be fine. We can continue to fix issues and refactor the code.

I have some time off this week, maybe I'll get back on it ;)

@Lpsd
Copy link
Member Author

Lpsd commented Aug 17, 2021

Note for vendor crash fix cegui/cegui@263e53d

@Pirulax
Copy link
Contributor

Pirulax commented Aug 20, 2021

What's left exactly?
Would be nice to finish this

@Lpsd
Copy link
Member Author

Lpsd commented Aug 20, 2021

I have some time off this week, maybe I'll get back on it ;)

never happened, so still pretty much the same as last comment

@Lpsd Lpsd mentioned this pull request Sep 24, 2021
@Allerek
Copy link
Contributor

Allerek commented Sep 28, 2021

#2136

This should be checked.

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

github-actions bot commented Jan 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.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 7, 2022

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

@github-actions github-actions bot closed this Feb 7, 2022
Vendor upgrades automation moved this from In progress to Done Feb 7, 2022
@patrikjuvonen patrikjuvonen removed this from the Spring Maintenance milestone Aug 24, 2022
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
Development

Successfully merging this pull request may close these issues.

setElementParent on GUI elements is slightly broken Update CEGUI Allow server to change CEGUI skin
9 participants