Opened 9 years ago

Closed 8 years ago

Last modified 8 years ago

#3150 closed defect (fixed)

[PATCH] Inconsistent exploration score for teams in summary gui

Reported by: mimo Owned by: Imarok
Priority: Should Have Milestone: Alpha 21
Component: UI & Simulation Keywords: patch
Cc: Patch:

Description (last modified by mimo)

In the miscellaneous tab, the map exploration % is correctly computed, taking into account the common part between the different members of the team. But the exploration score displayed in the score tab is a simple sum of the exploration scores of the members of the team: the common explored area should also be taken into account there.

Attachments (9)

team_exploration_score.patch (1.8 KB ) - added by Imarok 8 years ago.
fixes the team exploration score
team_exploration_score_v2.patch (1.6 KB ) - added by Imarok 8 years ago.
The same as before but in a bit more elegant way ;)
team_exploration_score_v4.patch (9.8 KB ) - added by Imarok 8 years ago.
changes g_TeamMiscHelperData to g_TeamHelperData to be used in all panels and fixes the team exploration score
team_exploration_score_v4.1.patch (9.6 KB ) - added by Imarok 8 years ago.
changes g_TeamMiscHelperData to g_TeamHelperData to be used in all panels and fixes the team exploration score
team_exploration_score_v5.patch (9.5 KB ) - added by Imarok 8 years ago.
Move teamHelperData calcuations into an seperate file and fix the map exploration score
team_exploration_score_v5.1.patch (9.6 KB ) - added by Imarok 8 years ago.
Move teamHelperData calcuations into an seperate file and fix the map exploration score
team_exploration_score_v5.2.patch (8.8 KB ) - added by Imarok 8 years ago.
Move teamHelperData calcuations into an seperate file and fix the map exploration score
team_exploration_score_v5.3.patch (11.7 KB ) - added by Imarok 8 years ago.
Move teamHelperData calcuations into an seperate file and fix the map exploration score. Also done some cleanup.
summary_counters_v2.patch (13.5 KB ) - added by Imarok 8 years ago.
Move teamHelperData calculations into a seperate function and fix: team map exploration score, team feminisation, team barter efficency, color of team outcome and team total score. Also done some cleanup.

Download all attachments as: .zip

Change History (32)

comment:1 by mimo, 9 years ago

Description: modified (diff)

comment:2 by leper, 9 years ago

Keywords: simple added

comment:3 by Imarok, 8 years ago

Owner: set to Imarok

by Imarok, 8 years ago

fixes the team exploration score

comment:4 by Imarok, 8 years ago

Keywords: review patch added
Milestone: BacklogAlpha 21
Priority: Nice to HaveShould Have
Summary: Inconsistent exploration score for teams in summary gui[PATCH]Inconsistent exploration score for teams in summary gui

comment:5 by Itms, 8 years ago

Keywords: review removed

Hi and thanks for the patch :)

The big issue is that you shouldn't set that global variable for team computation inside the exploration score computation for each player. This is ugly and you have absolutely no guarantee that this function will be called before calculateScoreTeam. You must find another way to give the playerData bit to the team function.

Also please take a look at our wiki:Coding_Conventions, especially with respect to conditionals and loops. We always use new lines for the bodies, and we don't add the curly braces when the body is one line long. Also make sure to add a space between if and (foo).

Thanks for working on the issue!

comment:6 by Imarok, 8 years ago

@ltms: calculateMiscellaneous(counters) also uses a global variable, which is set in functions "you have absolutely no guarantee that this functions will be called before" calculateMiscelaneous

by Imarok, 8 years ago

The same as before but in a bit more elegant way ;)

comment:7 by Imarok, 8 years ago

Keywords: review added

comment:8 by Itms, 8 years ago

It's still not meaningful to call calculateMapExploration here: as you can see the other score functions don't call any of those functions that update g_TeamMiscHelperData. If you're confident that this global variable will be up-to-date when calculateScoreTeam is called, you don't have to do anything :)

I would also add a comment to the new code, in order to explain that w == 2 is the team exploration score and that it should take into account the fact that there can be overlapping explored portions.

comment:9 by Imarok, 8 years ago

The problem is g_TeamMiscHelperData only gets updated when you open the Miscelaneous tab. So I need to manually update when the score tab is opened. Therefore I think calculateExplorationScore is the best place.

Last edited 8 years ago by Imarok (previous) (diff)

comment:10 by Itms, 8 years ago

Keywords: review removed

In my opinion a proper fix will require more work. It would be nice if g_TeamMiscHelperData was nicely computed by some dedicated function, in order to be used in any of the summary tabs (and not only Miscellaneous).

Pay attention to whitespace, there are tabs in line 63 of the latest patch.

by Imarok, 8 years ago

changes g_TeamMiscHelperData to g_TeamHelperData to be used in all panels and fixes the team exploration score

comment:11 by Imarok, 8 years ago

Keywords: review added

by Imarok, 8 years ago

changes g_TeamMiscHelperData to g_TeamHelperData to be used in all panels and fixes the team exploration score

comment:12 by Imarok, 8 years ago

changed if (!g_TeamHelperData[playerState.team].hasOwnProperty("food")) to if (!g_TeamHelperData[playerState.team].food) etc.

comment:13 by Itms, 8 years ago

Keywords: review removed

The changes are nice but this is still not what I had in mind: could you add a new function that would fill g_TeamHelperData once and for all when the summary screen is opened?

comment:14 by Imarok, 8 years ago

What will this improve? Should the data of the single players also calculated by this function?

by Imarok, 8 years ago

Move teamHelperData calcuations into an seperate file and fix the map exploration score

comment:15 by Imarok, 8 years ago

Keywords: review added

comment:16 by Vladislav Belov, 8 years ago

About patch. There is an indent and comment issue, counters.js:106, and // The instead of //the. Probably, add "%" to the teamTotal after all ifs? The rest of the patch looks good.

by Imarok, 8 years ago

Move teamHelperData calcuations into an seperate file and fix the map exploration score

in reply to:  16 comment:17 by Imarok, 8 years ago

Replying to vladislavbelov:

About patch. There is an indent and comment issue, counters.js:106, and // The instead of //the. Probably, add "%" to the teamTotal after all ifs? The rest of the patch looks good.

done

comment:18 by elexis, 8 years ago

You poor soul had to go through so many iterations already, but there is still room for improvement ;)

First of all, if you do it like that, you don't need the first two lines, as the value will be overwritten anyway.

    if (!g_TeamHelperData[playerState.team].foo) 
        g_TeamHelperData[playerState.team].foo = 0;
    g_TeamHelperData[playerState.team].foo = bar;

Can't you just overwrite the value and init everything to 0 in one go?

g_TeamHelperData[playerState.team] = {
   "vegetarianFood": foo,
   ...
};

If you have to do those checks for any reason, you can still use a loop:

for (let prop in ["vegetarianFood", ...])
   if (!g_TeamHelperData[playerState.team][prop]) 
 	g_TeamHelperData[playerState.team][prop] = 0;

in reply to:  18 comment:19 by Imarok, 8 years ago

Replying to elexis:

You poor soul had to go through so many iterations already, but there is still room for improvement ;)

No problem. I'm a bit out of programming practice and it's my first time with JavaScript. ;)

by Imarok, 8 years ago

Move teamHelperData calcuations into an seperate file and fix the map exploration score

in reply to:  18 comment:20 by Imarok, 8 years ago

Replying to elexis:

Can't you just overwrite the value and init everything to 0 in one go?

g_TeamHelperData[playerState.team] = {
   "vegetarianFood": foo,
   ...
};

done

by Imarok, 8 years ago

Move teamHelperData calcuations into an seperate file and fix the map exploration score. Also done some cleanup.

by Imarok, 8 years ago

Attachment: summary_counters_v2.patch added

Move teamHelperData calculations into a seperate function and fix: team map exploration score, team feminisation, team barter efficency, color of team outcome and team total score. Also done some cleanup.

comment:21 by Imarok, 8 years ago

also fixes #3561

comment:22 by Itms, 8 years ago

Resolution: fixed
Status: newclosed

In 18026:

Fixes the handling of teams in the summary screen, along with some cleanup. Patch by Imarok, fixes #3150, #3561.

comment:23 by Itms, 8 years ago

Keywords: simple review removed
Summary: [PATCH]Inconsistent exploration score for teams in summary gui[PATCH] Inconsistent exploration score for teams in summary gui

Thanks a lot for this patch!

With respect to the latest version, I fixed some little things:

  • some spaces at the end of lines
  • an extra space before a comma and a missing one between if and (
  • I refactored a bit a place where you changed the conditionals (there was a ternary on w == 4 in the conditional for w > 4)
  • I worked around the ugly teamTotal += total in one case, total = teamTotal in the other. That meant total didn't designate the same thing each time, which is semantically bad.
  • I reworded some comments

which looks like a big number of changes but the patch was still pretty good; good job! :P

Note: See TracTickets for help on using tickets.