widelands-dev team mailing list archive

[Notabilis <notabilis27@xxxxxx>]

Review: Approve

Testing worked fine, one comment in the diff.

As a suggestion: What about moving spectators to the first player position on new games, too?

Diff comments:

> 
> === modified file 'src/wui/interactive_gamebase.cc'
> --- src/wui/interactive_gamebase.cc	2018-04-07 16:59:00 +0000
> +++ src/wui/interactive_gamebase.cc	2018-04-16 07:21:13 +0000
> @@ -148,6 +148,27 @@
>  	hide_minimap();
>  }
>  
> +void InteractiveGameBase::start() {
> +	InteractiveBase::start();

This call is new (as in: hasn't been done in start() before), right? Seems to just call the empty Panel:start().
Is there a reason that this is called? The previous empty override was making explicit that it isn't.

> +	// Multiplayer games don't save the view position, so we go to the starting position instead
> +	if (is_multiplayer()) {
> +		Widelands::PlayerNumber pln = player_number();
> +		const Widelands::PlayerNumber max = game().map().get_nrplayers();
> +		if (pln == 0) {
> +			// Spectator, use the view of the first viable player
> +			for (pln = 1; pln <= max; ++pln) {
> +				if (game().get_player(pln)) {
> +					break;
> +				}
> +			}
> +		}
> +		// Adding a check, just in case there was no viable player found for spectator
> +		if (game().get_player(pln)) {
> +			map_view()->scroll_to_field(game().map().get_starting_pos(pln), MapView::Transition::Jump);
> +		}
> +	}
> +}
> +
>  void InteractiveGameBase::on_buildhelp_changed(const bool value) {
>  	toggle_buildhelp_->set_perm_pressed(value);
>  }


-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1623375-multiplayer-starting-view/+merge/343287
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1623375-multiplayer-starting-view.