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

Combine overlapping moving fleet buttons into a single FleetWnd #1957

Merged
merged 17 commits into from Feb 4, 2018

Conversation

Projects
None yet
5 participants
@LGM-Doyle
Contributor

LGM-Doyle commented Jan 25, 2018

Overview
This PR changes the selection behavior of fleet buttons for moving fleets, so that when a fleet button for a moving fleet is selected it will open a FleetWnd with the fleets in the selected fleet button and any fleet buttons it touches.

This addresses the issue from the forum, when fleet buttons are too close then it is not possible to select the underlying fleet button at any zoom level.

This PR also adds FleetWnd tracking behavior for moving fleets on subequent turns, when the fleets move. If all the fleets, or all of the selected fleets remain "close" together, the the FleetWnd will remain open and track the pack of fleets movement. "close" is defined as about the size of the fleet button at the original zoom level that the FleetWnd was opened at.

Details to Aid reviewing
The code make changes to FleetWnd and MapWnd.

The FleetWnd changes are to add m_bounding_box which determines during each Refresh() if moving fleets would still fit within m_bounding_box if it were translated to their new location. Otherwise, if closes the FleetWnd.

The MapWnd changes are in two parts:

  • When the moving fleets buttons are created they are sorted and stored into two types: on starlanes and offroad. This facilitates efficiently comparing for overlap only fleet buttons on the same starlane.
  • When fleet buttons are clicked a new function MapWnd::FleetIDsOfFleetButtonsOverlapping(fleet) returns fleet ids for overlapping buttons, with the categories of in system, moving on a starlane and off-road. These ids are used for opening FleetWnds and other actions.

LGM-Doyle added some commits Jan 24, 2018

In FleetWnd.cpp add CreateOrGrowBox(box, pt) and CouldContain(box, pt)
These are utility functions that will allow the FleetWnd to decide
whether to track moving fleet buttons, in subsequent turns.
Enable FleetWnd to track groups of moving fleets.
If the initial fleet ids passed to FleetWnd(ids, leeway) remain as close
together as their initial spacing, plus the leeway FleetWnd will track
the fleets the same as its tracking of fleets at a single system.
In MapWnd.cpp add IsOnStarlane(fleet) IsMovingOnStarlane() IsOffRoad()
These utility functions determine if a fleet is moving on or off a
starlane and will be used to group fleet buttons.
In MapWnd.cpp click to open a FleetWnd for all touching moving FleetB…
…uttons

It does this by separating fleets moving on starlanes and offroad fleets
into separate sets, so that they can be efficiently searched.
In MapWnd::DoFleetButtonsLayout() merge duplicate code.
The stationary and departing code was identical except for a single
bool.
@LGM-Doyle

This comment has been minimized.

Contributor

LGM-Doyle commented Jan 25, 2018

@dbenage-cx and @Dilvish-fo you may wish to review this PR, since you both made suggestions in the forum.

}
/** Can \p bounds encompass \p candidate? */
bool CouldContain(GG::Rect bounds, GG::Rect candidate) {

This comment has been minimized.

@geoffthemedio

geoffthemedio Jan 25, 2018

Member

somewhat misleading function name... doesn't check if candidate is or could be within, just that it is smaller or equal in size.

This comment has been minimized.

@Dilvish-fo

Dilvish-fo Jan 25, 2018

Member

If I am understanding right, this is being used to check if fleets sharing a FleetWnd have dispersed too far, from one turn to the next, to still share the FleetWnd, and that suggests a names to me like FleetsHaveNotDispersed or FleetsStillClustered.

This comment has been minimized.

@geoffthemedio

geoffthemedio Jan 25, 2018

Member

The function itself has nothing to do with fleets, so I'd have its name describe what it actually does, not how it's used in one case.

This comment has been minimized.

@dbenage-cx

dbenage-cx Jan 25, 2018

Member

Name suggestions seem easier to come up with if args are swapped: CouldRelocateWithin

This comment has been minimized.

@Dilvish-fo

Dilvish-fo Jan 26, 2018

Member

so I'd have its name describe what it actually does, not how it's used in one case.

It seems to me that I am proposing to have the name describe what it actually does in the most significant sense-- the determination that it is making, rather than the details of the logic used to make that determination. And I am puzzled about the "not how it's used in one case" -- it appears to me that this function is used in exactly two places, and in both places it is used the same general way with the same the significance.

The name CouldRelocateWithin could be technically accurate as suggested, but it would seem to me to imply a different significance and purpose for the function than what it actually has, which would make code harder to understand rather than easier.

This comment has been minimized.

@dbenage-cx

dbenage-cx Jan 26, 2018

Member

In this sense, I'd expect FleetsX(non-Fleet args) to derive fleets from args.
Would view this function with a potential to move into GG/PtRect as non-/member function if later desired for other cases.

This comment has been minimized.

@LGM-Doyle

LGM-Doyle Jan 26, 2018

Contributor

I swapped the order and named it LessThanOrEqual(). The line of code is now
if( ... && LessThanOrEqual(fleets_bounding_box, m_bounding_box)), which is hopefully clearer.

@Dilvish-fo I expanded the comment explaining the clustering, so that although the functions used are generic the intent is clear from the comment.

@dbenage-cx, I did not add this function to "PtRect.h", but I agree that the set of LessThan(), GreaterThan() etc. may prove more generally useful in future.

This comment has been minimized.

@geoffthemedio

geoffthemedio Jan 26, 2018

Member

I'd prefer SmallerThan and BiggerThan for this case.

This comment has been minimized.

@LGM-Doyle

LGM-Doyle Jan 26, 2018

Contributor

I changed it to bool SmallerOrEqual(GG::Rect ll, GG::Rect rr).

}
GG::Pt bounding_box_center{fleets_bounding_box.MidX(), fleets_bounding_box.MidY()};

