#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 )
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)
Change History (32)
comment:1 by , 9 years ago
Description: | modified (diff) |
---|
comment:2 by , 9 years ago
Keywords: | simple added |
---|
comment:3 by , 8 years ago
Owner: | set to |
---|
by , 8 years ago
Attachment: | team_exploration_score.patch added |
---|
comment:4 by , 8 years ago
Keywords: | review patch added |
---|---|
Milestone: | Backlog → Alpha 21 |
Priority: | Nice to Have → Should Have |
Summary: | Inconsistent exploration score for teams in summary gui → [PATCH]Inconsistent exploration score for teams in summary gui |
comment:5 by , 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 , 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 , 8 years ago
Attachment: | team_exploration_score_v2.patch added |
---|
The same as before but in a bit more elegant way ;)
comment:7 by , 8 years ago
Keywords: | review added |
---|
comment:8 by , 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 , 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.
comment:10 by , 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 , 8 years ago
Attachment: | team_exploration_score_v4.patch added |
---|
changes g_TeamMiscHelperData to g_TeamHelperData to be used in all panels and fixes the team exploration score
comment:11 by , 8 years ago
Keywords: | review added |
---|
by , 8 years ago
Attachment: | team_exploration_score_v4.1.patch added |
---|
changes g_TeamMiscHelperData to g_TeamHelperData to be used in all panels and fixes the team exploration score
comment:12 by , 8 years ago
changed if (!g_TeamHelperData[playerState.team].hasOwnProperty("food"))
to if (!g_TeamHelperData[playerState.team].food)
etc.
comment:13 by , 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 , 8 years ago
What will this improve? Should the data of the single players also calculated by this function?
by , 8 years ago
Attachment: | team_exploration_score_v5.patch added |
---|
Move teamHelperData calcuations into an seperate file and fix the map exploration score
comment:15 by , 8 years ago
Keywords: | review added |
---|
follow-up: 17 comment:16 by , 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 , 8 years ago
Attachment: | team_exploration_score_v5.1.patch added |
---|
Move teamHelperData calcuations into an seperate file and fix the map exploration score
comment:17 by , 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
follow-ups: 19 20 comment:18 by , 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;
comment:19 by , 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 , 8 years ago
Attachment: | team_exploration_score_v5.2.patch added |
---|
Move teamHelperData calcuations into an seperate file and fix the map exploration score
comment:20 by , 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 , 8 years ago
Attachment: | team_exploration_score_v5.3.patch added |
---|
Move teamHelperData calcuations into an seperate file and fix the map exploration score. Also done some cleanup.
by , 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:23 by , 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 forw > 4
) - I worked around the ugly
teamTotal += total
in one case,total = teamTotal
in the other. That meanttotal
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
fixes the team exploration score