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

No/configurable "Lip" around windows #6568

Closed
DorpsGek opened this issue May 28, 2017 · 8 comments
Closed

No/configurable "Lip" around windows #6568

DorpsGek opened this issue May 28, 2017 · 8 comments
Labels
component: interface This is an interface issue flyspray This issue is imported from FlySpray (https://bugs.openttd.org/)

Comments

@DorpsGek
Copy link
Member

SirkoZ opened the ticket and wrote:

An interface annoyance that I've been meaning to report for some time now I would like to report:

Task windows in OpenTTD are displayed with a certain distance from each other ("lip" a few pixels wide) -
one can join them together by hand however it would be convenient if they were already displayed one near another
without the space between them. Of course that could be a setting / lip or no lip...

Attachments

Reported version: trunk
Operating system: All


This issue was imported from FlySpray: https://bugs.openttd.org/task/6568
@DorpsGek
Copy link
Member Author

3298 wrote:

This has bothered me a bit too, so I tracked down the code responsible for this. It's in window.cpp, function GetAutoPlacePosition. There are several occurrences of the number 2 in this function; these cause the windows to spawn with a distance of 2 pixels. Changing the distance (or removing it) is obviously as easy as changing these numbers.
I'm not providing a quick patch this time because I'm not sure what the best course of action is: removing the distance entirely? making a setting for it (GUI or openttd.cfg-only)? only making it a constant for source-level configuration?
If anything is changed here, something should also be done about that serious infestation of magic numbers in this function and some nearby related functions. As an example, IsGoodAutoPlace2 contains the unexplained condition top < 22 (which IsGoodAutoPlace1 used to contain as well, but it was replaced by (main_toolbar != NULL && top < main_toolbar->height) at some point; this change also kind of explains the starting point (0, 24) used in the last window placement attempt (note: this attempt will eventually place windows outside the screen!)), and LocalGetWindowPlacement has multiple instances of 10, 20, and a 60.
There is a somewhat related task here, #5451. It's about positioning child windows in relation to their parent windows; a clean solution for that likely requires edits in LocalGetWindowPlacement, right where those 10s and 20s are. I'd like to do these two tasks in one go. What's the devs' opinion on the window spawn distance?


This comment was imported from FlySpray: https://bugs.openttd.org/task/6568#comment14453

@DorpsGek
Copy link
Member Author

DorpsGek commented Jul 4, 2017

frosch wrote:

I think #5451 is way more complicated, so I don't think combining the tasks makes sense.

Attached is a suggested fix for this task:
- The 2 pixel border is removed. Windows snap to 0 distance, so the distance on spawn makes no sense.
- Replaced the constants for the toolbar height with the actual toolbar height.
- Replaced the offsets for child positions with the actual size of the closebox.
- Make the positioning of windows text-direction dependent. With RTL languages the windows will be positioned starting from the right.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/6568#comment14460

@DorpsGek
Copy link
Member Author

DorpsGek commented Jul 5, 2017

3298 wrote:

My approval for the patch probably isn't worth a lot, but you have it. :) I'd have thought a dev would just tune in, write about their opinion, and leave again until I present the patch I more-or-less promised above. I wouldn't have thought of the flipped interface for RTL languages, though, so ... thanks!
Just to explain: I had a slight overhaul of the window placement code in mind (though pretty much imperceptible to the user unless they fill the screen with windows), mostly doing away with the final placement attempt and instead scoring each placement according to what speaks against it (number of overlapping windows, penalty for being halfway outside the screen, ...) and just taking the best one. You know, just one of my overengineered solutions like the transparency window a few years ago, if you remember that. ;)
With that in place, #5451 would be as simple as giving placement next to the parent window a favorable score and removing the special-case placement with a fixed offset to the parent window; the effect should be that the child window is placed next to the parent (i.e. not overlapping, which was the complaint in #5451) unless that space is already cluttered (then it would be subject to normal placement; by the way, if I'm reading it right your patch introduces a similar kind of fallback: if the child window goes outside the screen, it falls through to the normal placement, the old code would simply ignore the bottom edge (bad!) and move a bit to the left when hitting the right edge).

I can easily put those changes on top of your patch, some of the magic number fixes may even be helpful (by explaining). Don't hesitate to commit the patch even if you want my proposed changes.


This comment was imported from FlySpray: https://bugs.openttd.org/task/6568#comment14461

@DorpsGek
Copy link
Member Author

DorpsGek commented Jul 5, 2017

3298 wrote:

Just noticed this one: You missed a place where the text direction should be respected. In IsGoodAutoPlace2 the window is allowed to go partially off-screen, but how far differs for left and right (left 25%, right 50%). For RTL this should probably be swapped.


This comment was imported from FlySpray: https://bugs.openttd.org/task/6568#comment14462

@DorpsGek
Copy link
Member Author

DorpsGek commented Jul 5, 2017

SirkoZ wrote:

All I can say to both of you is thank you for such quick response and even a working patch. OpenTTD has never been so good.


This comment was imported from FlySpray: https://bugs.openttd.org/task/6568#comment14463

@DorpsGek
Copy link
Member Author

DorpsGek commented Jul 6, 2017

3298 wrote:

Edit: please ignore - apparently refreshing the page after submitting a comment re-submits it.

Edit2: Actually, I'll turn this comment into a useful one. Firstly, you also broke the placement of child-windows of construction toolbars - add the "return pt;" back into the if-branch checking for WC_BUILD_TOOLBAR and WC_SCEN_LAND_GEN to fix that.

Other than that, I finished the patch that places windows according to scores (attached).
- The first two hunks allow me to ask for the Z priority of a window before I have a Window class for it. The relevant data already exists in WindowDesc, which is available during placement.
- The next hunk is pretty large. At the start you see a few constants for the placement penalties. Feel free to tweak them, the documentation should be enough to tell you what they do.
- The next part of that hunk merges IsGoodAutoPlace1 and IsGoodAutoPlace2 into a single function. The difference between those functions was fairly small already, and with my scoring system the merged function tries to do it the way of IsGoodAutoPlace1, falling back to what once was IsGoodAutoPlace2 if the screen bounds check of IsGoodAutoPlace1 fails (while increasing the penalty, obviously). The loop at the end, which prevented placing a window on top of another, was changed to just count the overlapping windows and increase the penalty for each. I also removed your new toolbar_y stuff in favor of a simple condition in the overlap-checking loop (the toolbars have high Z priority; placement below windows with higher Z priority than the new window is now forbidden) - I see no reason to prevent placement next to the toolbar, which the old code did. While at it, I made the code ignore overlap with tooltips as well (those made for some weird placement before). The merged function returns the penalty instead of true/false now, and it needs a few more parameters.
- Still in the same hunk, GetAutoPlacePosition needs to keep track of the penalty of the best placement yet. This is passed by-reference to IsGoodAutoPlace alongside pt. The first placement attempt (top left, RTL-aware) now tries a true top-left position (next to the toolbar), only falling back to toolbar_y pixels lower if that fails.
The second and third attempts were merged like the functions they depended on. You'll notice a line dealing with window parents here - this is what persuades the game to place child windows next to parents by increasing the penalty if the window has a parent and this spot is not next to it, before the penalty-checking function IsGoodAutoPlace is even started (because that has no access to the parent, and it makes no sense to pass the parent reference down 8 times when the penalty can be calculated 1 time).
I kept the return statements in these attempts - they are triggered if the penalty is 0, which is a perfect spot. If the two attempts run through without a perfect result, I take the best one. Only when no spot worked (screen covered in higher-priority windows?), a third attempt is started, which simply places the window at (0, toolbar_y) so it can be moved / closed, at least when the movable higher-priority windows are out of the way. (Toolbar and statusbar are immovable, but using toolbar_y avoids the toolbar and keeps closebox and caption away from the statusbar.) This replaces the old fourth attempt, which went down-right for each subsequent window, completely ignoring the screen edges. I considered this final attempt impossible enough to put a console warning into it in case it was ever needed.
- At the end of the hunk, a small comment typo I noticed along the way is fixed.
- The final two hunks change LocalGetWindowPlacement so it passes a few more parameters down to GetAutoPlacePosition, and remove the child window special case (except for the child windows of the construction toolbars) so the penalty-based placement can take over, which favors spots next to the parent. That's #5451.
Oh, the two tiny issues mentioned at the top of this comment and my previous comment are fixed in my patch, too. Not a big deal, though.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/6568#comment14464

@DorpsGek
Copy link
Member Author

frosch wrote:

Updated patch for priority and positioning in #5451.


This comment was imported from FlySpray: https://bugs.openttd.org/task/6568#comment14652

@DorpsGek
Copy link
Member Author

frosch closed the ticket.

Reason for closing: Implemented

in r27900


This comment was imported from FlySpray: https://bugs.openttd.org/task/6568

@DorpsGek DorpsGek added component: interface This is an interface issue flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) enhancement labels Apr 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: interface This is an interface issue flyspray This issue is imported from FlySpray (https://bugs.openttd.org/)
Projects
None yet
Development

No branches or pull requests

1 participant