Project

Profile

Help

HostedRedmine.com has moved to the Planio platform. All logins and passwords remained the same. All users will be able to login and use Redmine just as before. Read more...

Bug #921933

Production of obsolete buildings not really upgraded

Added by Anonymous over 1 year ago. Updated 8 months ago.

Status:
Closed
Priority:
Normal
Category:
Server
Sprint/Milestone:
Start date:
Due date:
% Done:

0%

Estimated time:

Description

The server sends messages like

Production of Barracks upgraded to Barracks II in City A
City A is building Barracks(O) which is no longer available

It does get actually switched, city_build_building just uses the old value
when checking if it's still available.

History

#1 Updated by Anonymous over 1 year ago

Fix by fetching the improvement being produced only after the upgrade.
Also prevent buying an obsolete building, it doesn't seem to make sense
anyway and could be used for a partial buy of the upgraded building.

#2 Updated by Marko Lindqvist over 1 year ago

Ilkka Virta wrote:

Fix by fetching the improvement being produced only after the upgrade.

You should then leave it uninitialized until that point. As per CodingStyle:

"- Never initialize variables with values that make no sense as their
value in case they get used. If there's no sensible initialization
value for a variable, leave it uninitialized. This allows various
tools to detect if such a variable ever gets used without assigning
proper value to it."

Also prevent buying an obsolete building, it doesn't seem to make sense
anyway and could be used for a partial buy of the upgraded building.

That's a separate issue, which should be handled separately. It even adds a translatable string while the main issue does not -> it may cause patch as a whole to get postponed.
Recently Sveinung wrote something along the lines: "When you begin a sentence with 'Also', you usually should have a separate ticket"

#3 Updated by Anonymous over 1 year ago

Marko Lindqvist wrote:

Ilkka Virta wrote:
You should then leave it uninitialized until that point.

Yep.

Also prevent buying an obsolete building, it doesn't seem to make sense
anyway and could be used for a partial buy of the upgraded building.

That's a separate issue, which should be handled separately. It even adds a translatable string while the main issue does not -> it may cause patch as a whole to get postponed.

I did also add the 'city_can_change_build()' test, which already is a constraint
that didn't exist there before. (It also wouldn't have made a difference now if the
buy was prevented, but could matter if some other factor preventing changing
was added at some point.)

In any case, I do think it is rather an integral part of the update behaviour to
decide what happens if a player tries to buy the newly-obsolete building. Some of
the options to that are at odds with the general sense that it's not possible
to buy something partially, or to buy something and then change to something
else. Having the same behaviour as for units wouldn't be very useful either, since
getting the obsolete building would be totally useless and it would just get sold the
next time the player discovers a new tech. Hence, I would suggest just preventing
the buy.

But, I don't know what you think the proper behaviour should be, or if you consider
the current behaviour a bug to begin with, so I'm not sure if there's much more I
can do about this.

#4 Updated by Marko Lindqvist over 1 year ago

Ilkka Virta wrote:

In any case, I do think it is rather an integral part of the update behaviour to
decide what happens if a player tries to buy the newly-obsolete building.

It's not integral part of fixing the erroneous message about the obsolete production. It's a separate issue.
The main fix should be able to get in fast. The piggybacking buying part prevents that.

#5 Updated by Marko Lindqvist over 1 year ago

  • Sprint/Milestone set to 2.6.5

S2_6:

git am ../../patch/0001-Fix-updating-production-of-obsolete-buildings-preven.patch
Applying: Fix updating production of obsolete buildings, prevent buying them
/fast/freeciv/master/src/.git/worktrees/src1/rebase-apply/patch:14: trailing whitespace.
if (VUT_IMPROVEMENT == pcity->production.kind
warning: 1 line adds whitespace errors.

Doesn't apply at all to master, have not tested branches in between.

#6 Updated by Marko Lindqvist over 1 year ago

Can you look at updating this before we enter another freeze (for 3.0.0-beta2) ?

#7 Updated by Marko Lindqvist about 1 year ago

  • Sprint/Milestone changed from 2.6.5 to 2.6.6

#8 Updated by Marko Lindqvist 9 months ago

Marko Lindqvist wrote:

Ilkka Virta wrote:

In any case, I do think it is rather an integral part of the update behaviour to
decide what happens if a player tries to buy the newly-obsolete building.

It's not integral part of fixing the erroneous message about the obsolete production. It's a separate issue.

-> https://osdn.net/projects/freeciv/ticket/43278

#11 Updated by Marko Lindqvist 8 months ago

  • Status changed from Resolved to Closed
  • Assignee set to Marko Lindqvist

Also available in: Atom PDF