Skip to content

Conversation

@Arufonsu
Copy link
Contributor

@Arufonsu Arufonsu commented Nov 17, 2024

This pull request introduces a simplified escape menu feature with an in-game interface setting to toggle it on/off. It also fixes a long-standing bug that caused an InvalidOperationException client crash when selecting 'yes' on the Combat Warning prompt during logout/exit.

Assets (make sure to merge before this PR)

Preview:

2024-11-17_20-02-32.mp4

Updated Preview (18-11-24)

2024-11-18_23-22-42.mp4

Updated Preview (23-11-24)

2024-11-23_20-26-18.mp4

+ In game interface setting to toggle this feature.
+ Fix: "InvalidOperationException: Collection was modified (during iteration);" client crash was happening upon selecting 'yes' on Combat Warning prompt when logging out/exiting.
@Arufonsu Arufonsu added enhancement Minor feature addition or quality of life change feature request New and valid feature request bug fix labels Nov 17, 2024
@Arufonsu Arufonsu requested review from a team and WeylonSantana November 17, 2024 22:14
@Arufonsu Arufonsu marked this pull request as ready for review November 17, 2024 23:04
@Arufonsu
Copy link
Contributor Author

Added to (v0.8.0-beta.120+) Roadmap

Copy link
Member

@pandinocoder pandinocoder left a comment

Choose a reason for hiding this comment

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

I'm fine with a change in UI, but I'm not ok with the new icons.

A general menu (that merely contains settings) should not be represented by a settings icon. I'd rather it get changed to a simpler triple-line (hamburger) menu icon.

The guild icon is not obvious as to what it means to me.

This icon (while still not super clear) to me looks like a more generic guild icon, and a pixel art version of the general structure of this icon I think would be better than the flag in the assets PR: https://cdn2.iconfinder.com/data/icons/rpg-basic-set-2/512/guild-512.png

@Arufonsu
Copy link
Contributor Author

Arufonsu commented Nov 23, 2024

Preview update:

2024-11-23_20-26-18.mp4

private readonly ImagePanel mMenuBackground;

private readonly Button mMenuButton;
public readonly Button mMenuButton;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this should be done


if (simplifiedEscapeMenuSetting)
{
Interface.GameUi?.SimplifiedEscapeMenu?.ToggleHidden();
Copy link
Member

Choose a reason for hiding this comment

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

Instead I think the button should be passed as a parameter 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.

    private void MenuButtonClicked(Base sender, ClickedEventArgs arguments)
    {
        var simplifiedEscapeMenuSetting = Globals.Database.SimplifiedEscapeMenu;

        if (simplifiedEscapeMenuSetting)
        {
            Interface.GameUi?.SimplifiedEscapeMenu?.ToggleHidden(mMenuButton);
        }
        else
        {
            Interface.GameUi?.EscapeMenu?.ToggleHidden();
        }
    }

LoadJsonUi(GameContentManager.UI.InGame, Graphics.Renderer?.GetResolutionString());
}

public override void ToggleHidden()
Copy link
Member

Choose a reason for hiding this comment

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

the override version should call ToggleHidden(default)

this should become public void ToggleHidden(Control? target)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

used Button? target instead since we passing over the menu button

    public void ToggleHidden(Button? target)
    {
        if (!_settingsWindow.IsHidden || target == null)
        {
            return;
        }

        if (this.IsHidden)
        {
            // Position the context menu within the game canvas if near borders.
            var menuPosX = target.LocalPosToCanvas(new Point(0, 0)).X;
            var menuPosY = target.LocalPosToCanvas(new Point(0, 0)).Y;
            var newX = menuPosX;
            var newY = menuPosY + target.Height + 6;

            if (newX + Width >= Canvas?.Width)
            {
                newX = menuPosX - Width + target.Width;
            }

            if (newY + Height >= Canvas?.Height)
            {
                newY = menuPosY - Height - 6;
            }

            SizeToChildren();
            Open(Pos.None);
            SetPosition(newX, newY);
        }
        else
        {
            Close();
        }
    }

Comment on lines 52 to 65
var menuPosX = Interface.GameUi.GameMenu.mMenuButton.LocalPosToCanvas(new Point(0, 0)).X;
var menuPosY = Interface.GameUi.GameMenu.mMenuButton.LocalPosToCanvas(new Point(0, 0)).Y;
var newX = menuPosX;
var newY = menuPosY + Interface.GameUi.GameMenu.mMenuButton.Height + 6;

if (newX + Width >= Canvas?.Width)
{
newX = menuPosX - Width + Interface.GameUi.GameMenu.mMenuButton.Width;
}

if (newY + Height >= Canvas?.Height)
{
newY = menuPosY - Height - 6;
}
Copy link
Member

Choose a reason for hiding this comment

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

Position calculations should only happen if target is not null

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


SizeToChildren();
Open(Pos.None);
SetPosition(newX, newY);
Copy link
Member

Choose a reason for hiding this comment

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

Only set position if target wasn't null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added an early return within method if target is null already 👍🏽

@pandinocoder pandinocoder merged commit 9f8b51d into main Nov 26, 2024
1 check passed
@pandinocoder pandinocoder deleted the feature/simplified-escape-menu branch November 26, 2024 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Minor feature addition or quality of life change feature request New and valid feature request

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants