Merge pull request #727 from stm2/bug2355

simplify academy teaching code, addressing bug 2335

src/academy.c

@@ -22,13 +22,14 @@ OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
 #include <kernel/building.h>
 #include <kernel/item.h>
 #include <kernel/pool.h>
+
 #include "academy.h"
 #include "study.h"
 
-void academy_teaching_bonus(struct unit *u, skill_t sk, int student_days) {
+void academy_teaching_bonus(struct unit *u, skill_t sk, int students) {
-    if (student_days && sk != NOSKILL) {
+    if (students && sk != NOSKILL) {
-        /* actually days / STUDYDAYS * EXPERIENCEDAYS / MAX_STUDENTS */
+        /* actually students * EXPERIENCEDAYS / MAX_STUDENTS */
-        learn_skill(u, sk, student_days / STUDYDAYS); 
+        learn_skill(u, sk, students);
     }
 }
 

src/items.c

@@ -74,9 +74,9 @@ struct order *ord)
             if (amount > MAXGAIN) {
                 amount = MAXGAIN;
             }
-            teach->value += amount * STUDYDAYS;
+            teach->days += amount * STUDYDAYS;
-            if (teach->value > MAXGAIN * STUDYDAYS) {
+            if (teach->days > MAXGAIN * STUDYDAYS) {
-                teach->value = MAXGAIN * STUDYDAYS;
+                teach->days = MAXGAIN * STUDYDAYS;
             }
             i_change(&u->items, itype, -amount);
             return 0;

src/study.c

@@ -196,89 +196,51 @@ static int study_days(unit * student, skill_t sk)
 
 static int
 teach_unit(unit * teacher, unit * student, int nteaching, skill_t sk,
-    bool report, int *academy)
+    bool report, int *academy_students)
 {
     teaching_info *teach = NULL;
     attrib *a;
-    int n;
+    int students;
-
-    /* learning sind die Tage, die sie schon durch andere Lehrer zugute
-     * geschrieben bekommen haben. Total darf dies nicht ueber 30 Tage pro Mann
-     * steigen.
-     *
-     * n ist die Anzahl zusaetzlich gelernter Tage. n darf max. die Differenz
-     * von schon gelernten Tagen zum MAX(30 Tage pro Mann) betragen. */
 
     if (magic_lowskill(student)) {
         cmistake(teacher, teacher->thisorder, 292, MSG_EVENT);
         return 0;
     }
 
-    n = STUDYDAYS * student->number;
+    students = student->number;
+    /* subtract already taught students */
     a = a_find(student->attribs, &at_learning);
     if (a != NULL) {
         teach = (teaching_info *)a->data.v;
-        n -= teach->value;
+        students -= teach->students;
     }
 
-    n = MIN(n, nteaching);
+    students = MIN(students, nteaching);
 
-    if (n != 0) {
+    if (students > 0) {
         if (teach == NULL) {
             a = a_add(&student->attribs, a_new(&at_learning));
             teach = (teaching_info *)a->data.v;
         }
         selist_push(&teach->teachers, teacher);
-        teach->value += n;
+        teach->days += students * STUDYDAYS;
+        teach->students += students; 
 
         if (student->building && teacher->building == student->building) {
             /* Solange Akademien groessenbeschraenkt sind, sollte Lehrer und
              * Student auch in unterschiedlichen Gebaeuden stehen duerfen */
+            /* FIXME comment contradicts implementation */
             if (academy_can_teach(teacher, student, sk)) {
                 /* Jeder Schueler zusaetzlich +10 Tage wenn in Uni. */
-                teach->value += (n / STUDYDAYS) * EXPERIENCEDAYS;  /* learning erhoehen */
+                teach->days += students * EXPERIENCEDAYS;  /* learning erhoehen */
-                                                /* Lehrer zusaetzlich +1 Tag pro Schueler. */
+                /* Lehrer zusaetzlich +1 Tag pro Schueler. */
-                if (academy) {
+                if (academy_students) {
-                    *academy += n;
+                    *academy_students += students;
                 }
             }
         }
-        /* Teaching ist die Anzahl Leute, denen man noch was beibringen kann. Da
-         * hier nicht n verwendet wird, werden die Leute gezaehlt und nicht die
-         * effektiv gelernten Tage. -> FALSCH ? (ENNO)
-         *
-         * Eine Einheit A von 11 Mann mit Talent 0 profitiert vom ersten Lehrer B
-         * also 10x30=300 tage, und der zweite Lehrer C lehrt fuer nur noch 1x30=30
-         * Tage (damit das Maximum von 11x30=330 nicht ueberschritten wird).
-         *
-         * Damit es aber in der Ausfuehrung nicht auf die Reihenfolge drauf ankommt,
-         * darf der zweite Lehrer C keine weiteren Einheiten D mehr lehren. Also
-         * wird student 30 Tage gutgeschrieben, aber teaching sinkt auf 0 (300-11x30 <=
-         * 0).
-         *
-         * Sonst traete dies auf:
-         *
-         * A: lernt B: lehrt A C: lehrt A D D: lernt
-         *
-         * Wenn B vor C dran ist, lehrt C nur 30 Tage an A (wie oben) und
-         * 270 Tage an D.
-         *
-         * Ist C aber vor B dran, lehrt C 300 tage an A, und 0 tage an D,
-         * und B lehrt auch 0 tage an A.
-         * (Na und? -stm)
-         *
-         * Deswegen darf C D nie lehren duerfen. (Warum? -stm)
-         *
-         * -> Das ist wirr. wer hat das entworfen?
-         * Besser waere, man macht erst vorab alle zuordnungen, und dann
-         * die Talentaenderung (enno).
-         */
-
-        /* FIXME: this code no effect; check if the refactoring done in 1e51d0e9e238e1e6e073cab2060777038e1acfa1 fucked this up */
-        nteaching = MAX(0, nteaching - student->number * STUDYDAYS);
-
     }
-    return n;
+    return students;
 }
 
 int teach_cmd(unit * teacher, struct order *ord)
@@ -286,7 +248,7 @@ int teach_cmd(unit * teacher, struct order *ord)
     plane *pl;
     region *r = teacher->region;
     skill_t sk_academy = NOSKILL;
-    int teaching, i, j, count, academy = 0;
+    int teaching, i, j, count, academy_students = 0;
 
     if (r->attribs) {
         if (get_curse(r->attribs, &ct_gbdream)) {
@@ -306,17 +268,17 @@ int teach_cmd(unit * teacher, struct order *ord)
         return 0;
     }
 
-    teaching = teacher->number * STUDYDAYS * TEACHNUMBER;
+    teaching = teacher->number  * TEACHNUMBER;
 
     if ((i = get_effect(teacher, oldpotiontype[P_FOOL])) > 0) { /* Trank "Dumpfbackenbrot" */
         i = MIN(i, teacher->number * TEACHNUMBER);
         /* Trank wirkt pro Schueler, nicht pro Lehrer */
-        teaching -= i * STUDYDAYS;
+        teaching -= i;
         change_effect(teacher, oldpotiontype[P_FOOL], -i);
-        j = teaching / STUDYDAYS;
+        j = teaching;
         ADDMSG(&teacher->faction->msgs, msg_message("teachdumb", "teacher amount", teacher, j));
     }
-    if (teaching == 0)
+    if (teaching <= 0)
         return 0;
 
     count = 0;
@@ -335,7 +297,7 @@ int teach_cmd(unit * teacher, struct order *ord)
             teachskill[t] = getskill(teacher->faction->locale);
         } while (sk != NOSKILL);
 
-        for (student = r->units; teaching && student; student = student->next) {
+        for (student = r->units; teaching > 0 && student; student = student->next) {
             if (LongHunger(student)) {
                 continue;
             }
@@ -354,7 +316,7 @@ int teach_cmd(unit * teacher, struct order *ord)
                     }
                     if (sk != NOSKILL
                         && effskill_study(teacher, sk, 0) - TEACHDIFFERENCE > effskill_study(student, sk, 0)) {
-                        teaching -= teach_unit(teacher, student, teaching, sk, true, &academy);
+                        teaching -= teach_unit(teacher, student, teaching, sk, true, &academy_students);
                     }
                 }
             }
@@ -366,7 +328,7 @@ int teach_cmd(unit * teacher, struct order *ord)
                     sk = getskill(student->faction->locale);
                     if (sk != NOSKILL
                         && effskill_study(teacher, sk, 0) - TEACHDIFFERENCE >= effskill(student, sk, 0)) {
-                        teaching -= teach_unit(teacher, student, teaching, sk, true, &academy);
+                        teaching -= teach_unit(teacher, student, teaching, sk, true, &academy_students);
                     }
                 }
             }
