[http://sourceforge.net/p/ufoai/patches/601 Item 601] imported from sourceforge.net tracker on 2013-01-28 20:46:59

Step 1 was the creation of pilots depending on campaign files.
Step 2 was creation of skills.
Step 3 is implementation of skills effects ingame and number of missions / kills saved and shown.
===== Comments Ported from Sourceforge =====

====== tlh2000 (2011-02-27 19:12:54) ======

thanks a lot for your patches - but please keep an eye on our coding guidelines
====== malickufoai (2011-02-28 22:09:33) ======

I hope it helps, more are coming...
Please advise what needs to be improved in my coding, I'll do my best to comply with the guide lines.

Malick
====== tlh2000 (2011-03-01 09:09:05) ======

see here: http://ufoai.ninex.info/wiki/index.php/Coding_guidelines

it's mainly about whitespaces
====== geever (2011-03-03 22:04:09) ======

Hi,

Thanks for the patches. There is still some work on them though.
I'm damn tired but try to collect stuff were bugging me, to help (I'm late with it already :) )

#0 I think the two patches should be one. the first (0002) does almost nothing without the second (0003), but that's my opinion

#1 the second (0003) patch doesn't apply: that's probably because started to work on a separate branch and your earlier patch (0001) was applied to master with modifications so the two branch has been diverged, you should replay these changes on a fresh branch opened from master (or on master itself)

due to #1 I could only test the first patch (0002) but read both. I expected seeing pilot stats in aircraft menu but only found on employee screen -> "issue" #0

#2 panel pilot_info / visiblewhen: probably it worked the same way before, and it's a fine working construction but I would still like to mention it: We should use visiblewhen only if it's really necessary. The point is that visiblewhen conditions are checked in every rendering frame, not just on an event but every time the UI is drawn. In this case the cvar only changes on user action, so visiblewhen is suboptimal here. You can leave it as is for now but we should at least open a Feature Request for this optimalization.

#3 panel pilot_info / visiblewhen condition: the same category as #2. A strong Feature Request: We should eliminate the magic numbers and use identifiers: "soldier", "worker", "scientist", "pilot", it would make the conditions in E_EmployeeList_f easier too (nested ifs -> switch)

#4 Broken Save System: This is a serious issue, easy to solve though. The game saved with your patch cannot be loaded back. You need to extend saveCharacterConstants array with new skill tokens (cgame/save_character.h).

#5 I really don't like that HP value is called "Crash survival". Let it be "HP" and if a normal soldier have it around 50, make pilots around 30. Crash will damage pilots like soldiers (well, not necessary with the same formula) and it will determine if they survive or not.

#6 I don't see sense of the change of AIR_DestroyAircraft, it seems messy, I think it's f*cked up - but I only read the patch as it doesn't apply. Crash survival should be done in Rescue mission handling, see CP_SpawnRescueMission in cp_missions.c.

#7 Coding Guidelines
#8 Coding Guidelines
#9 Coding Guidelines
.... :)

-geever
====== malickufoai (2011-03-12 10:19:59) ======

Hello geever,

#0 : I'd like to create only 1 patch, but how ? Git seems to create them sequentially, not as a bundle.
#1 : OK, that's because I created a branch, and mixed the process of merging, commiting, patching etc... I must admit, I'm a bit lost with these ^^
#2 /3 : I only copy pasted the existing code. Optimization can be done, I agree.
#4 : I found where the save system is impacted. May be corrected.
#5 : after some reflection, I agree. The "crash survival value" was taken directly in the function. So a pilot with this value at 10, for example, will have 10% of surviving a crash.
#6 : then, I think I missed something. In AIR_DestroyAircraft, the code deletes the pilot and all onboard employees. In my current code, I do not see it calling the rescue mission. I will investigate...
#7-8-9-^44 : OK...

Thanks,

Malick
====== tlh2000 (2011-03-12 14:48:04) ======

you can do a git rebase

git rebase -i HEAD~4

now you can replace the "pick" with "f" to merge the f-marked commit into the previous commit
with rebase -i you can also delete commits by deleting the line in the text editor- or rearrange the commits by changing the order of the commits in your editor
====== geever (2011-07-10 19:53:01) ======

updated patch
====== geever (2011-07-10 19:55:28) ======

I have updated and merged the patches so it applies to 872cba305079148305833e7ff6e85fd6a32d8c9e wo problem. We still need to fix/finish parts of it as Malick has disappeared in space...

-geever
====== geever (2011-07-10 19:56:49) ======

ps. I keep it in a local branch tracking origin/master to follow the line with it.

-geever
====== geever (2012-10-23 06:48:13.598000) ======

Basic implementation of pilot skills is in master already. Closing this.

-geever