Opened 3 years ago

Closed 20 months ago

Last modified 20 months 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 20 months ago.
fixes the team exploration score
team_exploration_score_v2.patch (1.6 KB) - added by Imarok 20 months ago.
The same as before but in a bit more elegant way ;)
team_exploration_score_v4.patch (9.8 KB) - added by Imarok 20 months 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 20 months 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 20 months 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 20 months 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 20 months 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 20 months 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 20 months 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 Changed 3 years ago by mimo

Description: modified (diff)

comment:2 Changed 3 years ago by leper

Keywords: simple added

comment:3 Changed 20 months ago by Imarok

Owner: set to Imarok

Changed 20 months ago by Imarok

fixes the team exploration score

comment:4 Changed 20 months ago by Imarok

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 Changed 20 months ago by Itms

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 Changed 20 months ago by Imarok

@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

Changed 20 months ago by Imarok

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

comment:7 Changed 20 months ago by Imarok

Keywords: review added

comment:8 Changed 20 months ago by Itms

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 Changed 20 months ago by Imarok

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 20 months ago by Imarok (previous) (diff)

comment:10 Changed 20 months ago by Itms

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.

Changed 20 months ago by Imarok

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

comment:11 Changed 20 months ago by Imarok

Keywords: review added

Changed 20 months ago by Imarok

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

comment:12 Changed 20 months ago by Imarok

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

comment:13 Changed 20 months ago by Itms

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 Changed 20 months ago by Imarok

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

Changed 20 months ago by Imarok

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

comment:15 Changed 20 months ago by Imarok

Keywords: review added

comment:16 Changed 20 months ago by Vladislav Belov

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.

Changed 20 months ago by Imarok

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

comment:17 in reply to:  16 Changed 20 months ago by Imarok

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 Changed 20 months ago by elexis

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;

comment:19 in reply to:  18 Changed 20 months ago by Imarok

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. ;)

Changed 20 months ago by Imarok

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

comment:20 in reply to:  18 Changed 20 months ago by Imarok

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

Changed 20 months ago by Imarok

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

Changed 20 months ago by Imarok

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 Changed 20 months ago by Imarok

also fixes #3561

comment:22 Changed 20 months ago by Itms

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 Changed 20 months ago by Itms

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.