@@ -477,15 +439,14 @@ int teach_cmd(unit * teacher, struct order *ord)
                 }
             }
             sk_academy = sk;
-            teaching -= teach_unit(teacher, student, teaching, sk, false, &academy);
+            teaching -= teach_unit(teacher, student, teaching, sk, false, &academy_students);
         }
         new_order = create_order(K_TEACH, teacher->faction->locale, "%s", zOrder);
         replace_order(&teacher->orders, ord, new_order);
         free_order(new_order);      /* parse_order & set_order have each increased the refcount */
     }
-    if (academy && sk_academy!=NOSKILL) {
+    if (academy_students > 0 && sk_academy!=NOSKILL) {
-        /* assert(academy % STUDYDAYS == 0); bug 2355: why? */
+        academy_teaching_bonus(teacher, sk_academy, academy_students);
-        academy_teaching_bonus(teacher, sk_academy, academy);
     }
     return 0;
 }
@@ -752,12 +713,12 @@ int study_cmd(unit * u, order * ord)
 
     if (get_effect(u, oldpotiontype[P_WISE])) {
         l = MIN(u->number, get_effect(u, oldpotiontype[P_WISE]));
-        teach->value += l * EXPERIENCEDAYS;
+        teach->days += l * EXPERIENCEDAYS;
         change_effect(u, oldpotiontype[P_WISE], -l);
     }
     if (get_effect(u, oldpotiontype[P_FOOL])) {
         l = MIN(u->number, get_effect(u, oldpotiontype[P_FOOL]));
-        teach->value -= l * STUDYDAYS;
+        teach->days -= l * STUDYDAYS;
         change_effect(u, oldpotiontype[P_FOOL], -l);
     }
 
@@ -766,16 +727,16 @@ int study_cmd(unit * u, order * ord)
         /* p ist Kosten ohne Uni, studycost mit; wenn
          * p!=studycost, ist die Einheit zwangsweise
          * in einer Uni */
-        teach->value += u->number * EXPERIENCEDAYS;
+        teach->days += u->number * EXPERIENCEDAYS;
     }
 
     if (is_cursed(r->attribs, &ct_badlearn)) {
-        teach->value -= u->number * EXPERIENCEDAYS;
+        teach->days -= u->number * EXPERIENCEDAYS;
     }
 
     multi *= study_speedup(u, sk, speed_rule);
     days = study_days(u, sk);
-    days = (int)((days + teach->value) * multi);
+    days = (int)((days + teach->days) * multi);
 
     /* the artacademy currently improves the learning of entertainment
        of all units in the region, to be able to make it cumulative with

src/study.h

@@ -50,7 +50,8 @@ extern "C" {
 #define TEACHNUMBER 10
     typedef struct teaching_info {
         struct selist *teachers;
-        int value;
+        int students;
+        int days;
     } teaching_info;
 
     extern const struct attrib_type at_learning;

src/study.test.c

@@ -251,6 +251,69 @@ static void test_academy_building(CuTest *tc) {
     test_cleanup();
 }
 
+/*
+u0 (1) TEACH u3 (1) u1 (9/10)
+u (2) TEACH u1 (1/10)
+ */
+
+static void test_academy_bonus(CuTest *tc) {
+    unit *u, *u0, *u1, *u3;
+    struct locale * loc;
+    building * b;
+
+    test_setup();
+
+    random_source_inject_constant(0.0);
+    init_resources();
+    loc = test_create_locale();
+    setup_locale(loc);
+    u = test_create_unit(test_create_faction(0), test_create_region(0, 0, 0));
+    u->faction->locale = loc;
+
+    u0 = test_create_unit(test_create_faction(0), test_create_region(0, 0, 0));
+    set_level(u, SK_CROSSBOW, TEACHDIFFERENCE);
+    set_level(u0, SK_CROSSBOW, TEACHDIFFERENCE);
+    
+    u1 = test_create_unit(u->faction, u->region);
+    u3 = test_create_unit(u->faction, u->region);
+    u0->thisorder = create_order(K_TEACH, loc, "%s %s", itoa36(u3->no), itoa36(u1->no));
+    u->thisorder = create_order(K_TEACH, loc, "%s", itoa36(u1->no));
+    u1->thisorder = create_order(K_STUDY, loc, skillnames[SK_CROSSBOW]);
+    u3->thisorder = create_order(K_STUDY, loc, skillnames[SK_CROSSBOW]);
+    
+    b = test_create_building(u->region, test_create_buildingtype("academy"));
+    b->size = 25;
+    u_set_building(u, b);
+    u_set_building(u0, b);
+    u_set_building(u1, b);
+    u_set_building(u3, b);
+
+    scale_number(u, 2);
+    scale_number(u1, 9);
+    scale_number(u3, 2);
+    i_change(&u1->items, get_resourcetype(R_SILVER)->itype, 5000);
+    b->flags = BLD_MAINTAINED;
+
+    learn_inject();
+    teach_cmd(u0, u0->thisorder);
+    teach_cmd(u, u->thisorder);
+    study_cmd(u1, u1->thisorder);
+    study_cmd(u3, u3->thisorder);
+
+    CuAssertIntEquals(tc, 4, log_size);
+    CuAssertIntEquals(tc, SK_CROSSBOW, log_learners[0].sk);
+    CuAssertPtrEquals(tc, u0, log_learners[0].u);
+    CuAssertIntEquals(tc, 10, log_learners[0].days);
+    CuAssertPtrEquals(tc, u, log_learners[1].u);
+    CuAssertIntEquals(tc, 1, log_learners[1].days);
+    CuAssertPtrEquals(tc, u1, log_learners[2].u);
+    CuAssertIntEquals(tc, 720, log_learners[2].days);
+    CuAssertPtrEquals(tc, u3, log_learners[3].u);
+    CuAssertIntEquals(tc, 160, log_learners[3].days);
+    learn_reset();
+    test_cleanup();
+}
+
 void test_learn_skill_single(CuTest *tc) {
     unit *u;
     skill *sv;
@@ -563,7 +626,7 @@ static void test_teach_message(CuTest *tc) {
     CuAssertPtrNotNull(tc, a->data.v);
     teach = (teaching_info *)a->data.v;
     CuAssertPtrNotNull(tc, teach->teachers);
-    CuAssertIntEquals(tc, 600, teach->value);
+    CuAssertIntEquals(tc, 600, teach->days);
     CuAssertIntEquals(tc, 2, selist_length(teach->teachers));
     CuAssertPtrEquals(tc, u1, selist_get(teach->teachers, 0));
     CuAssertPtrEquals(tc, u2, selist_get(teach->teachers, 1));
@@ -635,6 +698,7 @@ CuSuite *get_study_suite(void)
     SUITE_ADD_TEST(suite, test_study_with_bad_teacher);
     SUITE_ADD_TEST(suite, test_produceexp);
     SUITE_ADD_TEST(suite, test_academy_building);
+    SUITE_ADD_TEST(suite, test_academy_bonus);
     SUITE_ADD_TEST(suite, test_demon_skillchanges);
     SUITE_ADD_TEST(suite, test_study_bug_2194);
     return suite;