patchFreeciv - Patches: patch #3698, Corrections to docs/README.delta

 
 
Show feedback again

patch #3698: Corrections to docs/README.delta

Submitted by:  Sveinung Kvilhaugsvik <sveinung>
Submitted on:  Wed Feb 13 12:24:59 2013  
 
Category: docsPriority: 5 - Normal
Status: DonePrivacy: Public
Assigned to: pepeto <pepeto>Open/Closed: Closed
Planned Release: 2.5.0Contains string changes: None

Add a New Comment (Rich MarkupRich Markup):
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Sat Feb 23 09:12:22 2013, SVN revision 22431:

Improvements to "docs/README.delta".

Patch by Sveinung Kvilhaugsvik with minor change by me

See gna patch #3698

(Browse SVN revision 22431)

pepeto <pepeto>
Project MemberIn charge of this item.
Fri Feb 22 08:51:10 2013, comment #7:

New version0: do not refer to the real number of bytes of the packet-header, just link to HACKING...

(file #17312)

pepeto <pepeto>
Project MemberIn charge of this item.
Fri Feb 15 12:52:50 2013, comment #6:

Thank you for your help, Per. I have attached a new version adding the following explanation: "The old version filled in from is the previous packet of the same kind that has the same value in each key field. (If the packet's kind don't have any key fields the previous packet of the same kind is used) If no old version exists the unchanged fields will be assumed to be zero."

To prevent misunderstandings "(If the packet's kind don't have any key fields the previous packet of the same kind is used)" was added.

> No. It takes in account both keys.
Sorry to waste your time. I forgot to remove that after I looked closer at genhash. (At least I remembered to remove num_buckets)

(file #17201)

Sveinung Kvilhaugsvik <sveinung>
Project Member
Fri Feb 15 09:16:05 2013, comment #5:

> Packets without a key will use the previous packet of the same
> kind.


Yes.

> Packets with one key will use the previous packet of the same
> kind that had the same key.


Yes.

> Packets with two keys will use the previous packet of the same
> kind that has the same value when the keys are combined taking
> the first byte of key1 followed by the rest of key1 xor'ed
> with key2.


No. It takes in account both keys. There are two functions passed to the hash table initialization :

  • hash_packet_xxx() which returns a hash value (the index of the hash table where to start to lookup). With 2 keys, as for coordinates for Freeciv <= 2.1, it is calculated as :

So, the combination of both keys with a xor'ed value issued from the first key left-shifted by 1 byte and the second key. I don't know how it is supposed to work with other types...

  • cmp_packet_xxx() which returns 0 only if both keys are matched. This is the function that grant that both keys are correctly checked.

Note that there is no packet using 2 keys currently.

> If no previous packet following those conditions exist it will
> use zero.


Yes.

pepeto <pepeto>
Project MemberIn charge of this item.
Thu Feb 14 16:24:46 2013, comment #4:

My new understanding: Packets without a key will use the previous packet of the same kind. Packets with one key will use the previous packet of the same kind that had the same key. Packets with two keys will use the previous packet of the same kind that has the same value when the keys are combined taking the first byte of key1 followed by the rest of key1 xor'ed with key2. If no previous packet following those conditions exist it will use zero. Is this correct?

Sveinung Kvilhaugsvik <sveinung>
Project Member
Thu Feb 14 14:08:54 2013, comment #3:

> Maybe further clarifications would be welcome to determine how those bytes are used.
Good idea. I added "(See HACKING to learn how to understand the packet header)" (avoids duplication) to the attached update.

> Also, I proposed a change in the initial network protocol (in bug #19943) that could make this path out of date.

I'll keep an eye on it.

> Keys are taken in account in this case (that's why they are keys).

Thank you. My ability to properly understand C after a quick look is rather limited. I would still like details about how the old version is selected to be added. It don't have to be in this patch. I don't have to be the one to write it. I'll make a new try after switching my brain to C-mode and carefully reading the generated code. Letting experimental results alter my working hypothesis from the one extreme ("All delta packets have keys to look up fields") to the other ("Ignore keys, packets of a related kind and everything else! Just use the previous packet or 'zero' if no previous packet exist") clearly isn't working.

(file #17198)

Sveinung Kvilhaugsvik <sveinung>
Project Member
Wed Feb 13 17:00:39 2013, comment #2:

> Freeciv's packet header size is 4 bytes now.


Maybe further clarifications would be welcome to determine how those bytes are used. Also, I proposed a change in the initial network protocol (in bug #19943) that could make this path out of date.

> and keys etc aren't taken into account when selecting the old
> packet


Keys are taken in account in this case (that's why they are keys).

pepeto <pepeto>
Project MemberIn charge of this item.
Wed Feb 13 14:22:48 2013, comment #1:

"The values of the unchanged fields will be filled in from an old version at the receiving side." could use a clarification. I had a look at the generated code when delta is enabled. If it is as simple as it appears, and keys etc aren't taken into account when selecting the old packet, I suggest changing it to: "The values of the unchanged fields will be filled in from the previous packet of the same kind received at the receiving side."

Sveinung Kvilhaugsvik <sveinung>
Project Member
Wed Feb 13 12:24:59 2013, original submission:

Freeciv's packet header size is 4 bytes now. Remove a "the the" at the same time.

Sveinung Kvilhaugsvik <sveinung>
Project Member

 

(Note: upload size limit is set to 1024 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files
file #17312:  README.delta.diff added by pepeto (1kB - text/x-patch)
file #17201:  README.delta.patch added by sveinung (1kB - text/x-diff - version 3)
file #17198:  README.delta_corrections.patch added by sveinung (926B - text/x-diff - version 2)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by pepeto (Posted a comment)
  • -unavailable- added by sveinung (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Error: not logged in

     

     

    Follow 11 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Sat Feb 23 09:12:48 2013pepetoStatusReady For Test=>Done
      Open/ClosedOpen=>Closed
    Fri Feb 22 08:51:10 2013pepetoAttached File-=>Added README.delta.diff, #17312
      StatusIn Progress=>Ready For Test
      Planned Release=>2.5.0
    Mon Feb 18 10:18:49 2013pepetoStatusNone=>In Progress
      Assigned toNone=>pepeto
    Fri Feb 15 12:52:50 2013sveinungAttached File-=>Added README.delta.patch, #17201
    Fri Feb 15 09:16:05 2013pepetoCategoryNone=>docs
    Thu Feb 14 14:08:54 2013sveinungAttached File-=>Added README.delta_corrections.patch, #17198
    Wed Feb 13 12:24:59 2013sveinungAttached File-=>Added README.delta_corrections.patch, #17192
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup