Implement window snapping

Return-To-The-Roots / s25client

Return To The Roots (Settlers II(R) Clone)

http://www.rttr.info

GNU General Public License v2.0

477 stars 75 forks source link

Implement window snapping #1616

Closed falbrechtskirchinger closed 1 year ago

falbrechtskirchinger commented 1 year ago

Make windows align along edges within a certain snap distance.

Fixes #574

To-do:

[x] Add setting.
[x] Choose a sensible default distance. (8 seems reasonable. That's about what KWin uses.)
[x] Add unit tests.
[x] Rename to snapWindow() during final rebase.

falbrechtskirchinger commented 1 year ago

Adding thorough tests is quite the task. I count at least 64 window positions that could be tested. For that reason, the current tests are somewhat limited.
I looked into the performance impact with a large-ish number of windows (about 70) by measuring the time spent in IngameWindow::MouseMove() for isMoving == true: Of about 2500 samples, ~70% fell below 10us, while ~90% fell below 15us. About 1% were above 100us with 5 outliers between 3-5ms. Negligible, or at least not significant enough to warrant a spatial data structure for the windows.
My initial plan was to disable snapping by default, but users are most likely used to this function already. I chose 8 for the default snap distance after measuring the snap distance of my desktop window manager. Since the function is not yet configurable from the options screen, it might still be better to disable it by default. Opinions?

falbrechtskirchinger commented 1 year ago

Looks good so far, it's just a bit hard to understand/follow the code. Likely due to how this evolved.

Indeed, I started naming the variables before I knew the specifics of the calculations ahead.

falbrechtskirchinger commented 1 year ago

I still don't understand why we have minDist/minOffset and curMinDist/curMinOffset

I see. Since the lambdas are now called conditionally, there's no need to track distance and offset in temporaries. Additionally, with minDist not being updated the behavior was not as intended.

I've added a unit test to ensure we snap to the closest window.

falbrechtskirchinger commented 1 year ago

introduce a new type alias for the return value

Since I need this in WindowManager.h and IngameWindow.h, where would you prefer I add this alias?

SnapOffset.h (just like DrawPoint.h)
Window.h (currently not included in WindowManager.h)
Duplicate the alias in both files.
Other?

falbrechtskirchinger commented 1 year ago

I've modified your comment suggestions a bit further. See if that's OK.

Edit: Noticed I missed using SnapOffset in the WindowManager unit test. Edit 2: Same in IngameWindow::MouseLeftDown().

falbrechtskirchinger commented 1 year ago

@Flamefire When I force-push the rebased version, there's no way for you to get a diff without the changes that have already been merged to master, right? I.e., both clicking the "Compare" link as well as " Show changes since your last review" in the "Files changed" tab show a diff cluttered with stuff that's already been reviewed.

Would it be easier if you used "Update branch" to merge master yourself and I force-push my changes over that? Then "Compare" should only show my changes made during the rebase (that'd just be the rename SnapWindow() -> snapWindow()).

Flamefire commented 1 year ago

Would it be easier if you used "Update branch" to merge master yourself and I force-push my changes over that? Then "Compare" should only show my changes made during the rebase (that'd just be the rename SnapWindow() -> snapWindow()).

Good idea!

falbrechtskirchinger commented 1 year ago

Would it be easier if you used "Update branch" to merge master yourself and I force-push my changes over that? Then "Compare" should only show my changes made during the rebase (that'd just be the rename SnapWindow() -> snapWindow()).

Good idea!

Done.