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...

Feature #921525

New improvement genus for coinage-like improvements

Added by Alina L. over 1 year ago. Updated over 1 year ago.

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

0%

Estimated time:

Description

Idea: Put the "can never be finished / whole production output used up every turn" functionality, which is currently attached to the "Gold" improvement flag, into a new improvement genus "Convert". The "Gold" flag is still responsible for the "...and give that amount of gold" part.

Reasoning:
  • Paves the way for allowing other ways to convert production in the future: into other things (e.g. science), or at different rates (think unlocking more efficient coinage improvements later in the game)
  • Splits two conceptually different things into separate ruleset representations
  • Helps break up the nebulously defined "Special" improvement genus

History

#1 Updated by Alina L. over 1 year ago

While making the changes to support the new genus, I noticed some duplicate code in the sdl2 client. This is probably best done separately, by someone more familiar with the sdl2 client (or the clients in general) than me, so I raised Bug #921528.

#2 Updated by Marko Lindqvist over 1 year ago

Prefer raising new tickets to osdn ticketing system ( https://osdn.net/projects/freeciv/ticket/ ), unless the new ticket is linked to others already existing in hrm in a way that makes it still more sensible to handle it in hrm.

New development should go to 3.2 (master) only by default. For targeting to S3_1 you need to have good reasoning why it should still be included there.

#3 Updated by Alina L. over 1 year ago

Marko Lindqvist wrote:

Prefer raising new tickets to osdn ticketing system ( https://osdn.net/projects/freeciv/ticket/ ), unless the new ticket is linked to others already existing in hrm in a way that makes it still more sensible to handle it in hrm.

I'll keep that in mind – wasn't sure if that was still a test run or whatever and figured I'd stick with what I knew worked.

Marko Lindqvist wrote:

New development should go to 3.2 (master) only by default. For targeting to S3_1 you need to have good reasoning why it should still be included there.

Main reason was that I'm working (custom ruleset-wise) with S3_1 atm, so that's where I started doing stuff. As far as I can tell, the changes required for S3_1 and master are pretty much identical (apart from experimental being renamed goldkeep, which git figures out on its own), so there's a decent chance this would turn into a single patch compatible with both branches.
I'd argue S3_1 d3f is still a ways off, and some of the blocking issues for that aren't really that much less "new development" than this (except for having been first raised a few years earlier), so I don't see that much of a reason not to include it there – post-branching or not. In the end I guess it's your decision though, not like I can approve and merge the patch myself :P

~AVL

#4 Updated by Marko Lindqvist over 1 year ago

Alina L. wrote:

there's a decent chance this would turn into a single patch compatible with both branches.

What I expect to be hardest part of this patch is the ruleset update (rscompat) code changes, and those will only be in the first branch where the feature is implemented (we don't support updating rulesets over two versions)

#5 Updated by Alina L. over 1 year ago

Marko Lindqvist wrote:

What I expect to be hardest part of this patch is the ruleset update (rscompat) code changes, and those will only be in the first branch where the feature is implemented (we don't support updating rulesets over two versions)

Ooh, right, that's a good point. Funny how one can tell oneself "I'll just finish this and then look at how this rscompat stuff works" and yet forget that exists a second later.
Yeah, that breaks the one-size-fits-all approach. Only master it is then.

#6 Updated by Marko Lindqvist over 1 year ago

Alina L. wrote:

I'd argue S3_1 d3f is still a ways off,

Usually I'd argue that pushing in new stuff, which we then need to wait to stabilize, is the reason d3f gets pushed so far from the branching point, but in case of S3_1 I have to admit that we have quite big issues to solve before d3f, so arguably there is still "safe" space for smaller features at the moment. Besides, we're not going to skip 3.0, so there's not yet hurry to get S3_1 to release shape.

#7 Updated by Alina L. over 1 year ago

Here's a preliminary patch with everything (as far as I can tell) except ruleset compatibility code. If there's anything you'd like me to change, let me know.
Notes:
  • The only new thing this directly allows for rulesets is adding an improvement to convert production into nothing; that option needed to be addressed explicitly in a few places regarding how city production is displayed.
  • For the purposes of is_special_improvement(), I'm still including Convert improvements – as far as grep can tell, that method is only ever used to check whether a city can have that building, or in a context where coinage was included anyway. This is still something I figure we'll need to clean up (along with the nebulosity of the Special genus as a whole), dependent on finding good terminology for it; I left a TODO comment in the code as well.

Regarding rscompat:
The change that needs to be made is simple – changing the genus of any IF_GOLD improvements from IG_SPECIAL to IG_CONVERT (and maybe setting its build cost to 999?); AFAICT, anything with a BuildingGenus requirement shouldn't be able to affect coinage in the first place.
The difficult thing is where that change happens – there doesn't currently seem to be any dedicated rscompat call between initially loading building data, and the sanity check that already requires the change. Way I see it, this pretty much gives us the options of (a) mixing compatibility code into load_ruleset_buildings(), (b) mixing more compatibility code (in addition to ignore_retired) into the sanity checks, or (c) introducing a new, pre-rssanity compat step. None of these options really jumps out as "the ideal one" tho :/
Thoughts?

~AVL

#8 Updated by Marko Lindqvist over 1 year ago

Alina L. wrote:

The difficult thing is where that change happens – there doesn't currently seem to be any dedicated rscompat call between initially loading building data, and the sanity check that already requires the change. Way I see it, this pretty much gives us the options of (a) mixing compatibility code into load_ruleset_buildings(), (b) mixing more compatibility code (in addition to ignore_retired) into the sanity checks, or (c) introducing a new, pre-rssanity compat step. None of these options really jumps out as "the ideal one" tho :/
Thoughts?

I haven't checked the code, so this is just an educated guess based on earlier work with rscompat:
Basically (a) with function 'enum impr_genus_id rscompat_genus_3_2(struct rscompat_info *compat, ???, enum impr_genus_id old_genus)' defined in rscompat.[ch] module. So load_ruleset_building() would only call that compatibility function when determining the genus.

This soon after S3_1 has been branched there is not much rscompat code in master. You should look S3_1 / S3_0 for better examples.

#9 Updated by Alina L. over 1 year ago

Added rscompat code. Test with a S3_1 ruleset I had lying around was successful. At this point, unless there's some stylistic issues or something I've missed, I'd say the patch is done.

Thanks for all the advice! :D

#10 Updated by Marko Lindqvist over 1 year ago

Generally looks very good. I found just two or three minor things:

- Style: In changed citydlg_common.c code there's no empty line between declaring variables (int gold;) and the code line (fc_snprintf(...)). It's not a new issue with this patch, but should be fixed while touching the code
- In rscompat_genus_3_2() you should also check from compat->ver_buildings that the ruleset loaded is an old one to update, and not already a 3.2 format one.

The one I'm still undecided if it should be changed or not; In several places you use full blown fc_snprintf() while just wanting to copy constant string.

#11 Updated by Marko Lindqvist over 1 year ago

Note to one pushing this in: network capstring update needs to be added to the commit. It's ok to leave it out from the patch in hrm as it would often cause conflict as capstr changes constantly.

#12 Updated by Alina L. over 1 year ago

Fixed.

Marko Lindqvist wrote:

The one I'm still undecided if it should be changed or not; In several places you use full blown fc_snprintf() while just wanting to copy constant string.

That was mostly imitating the surrounding code – I decided to replace them with fc_strlcpy() (and fc_strlcat()) though, since the only reasons I could think of to keep them (uniformity, and potentially saving a negligible amount of effort switching back in the future) aren't really good arguments.

#13 Updated by Marko Lindqvist over 1 year ago

  • Status changed from In Progress to Resolved
  • Assignee changed from Alina L. to Marko Lindqvist
  • Sprint/Milestone changed from 3.1.0 to 3.2.0

#14 Updated by Marko Lindqvist over 1 year ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF