Player manager and UI improvements#158
Conversation
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
ScalarVector1
left a comment
There was a problem hiding this comment.
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
| //Main.NewText(filter); | ||
|
|
||
| // Special toggle filter. | ||
| if (filter is ButtonOptionFilter buttonToggle) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
why is this a condition here? This seems counterintuitive
|
|
||
| if (Main.netMode != NetmodeID.SinglePlayer && !PermissionHandler.CanUseTools(Main.LocalPlayer)) | ||
| { | ||
| return; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
Is there a reason these extra external visible checks need to occur here?
|
|
||
| public override void Unload() | ||
| { | ||
| Terraria.On_Main.DoUpdate -= UpdateExtraTimes; |
There was a problem hiding this comment.
this is uneccisary in modern tModLoader
|
|
||
| if (type <= 0 || type >= BuffLoader.BuffCount) | ||
| { | ||
| ModLoader.GetMod("DragonLens").Logger.Warn( |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
nit: misleading comment, outline will be opposite hue/value to selected button color, not yellow
| SmartUIState topmost = GetTopmostHoveredState(); | ||
| bool result = owner is not null && ReferenceEquals(owner, topmost); | ||
|
|
||
| string elementName = element.GetType().Name; |
There was a problem hiding this comment.
These should be in the preprocessor if block as they're only relevant to the logging
|
|
||
| int index = UIStates.IndexOf(state); | ||
|
|
||
| if (index < 0 || index >= UserInterfaces.Count) |
There was a problem hiding this comment.
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?
Cleaner UI loader
Adds the new Player Manager (Browser) with new buttons:
UI interaction improvements
Misc:
Demo