UFO 2.5 Dev Production/Market Balance

[andrenal]

I believe campaigns now have a producerate variable, which can be used to make production quicker or slower across all items.

Don't you think we could have at standart difficult a balanced production with production costs near buy price, sometimes with a little profit, sometimes without but still good to produce but not to sell?

H-Hour, could you set up salaries for all difficulty levels? As I see it's missing at almost every but main. Probably someone thought "it will be the same as main if not set" but it's not true.

-geever

geever, what did you offer to me to fix, scripts or source code? I understood source code ...

[H-Hour]

H-Hour, could you set up salaries for all difficulty levels? As I see it's missing at almost every but main. Probably someone thought "it will be the same as main if not set" but it's not true.

Not at the moment. I am only able to check internet in brief occassional moments for a few weeks.

[geever]

geever, what did you offer to me to fix, scripts or source code? I understood source code ...

The code. For the scripts we can either copy the current hardcoded defaults or choose some reasonable.
I just want to eliminate the hardcoded part.

-geever

[Nokim]

1. Eliminate those hardcoded defaults. My idea is to set them to -1 in the code and check the values after the parser and drop campaign definitions which don't have the necessary values set. (for this we need to set reasonable values to campaign.ufo for other difficulty levels too)

It's simple, yes. The only thing i don't like in this - many copy/paste in campaign.ufo so much more thing to not forget to change for balance. Right now soldier's salary is 1500 in normal (rankbonus 0) and 3000 on easy (rankbonus 500 from hardcoded values).

2. Fix the Rankbonus logic, it's completely broken. Rankbonus shouldn't rely on the order of the definitions in the ufo file, rather use it's factor property or so.

Added rank_no to medals.ufo and using it instead of index. Problem is rank and employee type stored in different structures. Without redesign there is unnecessary search for parent structure to get employee type.

Calling debug_listhired in console crashes game after listing soldiers.

I'm not familiar with patches but tried to implement changes from above. Side effect - promotion of soldiers in existing saves by 4 ranks. And there is no any check for that.

[Nokim]

Err... Nobody interested?

[geever]

Were the patches uploaded last time either? Nevermind.

You're mixing the meaning of the index (Idx) of ranks which isn't good.

You should create an own function:

Code: [Select]

rank_t *RANK_GetByLevel (employeeType_t type, int level)
(rank_no should be "level" imho)

Also you should read our coding guidelines.

-geever

[Nokim]

You should create an own function:

Code: [Select]

rank_t *RANK_GetByLevel (employeeType_t type, int level)


The question is if we should change meaning of chr.score.rank. If no then

Code: [Select]

rank_t *CL_GetRankByIdx (const int index)just useless, if yes then new function like

Code: [Select]

rank_t *CL_GetRankLevelByIdx (const int index)required. Anyway my implementation is not perfect 

(rank_no should be "level" imho)

No problem.

Also you should read our coding guidelines.

Can you say most typical mistake?

Coding tips & FAQ really lacks info about using git and specially about work with patches and stash.

[geever]

The question is if we should change meaning of chr.score.rank. If no then

Code: [Select]

rank_t *CL_GetRankByIdx (const int index)just useless,

chr.score.rank should point to an exact rank, while level (or rank_no) is to define a promotion order between ranks. So the meaning of chr.score.rank should not be changed BUT CL_GetRankByIdx is not useless either. We wanna hide the underlying datastructure.

If we use &ccs.ranks At time we change the datastructure we will need to search and replace all these references. If we use CL_GetRankByIdx, only the body of that function needs to be changed.

I've already put changing ranks datastructure to a linked list on my personal wishlist so it's not limited to an artificial number (32 now).

You should also not forget, that level is scripted, so 2 (or more) ranks can have the same level (even for the same employee category) unless we put extra check on that in ranks parser. But why would we? I think it's more flexible to allow more ranks of the same level maybe with different requirements and let the system choose randomly on promotion from which are met.

Can you say most typical mistake?

Mostly whitespace errors. It makes easier to read the code if you separate arguments of a funtion and so, like

Code: [Select]

- const rank_t *rank = CL_GetRankByIdx(chr->score.rank,EMPL_SOLDIER);
+ const rank_t *rank = CL_GetRankByIdx(chr->score.rank, EMPL_SOLDIER);

-        for (int i=0;i<ccs.numRanks;i++)
-            if (ccs.ranks[i].rank_no==index && ccs.ranks[i].type==type)
+        for (int i = 0; i < ccs.numRanks; i++)
+            if (ccs.ranks[i].rank_no == index && ccs.ranks[i].type == type)

