[Freecol-developers] map visibility

[Michael T. Pope]

I have been working on the client IGC and the way it sets the active unit and
interacts with the GUI in general. The patch is now fairly intrusive so I
have held it up for some play testing. One important oddity is that when we
move a unit there is the following:

if (!gui.onScreen(tile)) gui.setSelectedTile(tile);

I think this expresses things poorly. I do not think we really want
to select the tile, all that is needed is that this new tile where a
unit has just moved to should be made visible, and setSelectedTile is
a convenient way to do this. (Note: onScreen() does not test the map
as displayed but returns false for a certain amount of visible edge,
so it is impossible to completely move off screen without having
onScreen() fail earlier, which is probably a good thing)

ISTM it would be better to have a GUI routine that wrapped this up.
Perhaps GUI.ensureTileVisible(Tile)? It would also be useful in
IGIH.animate*(). The controller then is expressing the requirement
more clearly, and onScreen would not need to be public. Comment from
the GUI wrangler:-)?


BTW, terrain mode is working again for me following git.5f978b. I
updated the wiki.

Cheers,
Mike Pope

[w***@genial.ms]

Hi,

> Gesendet: Donnerstag, 06. August 2015 um 12:09 Uhr
> Von: "Michael T. Pope" <***@computer.org>
> An: "FreeCol Developers" <freecol-***@lists.sourceforge.net>
> Betreff: [Freecol-developers] map visibility
>
> I have been working on the client IGC and the way it sets the active unit and
> interacts with the GUI in general. The patch is now fairly intrusive so I
> have held it up for some play testing.

Well, if you would have attached a diff, I could take a look at it or
try it out.

> One important oddity is that when we
> move a unit there is the following:
>
> if (!gui.onScreen(tile)) gui.setSelectedTile(tile);
>
> I think this expresses things poorly. I do not think we really want
> to select the tile, all that is needed is that this new tile where a
> unit has just moved to should be made visible, and setSelectedTile is
> a convenient way to do this. (Note: onScreen() does not test the map
> as displayed but returns false for a certain amount of visible edge,
> so it is impossible to completely move off screen without having
> onScreen() fail earlier, which is probably a good thing)
>
> ISTM it would be better to have a GUI routine that wrapped this up.
> Perhaps GUI.ensureTileVisible(Tile)? It would also be useful in
> IGIH.animate*(). The controller then is expressing the requirement
> more clearly, and onScreen would not need to be public. Comment from
> the GUI wrangler:-)?

It might be better to not change the selected tile, but not sure if
changing that causes the screen to some weird long unvisited part
of the map when switching to the tile view mode?
I think we have several methods already, which could provide that
functionality. Maybe setFocus, setFocusImmediately, centerActiveUnit,
isTileVisible, onScreen or some others in MapViewer would help?
But the call should be routed through SwingGUI and there are some
helper methods already.

> BTW, terrain mode is working again for me following git.5f978b. I
> updated the wiki.

Was it that commit or did git.c039176c fix it already?


Greetings,

wintertime

------------------------------------------------------------------------------

[Michael T. Pope]

On Thu, 6 Aug 2015 16:00:43 +0200
***@genial.ms wrote:
> > I have been working on the client IGC and the way it sets the active unit and
> > interacts with the GUI in general. The patch is now fairly intrusive so I
> > have held it up for some play testing.
>
> Well, if you would have attached a diff, I could take a look at it or
> try it out.

Its still too flakey, and does not yet include any proposed code to fix
what I discuss below.

> > One important oddity is that when we
> > move a unit there is the following:
> >
> > if (!gui.onScreen(tile)) gui.setSelectedTile(tile);
> >...
>
> It might be better to not change the selected tile, but not sure if
> changing that causes the screen to some weird long unvisited part
> of the map when switching to the tile view mode?

Yes, that might be a problem. I will experiment.

> I think we have several methods already, which could provide that
> functionality. Maybe setFocus, setFocusImmediately, centerActiveUnit,
> isTileVisible, onScreen or some others in MapViewer would help?
> But the call should be routed through SwingGUI and there are some
> helper methods already.

OK, I missed centerActiveUnit, that sounds promising.

> > BTW, terrain mode is working again for me following git.5f978b. I
> > updated the wiki.
>
> Was it that commit or did git.c039176c fix it already?

No idea. I only had a chance to try it again yesterday, when the
commit referenced was the last relevant one.

Cheers,
Mike Pope

[Michael T. Pope]

On Fri, 7 Aug 2015 07:34:43 +0930
"Michael T. Pope" <***@computer.org> wrote:
> > I think we have several methods already, which could provide that
> > functionality. Maybe setFocus, setFocusImmediately, centerActiveUnit,
> > isTileVisible, onScreen or some others in MapViewer would help?
> > But the call should be routed through SwingGUI and there are some
> > helper methods already.
>
> OK, I missed centerActiveUnit, that sounds promising.

centerActiveUnit is too strong, but looking deeper I think it might be ok
to just get rid of the setSelectedTile() altogether. IR#115 called for
animation-off to just skip movement display, which I think makes sense and
provides another useful work around for annoying broken graphics drivers
that cause animation to become glacial, and FreeCol to be blamed. So we
then only need worry about whether a tile is on screen when animation is
occurring, which means the decision to refocus during movement can be
handled entirely by the animation routines and not by IGC at all. I have
implemented IR#115, and am play testing dropping setSelectedTile from
moveMove.

Meanwhile we seem to have broken the initial unit selection after loading
a game. I do not know why, but I know where that should be
happening.

Cheers,
Mike Pope

[Michael T. Pope]

On Fri, 7 Aug 2015 07:34:43 +0930
"Michael T. Pope" <***@computer.org> wrote:
> On Thu, 6 Aug 2015 16:00:43 +0200
> ***@genial.ms wrote:
> > > I have been working on the client IGC and the way it sets the active unit and
> > > interacts with the GUI in general. The patch is now fairly intrusive so I
> > > have held it up for some play testing.
> >
> > Well, if you would have attached a diff, I could take a look at it or
> > try it out.
>
> Its still too flakey...

However part of it has been sorted out, so there is an attached diff this
time. There are many calls to updateMenuBar almost always associated with
a call to FreeColClient.updateActions (although the ordering is
sometimes inconsistent!). IGC quite rightly does this after almost
every action through IGC.updateControls. There is no reason to
separate the action update from the menu bar update, and clearly the
actions need to be correctly set first. The same applies to resetMenuBar.
The patch therefore moves the updateActions call into the GUI-level routine.

Selecting the active unit or tile can clearly change the available
user actions, so I have likewise made sure setActiveUnit and
setSelectedTile call gui.updateMenuBar, which simplifies the
corresponding MapViewer routines.

Cheers,
Mike Pope

[Michael T. Pope]

On Tue, 11 Aug 2015 09:43:40 +0200
"Sebastian Zhorel" <sebastian-***@email.de> wrote:
> Just fyi: I read the diff and it looks ok to me;
> I still need to try it out though.

Have you had a chance to look at this yet? Its surviving my testing.

Cheers,
Mike Pope

[w***@genial.ms]

> > Just fyi: I read the diff and it looks ok to me;
> > I still need to try it out though.
>
> Have you had a chance to look at this yet? Its surviving my testing.

Its no big change, just moving the update calls around a bit. ;)
I did not do much playtesting lately and tried the diff only very shortly
without seeing a problem and it works for you.
But I read the diff more than once and verified that the methods in
MapViewer are only called from SwingGUI with the removed update calls
done there. I'd say apply it!

------------------------------------------------------------------------------