[Freeciv-Dev] [bug #15422] Illness popup in client is inaccurate -- doesn't show risk from trade

[Matthias Pfafferodt]

Update of bug #15422 (project freeciv):

  Status:  Ready For Test => Fixed  
 Open/Closed:Open => Closed 


___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #15422] Illness popup in client is inaccurate -- doesn't show risk from trade

[Matthias Pfafferodt]

Follow-up Comment #16, bug #15422 (project freeciv):

> Again some comments: 

updated patch for S2_2

(file #8275)
___

Additional Item Attachment:

File name: 20100225-S2_2-fix-illness-due-to-trade-S2_2.patch Size:14 KB


___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #15422] Illness popup in client is inaccurate -- doesn't show risk from trade

[pepeto]

Follow-up Comment #15, bug #15422 (project freeciv):

Again some comments:
* We should avoid the changes of translatable strings in a stable branch,
especially when just modifying the format.
* in packhand.c there should the test to know what is the real value to read
from the packet, not assuming the server sent trade_illness.


___

Reply to this item at:

  

___
  Message posté via/par Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #15422] Illness popup in client is inaccurate -- doesn't show risk from trade

[Matthias Pfafferodt]

Follow-up Comment #14, bug #15422 (project freeciv):

> I am not sure to understand but if the value reflects the same
> behaviour that the old one, it doesn't require a capability
> change.

Both variables contain different values; I used the definitions of 'add-cap'
and 'remove-cap'.

=== patch for S2_2 ===

fix illness due to trade (S2_2)

* send illness_trade (illness due to trade) to the clients; this replaces the
variable illness, which is not needed as it will be recalculated each time by
the clients
* activate this function only if the (optional) capability trade_illness is
present in S2_2
* rename some functions
* recalculate illness after loading a savegame or trade route change

Needs an update for the network capabilities!

(file #8204)
___

Additional Item Attachment:

File name: version5-20100221-S2_2-fix-illness-due-to-trade-S2_2.patch Size:14
KB


___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #15422] Illness popup in client is inaccurate -- doesn't show risk from trade

[pepeto]

Follow-up Comment #13, bug #15422 (project freeciv):

I don't see what is different, but there is still a big problem with this
capability stuff.


___

Reply to this item at:

  

___
  Message posté via/par Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #15422] Illness popup in client is inaccurate -- doesn't show risk from trade

[Matthias Pfafferodt]

Update of bug #15422 (project freeciv):

  Depends on: => bugs #15373


___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #15422] Illness popup in client is inaccurate -- doesn't show risk from trade

[Matthias Pfafferodt]

Additional Item Attachment, bug #15422 (project freeciv):

File name: version4-20100221-S2_2-fix-illness-due-to-trade-S2_2.patch Size:15
KB
File name: version4-20100221-trunk-fix-illness-due-to-trade.patch Size:14 KB


___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #15422] Illness popup in client is inaccurate -- doesn't show risk from trade

[pepeto]

Follow-up Comment #12, bug #15422 (project freeciv):

>> I dunno what capability are you talking about... As you said,
>> you just renamed a field of the packet, so it's not
>> a 'change', because neither the size, neither the order of the
>> fields has been changed.
>
>
> Yes there are no changes in the structure of the network
> packages (only renaming an integer) but somehow I have to decide
> if it is an old version, i.e the value of 'illness' is send over
> the network, or a new version which will only send the value of
> 'illness_trade'. Are there over capabilities than network
> capabilities?

I am not sure to understand but if the value reflects the same behaviour that
the old one, it doesn't require a capability change. The name of the fields
are not sent through the network. However, if this value does not have the
same meaning of the old one, there should be:

UINT16 illness; remove-cap(illness_trade)
UINT16 illness_trade; add-cap(illness_trade)

and both fields should be set when preparing the packet for sending.


___

Reply to this item at:

  

___
  Message posté via/par Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #15422] Illness popup in client is inaccurate -- doesn't show risk from trade

[Matthias Pfafferodt]

Follow-up Comment #11, bug #15422 (project freeciv):

> I dunno what capability are you talking about... As you said,
> you just renamed a field of the packet, so it's not
> a 'change', because neither the size, neither the order of the
> fields has been changed.

Yes there are no changes in the structure of the network packages (only
renaming an integer) but somehow I have to decide if it is an old version,
i.e the value of 'illness' is send over the network, or a new version which
will only send the value of 'illness_trade'. Are there over capabilities than
network capabilities?

> You can add a send_city_info(city_owner(pcity), pcity) after
> it is recalculated. 

I check the code and found the function establish_trade_route() there the
recalculation can be done. Furthermore, the illness is recalculated after
loading a savegame.

___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #15422] Illness popup in client is inaccurate -- doesn't show risk from trade

[pepeto]

Follow-up Comment #10, bug #15422 (project freeciv):

You just renamed a field, so it is useless to change the capabilities.  It
would be useful to add a field like:

UINT16 illness_trade; add-cap(illness_trade)


Anyway, it doesn't require such changes, it is useless. In a other hand,
because there are currently many fields going to be removed from that packet,
maybe all changes should be merge into the same optional capability.


___

Reply to this item at:

  

___
  Message posté via/par Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #15422] Illness popup in client is inaccurate -- doesn't show risk from trade

[Matthias Pfafferodt]

Follow-up Comment #9, bug #15422 (project freeciv):

I did forgot to commit some important changes into git (capability changes);
version 3 attached

(file #8199)
___

Additional Item Attachment:

File name: version3-20100221-S2_2-fix-illness-due-to-trade-S2_2.patch Size:14
KB


___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #15422] Illness popup in client is inaccurate -- doesn't show risk from trade

[pepeto]

Follow-up Comment #8, bug #15422 (project freeciv):

> if server and client have the 'illness_trade' capability,
> illness due to trade will be displayed.

I dunno what capability are you talking about... As you said, you just
renamed a field of the packet, so it's not a 'change', because neither the
size, neither the order of the fields has been changed.


___

Reply to this item at:

  

___
  Message posté via/par Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #15422] Illness popup in client is inaccurate -- doesn't show risk from trade

[pepeto]

Follow-up Comment #7, bug #15422 (project freeciv):

> One small problem remains: the illness due to trade is only
> updated at the start of the turn ...

You can add a send_city_info(city_owner(pcity), pcity) after it is
recalculated.


___

Reply to this item at:

  

___
  Message posté via/par Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #15422] Illness popup in client is inaccurate -- doesn't show risk from trade

[Matthias Pfafferodt]

Follow-up Comment #6, bug #15422 (project freeciv):

> Does the packet city_short_info really need that information?
> This packet is used to send cities you cannot see internals. 

This discussions let me think again about my patches; here I found a solution
which works without adding additional network changes. The illness due to
trade is calculated in the server and send to the clients instead of the
illness value which can be calculated in the clients (moving workers changes
the pollution and thus changes the illness due to pollution). One small
problem remains: the illness due to trade is only updated at the start of the
turn ...

I will attach a patch for trunk and a patch for S2_2 (2.2.1).

> Actually, I forgot to mention that it is possible to perform
> such changes in a stable branch. You must then to add in the
> capability string (in version.in) a non-mandatory (named
> optional in that file), which doesn't begins with a '+'. Then
> after, you need in packets.def to add 'add-cap(my_new_cap)'
> after the field you want to add, and 'remove-cap(my_new_cap)'
> after the field you want to remove. Remember that the original
> configuration must leave intact, so the changes would affect
> only new clients and new servers, whereas old packets would be
> sent if at least one of them is old. 

I hope that I did this right; if server and client have the 'illness_trade'
capability, illness due to trade will be displayed. The network protocol is
not changed as there is only a renaming of a variable (illness =>
illness_trade)


(file #8196, file #8197)
___

Additional Item Attachment:

File name: version2-20100221-S2_2-fix-illness-due-to-trade-S2_2.patch Size:12
KB
File name: version2-20100221-trunk-fix-illness-due-to-trade.patch Size:12 KB


___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #15422] Illness popup in client is inaccurate -- doesn't show risk from trade

[pepeto]

Follow-up Comment #5, bug #15422 (project freeciv):

=== S2_2 patch ===
> * no changes to the network definitions

Actually, I forgot to mention that it is possible to perform such changes in
a stable branch. You must then to add in the capability string (in
version.in) a non-mandatory (named optional in that file), which doesn't
begins with a '+'. Then after, you need in packets.def to add
'add-cap(my_new_cap)' after the field you want to add, and
'remove-cap(my_new_cap)' after the field you want to remove. Remember that
the original configuration must leave intact, so the changes would affect
only new clients and new servers, whereas old packets would be sent if at
least one of them is old.

=== trunk patch ===
Does the packet city_short_info really need that information? This packet is
used to send cities you cannot see internals.


___

Reply to this item at:

  

___
  Message posté via/par Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #15422] Illness popup in client is inaccurate -- doesn't show risk from trade

[pepeto]

Follow-up Comment #4, bug #15422 (project freeciv):

> * changed network definitions

This means that you need to modify the capability string defined in
version.in.


___

Reply to this item at:

  

___
  Message posté via/par Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #15422] Illness popup in client is inaccurate -- doesn't show risk from trade

[Matthias Pfafferodt]

Update of bug #15422 (project freeciv):

  Status:None => Ready For Test 
 Planned Release: => 2.2.1  

___

Follow-up Comment #3:

=== S2_2 patch ===

deactivate illness due to trade (S2_2)

* no changes to the network definitions

=== trunk patch ===

fix illness due to trade

* send turn_plague to the clients
* do not send illness as it will be recalculated each time by the clients
* rename some functions
* changed network definitions

(file #8181, file #8182)
___

Additional Item Attachment:

File name: 20100221-S2_2-deactivate-illness-due-to-trade-S2_2.patch Size:2 KB
File name: 20100221-trunk-fix-illness-due-to-trade.patch Size:12 KB


___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #15422] Illness popup in client is inaccurate -- doesn't show risk from trade

[Matthias Pfafferodt]

Update of bug #15422 (project freeciv):

 Assigned to:None => syntron

___

Follow-up Comment #2:

Thanks for looking into the illness configuration! I think the correct patch
for S2_2 (2.2.1) will be the deactivation of illness due to trade. I will
create this patch and a patch for trunk moving the calculation into the
server, save it within the city struct and send it to the clients.

___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #15422] Illness popup in client is inaccurate -- doesn't show risk from trade

[Jacob Nevins]

Follow-up Comment #1, bug #15422 (project freeciv):

The other fix would be to calculate the breakdown on the server and send it
to the client -- I think this is what happens with happiness? Recalculating
it on the client feels fragile, and if we've got to change the packet format
anyway we may as well do it this way. (Plus it avoids the need to grow
CITY_SHORT_INFO, I think.)

___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #15422] Illness popup in client is inaccurate -- doesn't show risk from trade

[Jacob Nevins]

URL:
  

 Summary: Illness popup in client is inaccurate -- doesn't
show risk from trade
 Project: Freeciv
Submitted by: jtn
Submitted on: Friday 19/02/10 at 00:03
Category: rulesets
Severity: 3 - Normal
Priority: 5 - Normal
  Status: None
 Assigned to: None
Originator Email: 
 Open/Closed: Open
 Release: 2.2.0-RC1
 Discussion Lock: Any
Operating System: None
 Planned Release: 

___

Details:

In the Gtk client, I'm getting a different number in "Plague Risk" in the
city dialog to the total shown in the popup when I click on that risk.

For instance I have a city where the dialog claims a risk of 10.2, but the
popup says:


+9.0 : Risk from over crowdness
+0.0 : Risk from trade
+0.1 : Risk from pollution
 : Adds up to
 9.1 : Total chance for a plague


The difference between the two is that the city dialog,
gui-gtk-2.0/citydlg.c:city_dialog_update_information(), used the cached value
in the city structure (which comes from the server, I think?), whereas
get_city_dialog_illness_text(), which is used for the popup, works it out
from scratch (calling city_illness()) on the client.

In my setup these are on the same machine built from the same codebase. (The
patch for bug #15373 is applied locally, but I don't think that's
significant, see analysis below.)

I think the problem is that the calculation in get_trade_illness() depends on
"turn_plague", which is not included in
PACKET_CITY_INFO/PACKET_CITY_SHORT_INFO, so the client always sees it as -1
(never); thus, I don't think the displayed "Risk from trade" can ever be
non-zero, even if the real contribution is.

I tested this by hacking the server to always treat trade risk as zero but
keeping the client unmodified; the two numbers then agreed (and were the same
as the previous popup calculation). Also, the trade route endpoints of the
city in question do have a turn_plague that will cause a penalty.

The obvious fix is to add turn_plague to
PACKET_CITY_INFO/PACKET_CITY_SHORT_INFO, but presuambly that needs doing
carefully if it's possible at all. Perhaps it can be done with capabilities?
I haven't yet investigated what's possible here.

If it can be added in a backwards compatible way, then new clients can cope
with old servers by either refusing to display a popup at all or by
displaying incomplete information in the popup (no total).




___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev