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.

Sign up for GitHub

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

Jump to bottom

Fix Separated HUD bugs and set limits to keep UI pretty #33482

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Fix Separated HUD bugs and set limits to keep UI pretty #33482

wants to merge 17 commits into from
+137 −49

Conversation

SpaceManiac
Copy link
Contributor

@SpaceManiac SpaceManiac commented Nov 23, 2024 •
edited
Loading

About the PR

Sets limits on the widths of the left/right halves of the Separated HUD that avoid letterboxing and UI overlaps, and fixes some related bugs. It's still imperfect, but it should be an improvement over status quo.

Technical details

  1. Remember the selected width. Fixes Separated HUD Layout remembers your chat width, but does not start with it #25494.
  2. Fix top menu buttons overlapping each other at smaller chat widths.
  3. Dynamically assign a min/max for the separated split based on several factors:
    • Minimum at 15-tile-wide viewport at current effective viewport scale
    • Minimum at 1060 pixels, below which inventory UI elements begin to overlap
    • Soft maximum at 21-tile-wide viewport at current effective viewport scale
    • Soft maximum at screen width minus actual width of the chat
    • Hard maximum at screen width minus 372 pixels (width of the chat for non-admins)
  4. Fix SplitContainer's clickable and visual widths desyncing after a UI scale settings change.

See also space-wizards/RobustToolbox#5529

Media

Before video:

2024-11-22.23-42-51.mp4

After video:

2024-11-22.23-47-32.mp4

Requirements

Changelog
🆑

  • tweak: Fix some visual bugs with the Separated HUD, and set limits that make letterboxing less likely.

@github-actions github-actions bot added size/M Denotes a PR that changes 100-999 lines. Changes: UI Changes: Might require knowledge of UI design or code. S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. labels Nov 23, 2024
@beck-thompson beck-thompson added T: Bugfix Type: Bugs and/or bugfixes P2: Raised Priority: Item has a raised priority, indicating it might get increased maintainer attention. D2: Medium Difficulty: A good amount of codebase knowledge required. S: Needs Review Status: Requires additional reviews before being fully accepted and removed S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. labels Nov 23, 2024
Aeshus
Aeshus requested changes Nov 23, 2024
View reviewed changes
Copy link
Contributor

@Aeshus Aeshus left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the PR!

When I first tested this version, it forced itself to 45% of my screen and wouldn't let me move it.

image

After messing around in settings for a while, clicking random buttons, it seems to have fixed itself for full-screen. I could now resize it fine and it wasn't 45% of my screen.

image

Though, after the full-screen got fixed, my floating version broke as the chat now overlapped the actions and the chat was going off of the screen.

image

I suspect there's an issue with the viewport scaling & how it interacts with the settings, as when I toggle between the settings, it occasionally breaks and then fixes itself for different resolutions.

@SpaceManiac
Copy link
Contributor Author

SpaceManiac commented Nov 25, 2024 •
edited
Loading

when I toggle between the settings, it occasionally breaks and then fixes itself for different resolutions.

I spent some more time analyzing how the settings interact and now have a scale calculation that should work more reliably.

The only caveat is that it ignores "Vertical Viewport Fitting" (always acts as if it's on), but I don't think there's a good way around this, because doing otherwise would introduce a circular dependency.

@github-actions github-actions bot added size/L Denotes a PR that changes 1000-4999 lines. S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted and removed size/M Denotes a PR that changes 100-999 lines. labels Nov 25, 2024
@github-actions GitHub Actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@SpaceManiac SpaceManiac mentioned this pull request Nov 26, 2024
Merged
@SpaceManiac
Copy link
Contributor Author

Conflict resolution pending #33576 settling

@SpaceManiac
Copy link
Contributor Author

Revert of #33047 has deconflicted this PR

@github-actions github-actions bot removed the S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted label Dec 9, 2024
@Errant-4
Copy link
Member

Errant-4 commented Dec 9, 2024 •
edited
Loading

When I first tried it, it worked as expected. But when I switched to the Default layout and then back, it went to a squashed size and I was no longer able to resize it.
After restarting the game, it went back to normal and I was unable to reproduce the error. Sounds like the same/similar issue to what Aeshus was describing

@SpaceManiac
Copy link
Contributor Author

Unfortunately I'm not able to reproduce the problem

Aeshus
Aeshus reviewed Jan 23, 2025
View reviewed changes
Copy link
Contributor

@Aeshus Aeshus left a comment

Choose a reason for hiding this comment

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

Are you still interested in working on this?

I can walk through causing the bug this week if you're interested.

@SpaceManiac
Copy link
Contributor Author

Yes, if the overall direction of the change still works for you, I'm willing to figure out this detail.

@github-actions github-actions bot added the S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted label Jan 29, 2025
@github-actions GitHub Actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added size/M Denotes a PR that changes 100-999 lines. and removed size/L Denotes a PR that changes 1000-4999 lines. S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted labels Jan 30, 2025
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 6, 2025

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted label Feb 6, 2025
@github-actions github-actions bot added S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted and removed S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted labels Feb 6, 2025
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 8, 2025

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted label Feb 8, 2025
metalgearsloth
metalgearsloth reviewed Feb 11, 2025
View reviewed changes
Content.Client/UserInterface/Screens/SeparatedChatGameScreen.xaml.cs
Comment on lines +20 to +25
// The minimum width of InventoryGui + HotbarGui combined.
// Used to keep the PDA from overlapping the shoes slot.
private const int INVENTORY_MINIMUM_WIDTH =
(125 /* width */ - 2 /* negative margin */) * 2 /* ItemStatusPanel */
+ 68 /* width */ * 10 /* ItemSlotButton, including inventory */
+ 67 /* width */ * 2 /* hands */;
Copy link
Contributor

Choose a reason for hiding this comment

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

These values are bound to fall out of line so you can't just rely on magic numbers like this.

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

@metalgearsloth metalgearsloth metalgearsloth left review comments

@Aeshus Aeshus Aeshus requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
Changes: UI Changes: Might require knowledge of UI design or code. D2: Medium Difficulty: A good amount of codebase knowledge required. P2: Raised Priority: Item has a raised priority, indicating it might get increased maintainer attention. S: Needs Review Status: Requires additional reviews before being fully accepted size/M Denotes a PR that changes 100-999 lines. T: Bugfix Type: Bugs and/or bugfixes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Separated HUD Layout remembers your chat width, but does not start with it
5 participants
@SpaceManiac @Errant-4 @metalgearsloth @Aeshus @beck-thompson