Also, please put comments in separate lines before the thing it describes.

And check the gudeline of the Git commit comment:

Code: [Select]

* summary of the commit

* some detail notes if you feel like

The emplty line is important so as the asterisk at each line.

Coding tips & FAQ really lacks info about using git and specially about work with patches and stash.

The article Getting the source contain some information about using Git.

Your patch was about right but... there is always a but

That two patches should be two but not in this way. As you fixed problems in the first patch with the second, they should be merged so it's easier to understand - you can use interactive rebasing for that:

Code: [Select]

git rebase -iBut you put two different things in the first commit:
* the campaign parameters patch
* and (part of) the ranks patch

These should be splitted. Interactive rebasing can help in this one too.

Git help: http://www.kernel.org/pub/software/scm/git/docs/git-rebase.html

one more about the code:

Code: [Select]

+      s->base[EMPL_SOLDIER] = -1;
+ s->rankBonus[EMPL_SOLDIER] = -1;
+ s->base[EMPL_WORKER] = -1;
+ s->rankBonus[EMPL_WORKER] = -1;
+ s->base[EMPL_SCIENTIST] = -1;
+ s->rankBonus[EMPL_SCIENTIST] = -1;
+ s->base[EMPL_PILOT] = -1;
+ s->rankBonus[EMPL_PILOT] = -1;
+ s->base[EMPL_ROBOT] = -1;
+ s->rankBonus[EMPL_ROBOT] = -1;

You could/should use a loop on employeeTypes for things like this. Shorter, better.

-geever

[Nokim]

Thanks for detailed answer. That's what i wanted.

chr.score.rank should point to an exact rank

But that's integer value which fully depends on order ranks is parsed. Maybe it should be saved/loaded as string (rank name) and used as integer only at run time?

The article Getting the source contain some information about using Git.

Yes, there is some info about getting source. But here is not enough info about how contribute it back.

And i always wonder why you don't use std::vector, std::list, etc. Specially vector. This should be great help for storing data with same access speed as static array.

[Nokim]

OK, patches rewritten. I didn't touched promotion logic but it should be changed. It still relies on fact that higher rank have higher index.

[Sandro]

And i always wonder why you don't use std::vector, std::list, etc. Specially vector. This should be great help for storing data with same access speed as static array.

First, the whole project just got converted to C++ from C.
Second, STL containers are pointer unsafe: they can and will move data around, requiring you to have an extra level of indirection. Not to mention the extra fun of dynamically allocating memory in the middle of pseudo-realtime code (and we are CPU limited already). So for us, in many cases they are slower than static arrays.
Third, unless you typedef them, all the code turns into unreadable clutter of <>::

Said that, some of the code really could use STL containers. For example, replacing our linked list implementation with std::list probably could be useful; at least we can be sure that it is 100% debugged (we had problems with our implementation before, like crashes or data corruption in case of empty list).

[geever]

OK, patches rewritten. I didn't touched promotion logic but it should be changed. It still relies on fact that higher rank have higher index.

Your first patch have been added to master. It had a few whitespace errors but corrected. As I see there is still a few things in campaign_t that have hardcoded defaults.

The second patch is two patches actually. You're right on that ranks' id should be saved, not the index, but implementation is not fully correct. You shouldn't include a cp_ stuff from outside campaign code. The different game modes can be compiled to be different DLL/SO modules and such cross reference could make problems with linking.

There is two ways to resolve this issue

• Make ranks a campaign only by moving chr.score.rank into the employee datastructure and savesystem
• Make ranks common by moving the rank handling (cp_ranks.cpp/h) outside of campaign

I've discussed the matter with Mattn and H-Hour before and chose the second.

The first part of your second patch about rank levels is in master too, but not yet the save stuff.

-geever

[Nokim]

The second patch is two patches actually.

It was five patches merged in one. But how it was supposed to be divided?

• Make ranks common by moving the rank handling (cp_ranks.cpp/h) outside of campaign

Move files to cgame directory, rename files accordingly, correct inclusions, move ranks data from campaign to cl_rank.cpp as static var, eliminate remaining direct data access in other parts of game. Is that all? Oh, and something wrong with making - it still searches cp_rank.cpp in old location.

[Nokim]

I didn't rename rank functions. Or i should replace CL_ with GAME_ ?
And i'm not sure that i did all required modifications to makefiles. Game is building fine and running.

[Mattn]

nice - but we should go one step futher. the rank code should be something where the game code has access to, too - not only the cgame. this isn't a client-only feature (in the future at least)

so maybe move this close to e.g. chr shared code?