This comment has been minimized.

@geoffthemedio

geoffthemedio Jan 25, 2018

Member

might make your initialization format a bit more consistent... here GG::Pt b_b_c{}, above auto f_l = GG::Pt().

This comment has been minimized.

@LGM-Doyle

LGM-Doyle Jan 26, 2018

Contributor

Fixed in two locations.

@dbenage-cx

This comment has been minimized.

Member

dbenage-cx commented Jan 25, 2018

Fleets near systems seem a more common case for me, but also generally easier to zoom in to distinctively select (not necessary for this PR imo).

From brief testing, looks great for occurrences away from systems. 👍

@LGM-Doyle

Thanks everyone for the quick reviews.

}
/** Can \p bounds encompass \p candidate? */
bool CouldContain(GG::Rect bounds, GG::Rect candidate) {

This comment has been minimized.

@LGM-Doyle

LGM-Doyle Jan 26, 2018

Contributor

I swapped the order and named it LessThanOrEqual(). The line of code is now
if( ... && LessThanOrEqual(fleets_bounding_box, m_bounding_box)), which is hopefully clearer.

@Dilvish-fo I expanded the comment explaining the clustering, so that although the functions used are generic the intent is clear from the comment.

@dbenage-cx, I did not add this function to "PtRect.h", but I agree that the set of LessThan(), GreaterThan() etc. may prove more generally useful in future.

}
GG::Pt bounding_box_center{fleets_bounding_box.MidX(), fleets_bounding_box.MidY()};

This comment has been minimized.

@LGM-Doyle

LGM-Doyle Jan 26, 2018

Contributor

Fixed in two locations.

In FleetWnd use SmallerOrEqual(rect, rect) vs LessThanOrEqual()
Smaller is more meaningful for rectangles than less.

From review #1957
@geoffthemedio

This comment has been minimized.

Member

geoffthemedio commented Jan 27, 2018

A quirk / bug: I've got two fleets in a single window, both moving to the same place. When I right click on a destination, the other is removed from the fleets window. Clicking the fleet icon again brings them back, but by opening a new fleet window of a different size than the one I started with. If I close that and click on the fleet button again, I get back the original sized fleet window with both fleets in it.
before
after_move_order_click
after_fleet_button_click

LGM-Doyle added some commits Jan 28, 2018

Change FleetWnd::SetSelectedFleets(ids) to SelectFleets()
This is simpler and consistent with the singular SetFleet(id)
Add FleetWnd::DeselectAll().
This commit removes the ambiguity of using INVALID_OBJECT_ID or 0 to
indicate deselect all and sometimes INVALID_OBJECT_ID being an error.
Fix MapWnd::FleetButtonLeftClicked over extending lifetime of FleetWnd
FleetWnd is a CUIWnd with its options stored in OptionsDB.
FleetWnd may also be a singular window depending on optons.
If FleetButtonLeftClicked creates a shared_ptr<FleetWnd> whose scope
extends past the subsequent SelectFleet(), then the FleetWnd can't be
deleted and recreated because the CUIWnd config will be unavailable.

This commit confines the lifetime of the shared_ptr<FleetWnd> to the if
clause that uses it.
Fix FleetWnd::Refresh location determination for selected ships.
As observed in review #1957 moving a fleet from a group of moving fleets
near each other but not at the exact same location, removes nearby
fleets from the FleetWnd.  This is because the check for all selected
ships in an exact location preceded the check for all ships in the same
vicinity.

This commit changes the order of location checks, so that the check for
selected ships in the same vicinity is earlier.
@LGM-Doyle

This comment has been minimized.

Contributor

LGM-Doyle commented Jan 30, 2018

Thanks for reporting the bug.

It is two bugs that are now fixed.

First, the order that the FleetWnd location as determined meant that if all of the fleets selected to travel are at one (x,y) location, then that location was used without checking further and finding that there are other fleets already in the FleetWnd within the allowed vicinity. That caused the reduction of the fleets in the window to just those selected to travel.

That is now fixed by changing the order of the determination of the location of the FleetWnd so that it checks for grouped moving fleets first and then checks for fleets in a single system.

Second, an interaction, described below, between the CUIWnd initialization from OptionsDB, the restriction of the number of FleetWnds to one and a temporary shared_ptr<FleetWnd> in FleetButtonLeftClicked causes the FleetWnd size to change.

  • CUIWnd assumes that all named windows with config information in OptionsDB will be unique and uses ".initialized" in the database, not in the window to determine if the window has been initialized. CUIWnds are marked uninitialzed in the database in ~CUIWnd().
  • If the number of FleetWnds is restricted to one, then CloseAll() is called if a second FleetWnd is created. CloseAll() calls GUI::Remove() which immediately indirectly calls ~CUIWnd() if there are no extant shared_ptrs to this window. If ~CUIWnd is not immediately called then the following created FleetWnd will not be initialized.
  • In FleetButtonLeftClicked() a temporary shared_ptr<FleetWnd> is created. The bug is that this temporary still exists when the attempt to create the new FleetWnd happens.

I fixed this by confining the scope of the created shared_ptr to its if statement.

@LGM-Doyle LGM-Doyle merged commit 6e27806 into freeorion:master Feb 4, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@LGM-Doyle LGM-Doyle deleted the LGM-Doyle:improve_combined_fleet_wnd_overlapping_buttons branch Feb 4, 2018

@Vezzra Vezzra added the status:merged label Feb 5, 2018

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