Incidencia #44345

Load counter from ruleset

Abrir Fecha: 2022-04-13 12:06 Última actualización: 2022-07-17 17:23

Informador:
Propietario:
Tipo:
Estado:
Cerrado
Componente:
Hito:
Prioridad:
5 - Medium
Gravedad:
5 - Medium
Resolución:
Fixed
Fichero:
17

Details

Originally: #41121

Replace internal City Owned counter definition by loading zero or one counter definitions from the ruleset. There's no need to send the info to client yet.

Ticket History (3/46 Histories)

2022-04-13 12:06 Updated by: cazfi
  • New Ticket "Load counter from ruleset" created
2022-04-13 22:01 Updated by: lachu
Comentario

I read patch and I do not see if ruleset loader routine check there is too many counters in ruleset (array have fixed length). I will look inside on my free time and try to correct this.

OK: I opened up my IDE and there is check on line 1416.

(Edited, 2022-04-13 22:04 Updated by: lachu)
2022-04-16 23:30 Updated by: lachu
Comentario

I check coding-style. Few correction/fixes only. All good for now.

2022-04-17 18:56 Updated by: lachu
Comentario

Apply "Adds missing changes" and next "Patch only for testing purposes"

2022-04-17 23:09 Updated by: lachu
Comentario

Apply: Code of patch was adjusted to its responsibles I think, I will end this patch (at worst case) at end of next week. Sorry. And of course, responsibility instead of responsibles.

2022-04-23 18:39 Updated by: lachu
Comentario

@cazi: Can you look?

2022-04-25 04:37 Updated by: cazfi
Comentario

This looks mostly good already. Couple of things I noticed on quick reading:

- int num = nval; /* No "size_t" to printf */ -> Please use the SIZE_T_PRINTF macro instead of tricks like this (which may fail itself on some environments as size_t > int)
- In an error situation you section_list_destroy() twice - once when you detect the error, but still also in the end of handling
- Don't name variable outside macros, or similar constructs, from using namespace '*_'. You have 'curr_' named like that for no reason (and potentially conflicting with macro calls we later add there)
- Matter of taste, but I wouldn't have default value (of 1) for checkpoint, but to make it mandatory field instead

---

What you need to add:
- Saving the rules in tools/ruleutil/rulesave.c
- Documentation in rulesets (but we probably want to agree on one copy first, before having it copied to all supplied rulesets

2022-05-16 23:51 Updated by: lachu
Comentario

Reply To cazfi

This looks mostly good already. Couple of things I noticed on quick reading: - int num = nval; /* No "size_t" to printf */ -> Please use the SIZE_T_PRINTF macro instead of tricks like this (which may fail itself on some environments as size_t > int)
- In an error situation you section_list_destroy() twice - once when you detect the error, but still also in the end of handling
- Don't name variable outside macros, or similar constructs, from using namespace '*_'. You have 'curr_' named like that for no reason (and potentially conflicting with macro calls we later add there)
- Matter of taste, but I wouldn't have default value (of 1) for checkpoint, but to make it mandatory field instead
--- What you need to add:
- Saving the rules in tools/ruleutil/rulesave.c
- Documentation in rulesets (but we probably want to agree on one copy first, before having it copied to all supplied rulesets

Please ignore:

File 0002-Implement-basic-counters-functionality-to-ruledit.patch (File ID: 9273) is attached

About (please ignore for this ticket, but read my comment).

File 0001-Repair-BUG-with-specenum-counting.patch (File ID: 9272) is attached

It repairs intended change to counter_behaviour specenum, which makes default value not be treated by any part of freeciv (default int/enum is zero and by skipping value0 we solve many thinks). But... In future, I set MAX_COUNTERS to COUNTER_BEHAVIOUR_LAST, so it introducing bugs. I must review my changes. Sorry I miss this :-( . It also introduces remove_counter routine. If you wish, I will split this patch, but you must reopen related ticket. Any other way to solve this?

About:

0003-OSDN-44345-S-awomir-Lach-slawek-lach.art.pl.patch(1KB)
Add routines to save city counters

It is change related to this ticket. I untested it, but implementation is very simple and I do not see any possible problems.

2022-05-25 01:02 Updated by: lachu
Comentario

Reply To cazfi

This looks mostly good already. Couple of things I noticed on quick reading: - int num = nval; /* No "size_t" to printf */ -> Please use the SIZE_T_PRINTF macro instead of tricks like this (which may fail itself on some environments as size_t > int)
- In an error situation you section_list_destroy() twice - once when you detect the error, but still also in the end of handling
- Don't name variable outside macros, or similar constructs, from using namespace '*_'. You have 'curr_' named like that for no reason (and potentially conflicting with macro calls we later add there)
- Matter of taste, but I wouldn't have default value (of 1) for checkpoint, but to make it mandatory field instead
--- What you need to add:
- Saving the rules in tools/ruleutil/rulesave.c
- Documentation in rulesets (but we probably want to agree on one copy first, before having it copied to all supplied rulesets

See above post. I added also documentation.

2022-06-01 12:05 Updated by: cazfi
Comentario

Sorry I've had no time to look at this.

The ruledit conters support will definitely be needed. Good point. I don't think we have a ticket for that yet. Please open one, and keep things related to that one there.

In general there's too much stuff unrelated to the ticket at hand. Please try to keep things organized. It would really save me time if I wouldn't need to figure out what's relevant and what is not etc.

Can you create a single patch that has the content that is supposed to get pushed in in this ticket?

For the ruleset documentation comment, please follow the format that other similar section type documentation comments use.

2022-06-01 22:48 Updated by: lachu
2022-06-01 22:49 Updated by: lachu
Comentario

Reply To cazfi

Sorry I've had no time to look at this. The ruledit conters support will definitely be needed. Good point. I don't think we have a ticket for that yet. Please open one, and keep things related to that one there. In general there's too much stuff unrelated to the ticket at hand. Please try to keep things organized. It would really save me time if I wouldn't need to figure out what's relevant and what is not etc. Can you create a single patch that has the content that is supposed to get pushed in in this ticket? For the ruleset documentation comment, please follow the format that other similar section type documentation comments use.

Partially done.

2022-06-01 22:51 Updated by: lachu
  • File 0001-OSDN-44345-S-awomir-Lach-slawek-lach.art.pl.patch (File ID: 9389) is deleted
2022-06-04 00:05 Updated by: lachu
  • File 0001-OSDN-44345-S-awomir-Lach-slawek-lach.art.pl.patch (File ID: 9398) is attached
2022-06-04 00:06 Updated by: lachu
Comentario

Reply To cazfi

Sorry I've had no time to look at this. The ruledit conters support will definitely be needed. Good point. I don't think we have a ticket for that yet. Please open one, and keep things related to that one there. In general there's too much stuff unrelated to the ticket at hand. Please try to keep things organized. It would really save me time if I wouldn't need to figure out what's relevant and what is not etc. Can you create a single patch that has the content that is supposed to get pushed in in this ticket? For the ruleset documentation comment, please follow the format that other similar section type documentation comments use.

0001-OSDN-44345-S-awomir-Lach-slawek-lach.art.pl.patch(7KB)
Apply only this patch. It do not contains documentation and ruleset parts

0001-OSDN-44345-S-awomir-Lach-slawek-lach.art.pl.patch(7KB)

Documentation correction

Two above are prepared to apply - apply only these two to fulfilled ticket requirement.

2022-06-10 10:56 Updated by: cazfi
Comentario

Reply To lachu

0001-OSDN-44345-S-awomir-Lach-slawek-lach.art.pl.patch(7KB) Apply only this patch. It do not contains documentation and ruleset parts 0001-OSDN-44345-S-awomir-Lach-slawek-lach.art.pl.patch(7KB) Documentation correction

These seem like you've uploaded the same patch twice - there's no documentation patch.

- As you remove initialization from countersMAX_COUNTERS, you should declare it just "static struct counter countersMAX_COUNTERS;" - remove "{}" too
- From the rulesave it seems that you're placing these counter sections between "trade" (specifically: trade.goods_selection") and "goods" sections. Better to not split those two apart, but maybe place this (as a new feature) after "clauses"

2022-06-10 23:18 Updated by: lachu
  • File 0001-OSDN-44345-S-awomir-Lach-slawek-lach.art.pl.patch (File ID: 9398) is deleted
2022-06-10 23:39 Updated by: lachu
Comentario

Reply To cazfi

Reply To lachu

Apply these patches:

2022-06-10 23:19 Updated by: lachu

File 0002-OSDN-44345-S-awomir-Lach-slawek-lach.art.pl.patch (File ID: 9446) is attached

2022-06-10 23:32 Updated by: lachu

File 0001-OSDN-44345-S-awomir-Lach-slawek-lach.art.pl.patch (File ID: 9447) is attached

And:

0002-OSDN-41121-S-awomir-Lach-slawek-lach.art.pl.patch(6KB)
Firsr four hints ( 2022-04-25 04:37 Updated by: cazfi )implemented

I made style changes to global array of each counter class definition, but it is probably not related to this ticket. Put patch in this topic?

2022-06-17 21:26 Updated by: cazfi
Comentario

Can you make this just two patches; one for all the code changes and one for the documentation proposal? It seems that one is again expected to apply number of these patches (and it's not clear which ones and in which order) to get the correct "current" state of you work. I'm sure it would be possible to decipher from the comments and the patches themselves, but I'd prefer not to spend too much of my time on it (I have limited time for dozens of patches in my queue & all the other tasks I have)

2022-06-18 03:21 Updated by: lachu
Comentario

Apply All the related code patch (Loading + Saving Ruleset patch).

EDIT: Tested after solve problem with server/client version mismatch. Working.

(Edited, 2022-06-18 18:59 Updated by: lachu)
2022-06-25 16:47 Updated by: cazfi
Comentario

Looks good. Just two minor style issues

- Prefer i++ instead of ++i when either works. That needs fixing in a couple of "for (... ; ... ; ++xxx)"
- Couple of "save_default_int(... path,"xxx")" lack the space after comma

While we ideally would want to keep the ruleset documentation in sync with the code, this is getting too slow. Maybe we should split this ticket to the code change (almost ready) and to the documentation update to be handled after this?

2022-06-27 02:36 Updated by: None
Comentario

Reply To cazfi

Looks good. Just two minor style issues
- Prefer i++ instead of ++i when either works. That needs fixing in a couple of "for (... ; ... ; ++xxx)"
- Couple of "save_default_int(... path,"xxx")" lack the space after comma
While we ideally would want to keep the ruleset documentation in sync with the code, this is getting too slow. Maybe we should split this ticket to the code change (almost ready) and to the documentation update to be handled after this?

I think better approach is move to another target milestone/version. I have also other task and do not known how fast could I deliver changes :-( .

2022-06-27 02:37 Updated by: lachu
Comentario

Reply To (Anonymous)

Reply To cazfi

Looks good. Just two minor style issues
- Prefer i++ instead of ++i when either works. That needs fixing in a couple of "for (... ; ... ; ++xxx)"
- Couple of "save_default_int(... path,"xxx")" lack the space after comma
While we ideally would want to keep the ruleset documentation in sync with the code, this is getting too slow. Maybe we should split this ticket to the code change (almost ready) and to the documentation update to be handled after this?

I think better approach is move to another target milestone/version. I have also other task and do not known how fast could I deliver changes :-( .

This comment was mine. I have foreign language's trouble, so writing documentation is not my strong side.

2022-06-27 20:56 Updated by: lachu
Comentario

Reply To cazfi

Looks good. Just two minor style issues
- Prefer i++ instead of ++i when either works. That needs fixing in a couple of "for (... ; ... ; ++xxx)"
- Couple of "save_default_int(... path,"xxx")" lack the space after comma
While we ideally would want to keep the ruleset documentation in sync with the code, this is getting too slow. Maybe we should split this ticket to the code change (almost ready) and to the documentation update to be handled after this?

0002-OSDN-41121-S-awomir-Lach-slawek-lach.art.pl-Add-rout.patch(7KB)
Style fixes

Just apply this and the doc.

2022-07-06 08:49 Updated by: cazfi
Comentario

Reply To lachu

Reply To cazfi

Looks good. Just two minor style issues
- Prefer i++ instead of ++i when either works. That needs fixing in a couple of "for (... ; ... ; ++xxx)"

There seems to be change to the wrong direction in attach_city_counter()! load_ruleset_game() has one instance of '++curr' that is not fixed.

Spit the documentation update -> #45028

2022-07-06 23:21 Updated by: lachu
Comentario

Reply To cazfi

Reply To lachu

Reply To cazfi

Looks good. Just two minor style issues
- Prefer i++ instead of ++i when either works. That needs fixing in a couple of "for (... ; ... ; ++xxx)"

There seems to be change to the wrong direction in attach_city_counter()! load_ruleset_game() has one instance of '++curr' that is not fixed. Spit the documentation update -> #45028

Patch send and it has name "In response to: 2022-07-06 08:49 Updated by: cazfi"

2022-07-07 04:52 Updated by: cazfi
  • Propietario Update from (Ninguno) to cazfi
  • Resolución Update from Ninguno to Accepted
Comentario

I think this is now ready to be merged (starting review period).

It's ok for now, but in the future it might make more sense to set counter.index in attach_city_counter() than in ruleset.c. I think this needs to be changed latest when we provide client side with the counters information.

2022-07-09 05:17 Updated by: cazfi
  • Estado Update from Open to Cerrado
  • Resolución Update from Accepted to Fixed
2022-07-17 17:20 Updated by: lachu
Comentario

Reply To cazfi

I think this is now ready to be merged (starting review period). It's ok for now, but in the future it might make more sense to set counter.index in attach_city_counter() than in ruleset.c. I think this needs to be changed latest when we provide client side with the counters information.

So I will change this? Sorry for asking this, but I have little English skills. I (probably) have done documentation ticket for now.

2022-07-17 17:23 Updated by: cazfi
Comentario

Reply To lachu

Reply To cazfi

It's ok for now, but in the future it might make more sense to set counter.index in attach_city_counter() than in ruleset.c. I think this needs to be changed latest when we provide client side with the counters information.

So I will change this?

Yes, you can do that. Just open a ticket for it yourself.

Attachment File List

Editar

You are not logged in. I you are not logged in, your comment will be treated as an anonymous post. » Entrar