[Freeciv-Dev] (PR#6585) Delta version 9

["Mike Kaufman" <kaufman@xxxxxxxxxxxxxxxxxxxxxx>]

<URL: http://rt.freeciv.org/Ticket/Display.html?id=6585 >

Some comments:

############################################################
DOC/HACKING: give [order of] files to which you should direct attention to
 learn about the delta patch. (eg. "Look at common/packets.def first...")

 Also there should be a step by step of what it takes to add a new packet
 even if it's just:
   1. edit common/packets.def
   2. run  common/generate_packets.py

 But we should also explain if autoconf/automake/configure/etc need to be 
 run afterward.

 Also, The HINT packets, and how they're used is not explained to my 
 satisfaction. What sends these packets? Why are they needed for compression?

#######################################################
packets.def:

 Packets:
 ========
    
  They just start and end with a line only containing "end". Header
  line is: "<struct-name>;<packet-type>;<flags>". If packet-type isn't
  given the packet type is given as parameter. Flags is a comma
  seperated list of:

// this is extremely unclear, better would be:

  A packet definition starts with and header line, contains variable
  declarations, and ends with a single line containing the word "end"
  
  PACKET_<PACKET_NAME>=<packet num>; [<packet flags>]
    <type1> <field1>; [<field flags>]
    <type2> <field2>; [<field flags>]
    ...
  end

// should have a subsection here labeled:

   Header line:
   ------------

// now give exact syntax for the header line. in none of the packets I
// see a <struct-name> so I have no idea what the hell it is or does.
// include the purpose and convention of the numbering...

// this should be a subsubsection.
   Packet flags:
   -------------

   you really need to give a skeleton example here.

     // "pre", "post" should have separate lines.

     (packet_req_join_game). Sadly this also disables the use of 0 as
     the default value.
    // no packet by that name anymore

     no-handle: don't generate handle_* prototypes
    // explain the consequences here.

     add-cap: only transfer this field if the given capability is
     available at runtime.

     remove-cap: don't transfer this field if the given capability is
     available at runtime.

     // I take these take arguments somehow. give an example.

     // nothing for lsend, dsend?

     // "sc" and "cs" not even mentioned.


  # typedefs for arrays/structs
  MEMORY should be changed to BYTE

// There should be a comment before the packets begin giving a list of the 
// packet groupings to come and the comment should also give the number of
// the last packet so I don't have to strain when adding a new one.

  
// for PACKET_AUTHENTICATION_REPLY you have: cs,handle-per-conn,no-handle
// but if there's no prototype, why do we need handle-per-conn?

// for PACKET_CITY_NAME_SUGGESTION_REQ, the comment should go above both of
// those packets


  # _PLACE_STRUCTURAL: index to sship->structure[]
  # others: new value for sship->fuel etc; should be just
  #     one more than current value of ship->fuel etc
  #     (used to avoid possible problems if we send duplicate
  #     packets when client auto-builds?)

// I don't understand...

PACKET_RULESET_TERRAIN=101;sc
  TERRAIN id;

  bitvector(bv_terrain_flags) flags;

// bitvector type is not mentioned above?

############################################################
generate_packets.py

should output (on terminal) what files are being generated when it is 
being run. (perhaps with a -v flag)

// unintelligible. need more comments.

############################################################
packets_gen.h

// missing packet numbers, wtf? where's 77, 82, 109, etc? and what's up 
// with 134?

// also, freeze_hint and thaw_hint should be packets 2 and 3 or 4 and 5
// if they're critical packets (though I'm reserving judgement.)


############################################################
packets_gen.c

// I still would appreciate function comment headers even if they're empty
// somewhere, the way you handle variants is going to have to be explained...

############################################################
srv_main.c

// there needs to be a big honking comment where you #include srv_main_gen.c
// ditto on civclient_gen.c





###########################################################
I have NO idea how on earth to actually add a packet if I need to:
take for example the new effects in packets.c:

@@ -532,6 +535,65 @@
 }

 /*************************************************************************
+ puts a packet's vector of effect definitions onto the wire.
+**************************************************************************/
+static void package_effect_defs(struct data_out *dout, 
+                                struct effect_defn_vector *vec)
+{
+  int count = effect_defn_vector_size(vec);
+
+  dio_put_uint8(dout, count);
+
+  effect_defn_vector_iterate(*vec, defn) {
+    dio_put_uint8(dout, defn->type);
+    dio_put_uint8(dout, defn->range);
+    dio_put_uint8(dout, defn->outside);
+    dio_put_sint16(dout, defn->amount);
+    dio_put_sint16(dout, defn->value1);
+    dio_put_sint16(dout, defn->value2);
+    dio_put_uint8(dout, defn->cond_bldg);
+    dio_put_uint32(dout, defn->cond_nat);
+    dio_put_uint8(dout, defn->cond_gov);
+    dio_put_uint8(dout, defn->cond_adv);
+    dio_put_uint8(dout, defn->cond_eff);
+    dio_put_uint8(dout, defn->aff_unit);
+    dio_put_uint8(dout, defn->aff_terr);
+    dio_put_uint16(dout, defn->aff_spec);
+  } effect_defn_vector_iterate_end;
+}
+
+/*************************************************************************
+ takes a vector of effect definitions off the wire and into a packet.
+**************************************************************************/
+static void unpackage_effect_defs(struct data_in *din, 
+                                  struct effect_defn_vector *vec)
+{
+  int count;
+
+  dio_get_uint8(din, &count);
+
+  effect_defn_vector_init(vec);
+  effect_defn_vector_reserve(vec, count);
+
+  effect_defn_vector_iterate(*vec, defn) {
+    dio_get_uint8(din, (int *)&defn->type);
+    dio_get_uint8(din, (int *)&defn->range);
+    dio_get_uint8(din, (int *)&defn->outside);
+    dio_get_sint16(din, (int *)&defn->amount);
+    dio_get_sint16(din, (int *)&defn->value1);
+    dio_get_sint16(din, (int *)&defn->value2);
+    dio_get_uint8(din, &defn->cond_bldg);
+    dio_get_uint32(din, &defn->cond_nat);
+    dio_get_uint8(din, &defn->cond_gov);
+    dio_get_uint8(din, &defn->cond_adv);
+    dio_get_uint8(din, (int *)&defn->cond_eff);
+    dio_get_uint8(din, (int *)&defn->aff_unit);
+    dio_get_uint8(din, (int *)&defn->aff_terr);
+    dio_get_uint16(din, (int *)&defn->aff_spec);
+  } effect_defn_vector_iterate_end;
+}
+
+/*************************************************************************

@@ -1934,6 +1996,35 @@
 /*************************************************************************
 ...
 **************************************************************************/
+int send_packet_effect(struct connection *pc,
+                       const struct packet_effect *packet)
+{
+  SEND_PACKET_START(PACKET_EFFECT);
+
+  dio_put_uint8(&dout, packet->add_effect);
+  dio_put_uint8(&dout, packet->playerno);
+  package_effect_defs(&dout, (struct effect_defn_vector*)&packet->effect_defs);
+
+  SEND_PACKET_END;
+}
+
+/*************************************************************************
+...
+**************************************************************************/
+struct packet_effect *receive_packet_effect(struct connection *pc)
+{
+  RECEIVE_PACKET_START(packet_effect, packet);
+
+  dio_get_uint8(&din, (int*)&packet->add_effect);
+  dio_get_uint8(&din, &packet->playerno);
+  unpackage_effect_defs(&din, &packet->effect_defs);
+
+  RECEIVE_PACKET_END(packet);
+}
+
+/*************************************************************************
+...
+**************************************************************************/

@@ -2115,6 +2206,9 @@
   dio_put_uint8(&dout, packet->fuel);
   DIO_BV_PUT(&dout, packet->flags);
   DIO_BV_PUT(&dout, packet->roles);
+
+  package_effect_defs(&dout, (struct effect_defn_vector*)&packet->effect_defs);
+
   dio_put_uint8(&dout, packet->happy_cost);   /* unit upkeep -- SKi */
   dio_put_uint8(&dout, packet->shield_cost);
   dio_put_uint8(&dout, packet->food_cost);
@@ -2170,6 +2264,9 @@
   dio_get_uint8(&din, &packet->fuel);
   DIO_BV_GET(&din, packet->flags);
   DIO_BV_GET(&din, packet->roles);
+
+  unpackage_effect_defs(&din, &packet->effect_defs);
+
   dio_get_uint8(&din, &packet->happy_cost);   /* unit upkeep -- SKi */
   dio_get_uint8(&din, &packet->shield_cost);
   dio_get_uint8(&din, &packet->food_cost);
@@ -2228,6 +2325,8 @@
     dio_put_string(&dout, packet->graphic_alt);
   }

+  package_effect_defs(&dout, (struct effect_defn_vector*)&packet->effect_defs);
+
   /* This must be last, so client can determine length: */
   if(packet->helptext) {
     dio_put_string(&dout, packet->helptext);

etc... how do you do this? and then removing struct impr_effect *effect; is it
simply as easy to do as:
@@ -714,8 +719,8 @@
   int build_cost;
   int upkeep;
   int sabotage;
-  struct impr_effect *effect;
-  int variant;         /* FIXME: remove when gen-impr obsoletes */
+  struct effect_defn_vector effect_defs;
+  int variant;                 /* FIXME: remove when gen-impr obsoletes */
   char *helptext;              /* same as for packet_ruleset_unit, above */
   char soundtag[MAX_LEN_NAME];
   char soundtag_alt[MAX_LEN_NAME];
@@ -832,6 +837,8 @@

-mike