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

VGui: Rework #1752

Draft
wants to merge 58 commits into
base: master
Choose a base branch
from
Draft

VGui: Rework #1752

wants to merge 58 commits into from

Conversation

TimGoll
Copy link
Member

@TimGoll TimGoll commented Jan 30, 2025

This PR has many things yet to do, but I wanted to open a draft so it is open to discussion. There are a few things I want to tackle in this PR. It will probably be rather large, but without any meaningful new features. It is mostly renamed, reordered and cleaned up.

Reviewing this is probably hard, therefore the following list explains the changes. As you can see in the diff, it consists mostly of restructuring, deletions and added documentation. Nothing new was added, only some small improvements.

1. Broken Inheritance

The first issue I want to address in this PR is the broken vgui inheritance in TTT2. There are quite a few places in the codebase with code duplications and also many unnecessary vgui elements.

After some investigation I learned that both PANEL and LABEL are defined in the GMod codebase, while every other element with a D prefix is defined in lua. We could use those elements already existing, but there are so many modifications by now, that is is useful to stick to our copied and modied files.
However, in our code base TTT2:DLabel and TTT2:DPanel have many duplications because they both inherit from their respective engine defined base. In the engine LABEL inherits from PANEL. We can make TTT2:DPanel inherit from TTT2:DLabel`. This drastically cleans up our code and has only a single side effect: the two elements are mostly similar (with some changed default values).

2. Repetitive Code

The issue that triggered this rework is the following scenario. This is how it looks if you want to create a button:

local buttonReport = vgui.Create("DButtonTTT2", buttonArea)
buttonReport:SetText("search_call")
buttonReport:SetSize(self.sizes.widthButton, self.sizes.heightButton)
buttonReport:SetPos(0, self.sizes.padding + 1)
buttonReport:SetIcon(roles.DETECTIVE.iconMaterial, true, 16)
buttonReport.DoClick = function(btn)
    bodysearch.ClientReportsCorpse(data.rag)
end

When using method chaining this can be cleaned up a lot:

local buttonReport = vgui.Create("TTT2:DButton", buttonArea)
    :SetText("search_call")
    :SetSize(self.sizes.widthButton, self.sizes.heightButton)
    :SetPos(0, self.sizes.padding + 1)
    :SetIcon(roles.DETECTIVE.iconMaterial, true, 16)
    :On("Click", function(btn)
        bodysearch.ClientReportsCorpse(data.rag)
    end)

This makes it way more clean and less cluttered.

Note: I chose to do it this way because it adds new possibilities without breaking old code. The old UI code will work as before.

3. Events and Hook Overwrites

It is typical that our custom elements have something like this in their initialize function:

local oldClick = self.DoClick

self.DoClick = function(slf)
    sound.ConditionalPlay(soundClick, SOUND_TYPE_BUTTONS)

    oldClick(slf)
end

While this does work, it is rather tedious and prone to errors. This has to be done since there is no real inheritance in vgui elements that could use BaseClasses, at least not as far as I could tell. Additionally one might want to attach a function to an event after a element was already created. The developer here has to make sure that they don't overwrite an already existing hook.
The way the sound was added to buttons is a good example of the issue at hand.

The solution is to introduce a simple system, that mimics hooks. Elements can be registered with PANEL:On(name, function). That supports multiple events on a single event. So we can handle our button sounds internally in a clean way, while also exposing the event to the developer.

Similarly PANEL:After(delay, function) was introduced to provide a clean wrapper for timers.

Adding sound to a button would look like this:

local buttonReport = vgui.Create("TTT2:DButton", buttonArea)
    :On("Click", function(slf)
        sound.ConditionalPlay(soundClick, SOUND_TYPE_BUTTONS)
    end)

4. Attach

I also noticed that many of our custom elements are only copies of the panel with added setters and getters. We could probably delete quite a few of them, if we just reused existing ones. One solution I suggest is using PANEL:Attach(identifier, data). This is basically a fancy wrapper for PANEL.identifier = data to be able to attach any data while using method chaining.

5. PerformLayout and Caching

Currently, everything is done every frame. Translating, calculating sizes, changing colors. This is changed in the new basic panel by going back to GMods PANEL:PerformLayout system.

Internally, the base element now looks like this:

function PANEL:PerformLayout(w, h)
    self:TriggerOnWithBase("VSkinUpdate")
    self:TriggerOnWithBase("TranslationUpdate")
    self:TriggerOnWithBase("RebuildLayout", w, h)
end

This triggers three hooks on all inheriting panels. The first hook updates the cached colors, the second hook updates the cached translations, while the third rebuilds the layout. This also means that the panels will now invalidated on every vskin and language change.

This is done in three different hooks to keep the code tidy, while also making sure that subsequent overwrites do not break the core logic.

The paint hook was changed accordingly. Instead on relying on an individual draw function for every flavor of every element, mutual elements were selected:

function PANEL:Paint(w, h)
    if self:PaintBackground() then
        derma.SkinHook("Paint", "ColoredBoxTTT2", self, w, h)
    end

    if self:HasText() then
        derma.SkinHook("Paint", "LabelTTT2", self, w, h)
    end

    if self:HasIcon() then
        derma.SkinHook("Paint", "IconTTT2", self, w, h)
    end

    return true
end

This way many code duplications could be removed, while also vastly improving the rendering performance.

An element with many special features now looks like this:

local plyKarmaBox = vgui.Create("TTT2:DPanel", buttonArea)
    :SetSize(sizes.widthKarma, sizes.heightRow)
    :SetColor(colorTeamLight) -- sets the background color; if not set the vskin color is used
    :SetText(CLSCORE.eventsInfoKarma[ply.sid64] or 0)
    :SetFont("DermaTTT2CatHeader")
    :EnableCornerRadius(true) -- makes it a rounded box
    :EnableFlashColor(true) -- makes it pulsate to highlight this element
    :SetTooltipPanel(plyKarmaTooltipPanel)
    :SetTooltipFixedPosition(0, sizes.heightRow + 1)
    :SetTooltipFixedSize(widthKarmaTooltip, heightKarmaTooltip)

Note: This panel now also supports text align, even in combination with an icon. Use :SetHorizontalTextAlign() and :SetVerticalTextAlign for that. Moreover, :SetFitToContentX(true) and :SetFitToContentY(true) can be used to size this element to the size of the text and icons. This also works for buttons, making it really useful when supporting localization.

Since everything now properly inherits, the same settings are also available for buttons for example.

6. Documentation

We never added any documentation to vgui. The rewrite aims to add a lot of it - at least to the base classes.

7. Other Ideas

While researching this, I also found this library which seems quite nice: https://github.com/Threebow/better-derma-grid

@TimGoll TimGoll added the type/enhancement Enhancement or simple change to existing functionality label Jan 30, 2025
@nike4613
Copy link
Contributor

I've been on-and-off working on a framework to replace VGUI as much as possible myself. It has a lot of work to go (it can't even show anything yet!), but I have a vision for how I want it to be used:

{
    ty = "box",
    margin = 5,
    fill = true,
    {
        ty = "box",
        align = TOP,
        height = 20,
        fill = true,
        {
            ty = "text",
            translate = true,
            text = "some_text_key",
            align = { CENTER, CENTER },
        },
    },
    {
        vgui = "DPiPPanelTTT2",
        set = { Player = LocalPlayer() },
        init = function(pnl) end,
    },
}

Part of the goal is also to remove the pixel-sizing that is currently pervasive, provide automatic UI scaling, more aggressively cache data, etc.

@TimGoll
Copy link
Member Author

TimGoll commented Jan 30, 2025

Uh, this is also really neat! However, this won't stop my project. In fact, they don't interfere with each other. If you ever come around doing your idea, this rework will probably make it simpler!

@nike4613
Copy link
Contributor

Something in the PR description read as asking for ideas or some such, though I can't see it now. I figured it'd be worth mentioning here that I've been working on that as a result.

@TimGoll
Copy link
Member Author

TimGoll commented Jan 30, 2025

Something in the PR description read as asking for ideas or some such, though I can't see it now. I figured it'd be worth mentioning here that I've been working on that as a result.

Yes, I asked for feedback. However, this PR is ambitious as it is - I won't start implementing a new UI framework for now :D

@TimGoll
Copy link
Member Author

TimGoll commented Jan 30, 2025

Also, I want to mention something. The changes in this PR bring minimal issues with compatibility. Hooks, functions etc mostly stayed the same, I only introduced a new layer of interacting with them on top. While cleaning up the backend. If any addon actually uses our vgui, it should work without changes, maybe minimal changes

@TimGoll
Copy link
Member Author

TimGoll commented Feb 2, 2025

stylua sometimes handles these snippets in a bad way, not sure if there is anything that could be done about this. Most of the time it is as it is in the second example, but sometimes it is broken.

image

For vgui elements created with :Add this issue also exists. I worked around this by doing this:

image

@TimGoll
Copy link
Member Author

TimGoll commented Feb 6, 2025

image
More Panels: I added more panels to the testing suite. These only contain description or icon and make use of the new padding feature.

The first three have a fixed panel size and use text align, the last three have no size defined, only an icon size and padding. The panel size is then derived from these parameters

@TimGoll
Copy link
Member Author

TimGoll commented Feb 10, 2025

image
Stretch with vertical alignment: Panels now also support vertical alignment by setting PANEL:SetVerticalAlignment(true). All the other parameters are the same. And of course this works the same for buttons ans stretch panels.

Note: That the first three elements look broken is intended. The vertical height is too narrow to fit the content, auto stretch is disabled.

@TimGoll
Copy link
Member Author

TimGoll commented Feb 10, 2025

I also removed the background bleeding for outlines with rounded corners:

old:
image

new:
image

zoomed in (old vs new):
image

@TimGoll
Copy link
Member Author

TimGoll commented Feb 10, 2025

image

I also started converting everything to the new system

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement Enhancement or simple change to existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants