widelands-dev team mailing list archive

[SirVer <SirVer@xxxxxx>]

Review: Needs Fixing

Some thoughts inlined.

Diff comments:

> === modified file 'src/logic/player.cc'
> --- src/logic/player.cc	2017-03-03 18:13:55 +0000
> +++ src/logic/player.cc	2017-03-29 16:01:53 +0000
> @@ -1024,9 +1024,10 @@
>  	field.vision = fvision;
>  }
>  
> -void Player::unsee_node(MapIndex const i, Time const gametime, bool const forward) {
> +/// If 'hide_completely', fields will be marked as unexplored. Else, player no longer sees what's currently going on.
> +void Player::unsee_node(MapIndex const i, Time const gametime, const bool hide_completely,  bool const forward) {

I am a bit concerned about networking. If this is not triggered through a PlayerCommand (or through code that is executed for all players on all machines in a Game), this has the chance of causing a desync very much down the line. Is this possible?

>  	Field& field = fields_[i];
> -	if (field.vision <= 1)  //  Already does not see this
> +	if ((!hide_completely && field.vision <= 1) || field.vision < 1)  //  Already does not see this
>  		return;
>  
>  	//  If this is not already a forwarded call, we should inform allied players
> @@ -1035,13 +1036,18 @@
>  		update_team_players();
>  	if (!forward && !team_player_.empty()) {
>  		for (uint8_t j = 0; j < team_player_.size(); ++j)
> -			team_player_[j]->unsee_node(i, gametime, true);
> +			team_player_[j]->unsee_node(i, gametime, hide_completely, true);

how about hide_completetly => 'unexplore'? Also, this should be an enum class, not a bool IMHO.

>  	}
>  
> -	--field.vision;
> -	if (field.vision == 1)
> +	if (hide_completely) {
> +		field.vision = 0;
> +	} else {
> +		--field.vision;
> +		assert(1 <= field.vision);
> +	}
> +	if (field.vision < 2) {
>  		field.time_node_last_unseen = gametime;
> -	assert(1 <= field.vision);
> +	}
>  }
>  
>  /**


-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1675179-lua-hide-fields/+merge/320981
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1675179-lua-hide-fields.