Skip to content

Player manager and UI improvements#158

Open
emyhrberg wants to merge 7 commits into
ScalarVector1:masterfrom
emyhrberg:player-manager-and-ui-improvements
Open

Player manager and UI improvements#158
emyhrberg wants to merge 7 commits into
ScalarVector1:masterfrom
emyhrberg:player-manager-and-ui-improvements

Conversation

@emyhrberg
Copy link
Copy Markdown
Contributor

Adds the new Player Manager (Browser) with new buttons:

  • go to button to teleport to a player
  • bring here button to bring a player to you
  • freeze button to freeze a player
  • shows additional stats for players and new filters and drawing options
  • a new grid and list mode toggle for player manager

UI interaction improvements

  • Now draws a highlight of the tool when opening a tool's window (DraggableUIStates,Tool,etc)
  • Added so panels being dragged are always ordered on top by updating UILoader
  • Added so panels cant be clicked if they are below other panels by updating UILoader.UpdateUI()
  • Hide tooltips if hovering another panel with SmartUIElement.CanShowTooltip and UILoader.CanShowTooltip

Misc:

  • Added custom noclip shift speeds
  • Fixed StyledScrollbar so you can drag it
  • Fixed ToggleButton to use Asset instead of a string path
  • Added better grid list toggle to Browser

Demo

image

dladmin now works properly. 👍
- Fix: Remove welcome text for non-admins
- Fix: Remove toolbars from the map for non-admins
- Fix: Remove first time setup for non-admins
- Fix: Handle silent exception: index out of range in BuffSpawner receivepacket() better and safer
- Fix: FastForward. Packet underflow is still happening but no visual bugs yet
Player manager improvements:
- Added new buttons, filters, stats, and custom UI!

UI interaction improvements
- Now draws a highlight of the tool when opening a tool's window (DraggableUIStates,Tool,etc)
- Added so panels being dragged are always ordered on top by updating UILoader
- Added so panels cant be clicked if they are below other panels by updating UILoader.UpdateUI()
- Hide tooltips if hovering another panel with SmartUIElement.CanShowTooltip and UILoader.CanShowTooltip

Misc:
- Added custom noclip shift speeds
- Fixed StyledScrollbar so you can drag it
- Fixed ToggleButton to use Asset<texture2D> instead of a string path
- Added better grid list toggle to Browser
Copy link
Copy Markdown
Owner

@ScalarVector1 ScalarVector1 left a comment

Choose a reason for hiding this comment

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

Done my best to review the general UI changes, will take a look at the player management UI in either a seperate review or if made a seperate PR there, this is quite alot of changes in one PR regardless.

Couple of things to address here potentially, overall conceptually the UI changes I find are all positive, mostly just a couple of implementation nits

Comment thread Content/GUI/Filters.cs
//Main.NewText(filter);

// Special toggle filter.
if (filter is ButtonOptionFilter buttonToggle)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Why are these specific checks needed here exactly? This seems to be an impractical implementation that will continually grow for more filter types. Im unsure why these need to occur here to start?

// This logic exists because this dragger is always present, not just when customizing. Has to be done due to
// append order effecting click priority :/
if (!CustomizeTool.customizing)
if (!CustomizeTool.customizing && CanShowTooltip)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

why is this a condition here? This seems counterintuitive


if (Main.netMode != NetmodeID.SinglePlayer && !PermissionHandler.CanUseTools(Main.LocalPlayer))
{
return;
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Do we truly want to completely hide the UI in the instance that a player does not have permissions? Are there some tools that potentially we still need to allow or could exist in the future?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

We should note this isn't any sort of security, tools can still be triggered by hotkey

}

Tooltip tooltip = UILoader.GetUIState<Tooltip>();
if (tooltip != null && tooltip.Visible)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Is there a reason these extra external visible checks need to occur here?


public override void Unload()
{
Terraria.On_Main.DoUpdate -= UpdateExtraTimes;
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

this is uneccisary in modern tModLoader


if (type <= 0 || type >= BuffLoader.BuffCount)
{
ModLoader.GetMod("DragonLens").Logger.Warn(
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

You should be able to simply use the Mod property of ModType here rather than GetMod

#if DEBUG
if (Main.netMode == NetmodeID.Server)
Mod.Logger.Info($"Sending packet for tool {DisplayName} ({Name}) from server");
//if (Main.netMode == NetmodeID.Server)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

There isn't much value in commenting this as it wont ever make it to a release build due to the preprocessor statements, If we truly think this logging is unhelpful we should look at removing it completely


if (IsActive)
{
// Draw yellow outline if the tool is active
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

nit: misleading comment, outline will be opposite hue/value to selected button color, not yellow

Comment thread Core/Loaders/UILoading/UILoader.cs Outdated
SmartUIState topmost = GetTopmostHoveredState();
bool result = owner is not null && ReferenceEquals(owner, topmost);

string elementName = element.GetType().Name;
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

These should be in the preprocessor if block as they're only relevant to the logging

Comment thread Core/Loaders/UILoading/UILoader.cs Outdated

int index = UIStates.IndexOf(state);

if (index < 0 || index >= UserInterfaces.Count)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Is there a case where these indicies could become out of sync? What happens if one of these adds or RemoveAts throws while the other does not? Perhaps we should introduce some dictioanry or other mapping structure to pair them up?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants