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 13 Feb 2013 12:24:59 PM UTC  
 
Category: docsPriority: 5 - Normal
Status: DonePrivacy: Public
Assigned to: pepeto <pepeto>Open/Closed: Closed
Planned Release: 2.5.0

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 23 Feb 2013 09:12:22 AM UTC, 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 22 Feb 2013 08:51:10 AM UTC, 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 15 Feb 2013 12:52:50 PM UTC, 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 15 Feb 2013 09:16:05 AM UTC, 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 14 Feb 2013 04:24:46 PM UTC, 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 14 Feb 2013 02:08:54 PM UTC, 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 13 Feb 2013 05:00:39 PM UTC, 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 13 Feb 2013 02:22:48 PM UTC, 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 13 Feb 2013 12:24:59 PM UTC, 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.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 11 latest changes.

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

    Back to the top


    Powered by Savane 3.1-cleanup