Incidencia #44084

changed all ocurrences of action_list to action_group and act_list to act_group

Abrir Fecha: 2022-03-12 23:25 Última actualización: 2022-07-21 21:25

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

Details

for adding custom actions it will be necessary to store information about the actions, the method i thought about would be creating a speclist for action structs and changing the structs to store all necessary information about the actions. however action_list_iterate is being used, so it is necessary to change it. also having various lists function that aren't related to the lists would be confusing so i changed every ocurrence for the currently existing functions from list to group.i used ripgrep to find all files with either action_list or act_list and then on each of these files used sed to change action_list to action_group and act_list to act_group.i then used git diff to find all the differences and if it looked if it shouldn't be changed i reverted the change. example: the ChangeLog File i also added a rule_name field to the action struct.

Ticket History (3/17 Histories)

2022-03-12 23:25 Updated by: dark-ether
  • New Ticket "changed all ocurrences of action_list to action_group and act_list to act_group" created
2022-03-12 23:55 Updated by: cazfi
Comentario

Reply To dark-ether

however action_list_iterate is being used

I've not looked the patch yet, but aren't those (or at least some of them) iterations supposed to happen over all actions, custom or not. So all actions should be in the same list, no separate constructs for custom and internal actions (as much as there is "internal" actions in the long term plans - I don't know how much they are supposed to get generalized away).

2022-03-13 01:19 Updated by: dark-ether
Comentario

Reply To cazfi

Reply To dark-ether

however action_list_iterate is being used

I've not looked the patch yet, but aren't those (or at least some of them) iterations supposed to happen over all actions, custom or not. So all actions should be in the same list, no separate constructs for custom and internal actions (as much as there is "internal" actions in the long term plans - I don't know how much they are supposed to get generalized away).

i am not certain but i think no, there is a action_iterate function which seems to be for all actions iteration, and in the code i looked at using action_list_iterate it seems to iterate over a array of stored actions id so it probably is iteration over a subgroup of all actions. but even then i haven't changed much, just renamed all action_list and act_list ocurrences to action_group and act_group if i haven't missed anything, it should continue working exactly the same as before the patch. this patch is a preparatory one for when i create a speclist instance for actions

(Edited, 2022-03-13 01:23 Updated by: dark-ether)
2022-03-13 01:32 Updated by: dark-ether
Comentario

Reply To dark-ether

Reply To cazfi

Reply To dark-ether

however action_list_iterate is being used

I've not looked the patch yet, but aren't those (or at least some of them) iterations supposed to happen over all actions, custom or not. So all actions should be in the same list, no separate constructs for custom and internal actions (as much as there is "internal" actions in the long term plans - I don't know how much they are supposed to get generalized away).

i am not certain but i think no, there is a action_iterate function which seems to be for all actions iteration, and in the code i looked at using action_list_iterate it seems to iterate over a array of stored actions id so it probably is iteration over a subgroup of all actions. but even then i haven't changed much, just renamed all action_list and act_list ocurrences to action_group and act_group if i haven't missed anything, it should continue working exactly the same as before the patch. this patch is a preparatory one for when i create a speclist instance for actions

i went and to check and yes it is exactly this action_iterate is for all actions, action_list_iterate(with this patch being renamed to action_group_iterate) for some actions, if you look at their definition in actions.h action_iterate starts with id = 0 and goes up to id = NUM_ACTIONS-1 and action_group_iterate receives a array of actions ids and iterate over them. so when i add extra actions i will have to add another loop for the "extra actions", about long term plans i can't know what Sveinung planned but at least for me all actions should eventually be unhardcoded, and to do so temporalily there will be a a hardcoded action list and a custom action one and we gradually move actions from being hardcoded to being custom.

(Edited, 2022-03-13 01:47 Updated by: dark-ether)
2022-04-01 00:36 Updated by: alienvalkyrie
Comentario

Couple of things:

  • This ticket – changing the names of stuff called "action_list" that does not actually refer to (linked) lists – should probably be considered independent of any future goals related to custom actions; for one, I don't think it's sensible to use speclists for top-level ruleset data. I do think the goal of this ticket is sensible regardless of how we go about doing custom actions, but I felt it important to point this out.
  • Adding a rule_name field to the action struct should be a separate ticket – for one, it has nothing to do with the changing of a bunch of names; but also, there's a lot more that needs to be done along with adding the new field (to make sure it's actually useable – having a field with a name as well-established as rule_name comes with expectations from other people reading and working on the codebase).
  • It seems reverting some unintended changes from the original patch (in savecompat.c) slipped into the second (unrelated) patch; those should be added to (or rather, removed from) the actual patch for this ticket.
  • I'm not sure I like the name "action group" – it feels like it doesn't really mean anything. action_array might be a better name, though of course, these are different from "normal" arrays in that they are ACTION_NONE-terminated. So if there are other kinds of action array used throughout the codebase, that might not be an improvement.
  • A number of documentation comments still refer to "list" or "action list"; those should be brought in line with the new nomenclature.
  • Commit messages should reference the relevant ticket (e.g. "See osdn #44084"), and should, content-wise, explain what the patch does, rather than how it does it – for instance, to someone looking back through the codebase decades from now, it won't be relevant that this change was made using sed and ripgrep, but it will be useful to quickly see that it's about renaming things that were (erroneously) called "action_list". (You can also look at existing commits to get a feel for what they're typically like.)
  • Finally: Reviewing contributions is a lot easier when the involved text uses proper capitalization, punctuation and line breaks – when trying to read walls of all lowercase text with little punctuation or structure i for one find myself zoning out and having to read the same part multiple times to actually register what it says, this doesn't make it easy or fun to work on and makes it less likely your work gets the necessary attention.
2022-04-02 03:05 Updated by: dark-ether
Comentario

I mostly agree with everything you said, and am mostly thankful that you decided to respond anyway even considering all the problems in my patches. When i created this patch i was unaware that i could change git history as long as no one had received it yet,and wrongly assumed it was impossible without even thinking about it.
I do agree that action group is a bad name, going of your suggestion i would propose NTArrray, short for none terminated. I also considered Carrray as it similarly to a c string is terminated by the empty value but that could be confusing. i am open to further suggestions.
I will provide a new, better patch soon.
Sorry for my English.
Regarding discussion which would you prefer, that i create a ticket or forum post to discuss something i am planning or should i just create a ticket when i implemented something with a patch?
For instance after you mentioned that linked lists are undesirable on the game struct. I am planning on creating a wrapper for dynamic arrays. When i have an idea like this should i

  1. create a forum post to discuss
  2. create a ticket to discuss
  3. create a discussion in another platform i don't know about yet
  4. not discuss and only post a patch when complete

4 seems wrong, and i am leaning towards 2, obviously i am only going to have some discussion tickets open at a time ,so would that be okay? specifically i plan 2 discussion tickets one that is my current overarching goal and another on what i am specifically doing in the moment.

2022-04-02 06:06 Updated by: alienvalkyrie
Comentario

Reply To dark-ether

I am planning on creating a wrapper for dynamic arrays.

We already have dynamic arrays (though we call 'em "vectors"); see to comment at the top of utility/specvec.h for info, and the places where it's included for examples (a very prominent one being the requirement_vector).

However: We also don't generally use those for ruleset data, in favor of statically allocated arrays with a fixed maximum size. If I had to guess, the original reason for this is that we need that maximum size for the way the networking code handles arrays – the packet structs come with allocated space for the maximum possible array size. So we need to have a maximum possible number of e.g. unit types, or technologies, or custom actions; and when we have that, there's really no point in complicating the ruleset structure by using anything other than a statically allocated array.
(Expanding the networking code to be able to work with specvecs would be awesome, of course, but that's a whole 'nother thing entirely. It would also open the door for a number of issues about whose responsibility is freeing those specvecs.)

2022-04-02 06:08 Updated by: alienvalkyrie
Comentario

Reply To dark-ether

I do agree that action group is a bad name, going of your suggestion i would propose NTArrray, short for none terminated. I also considered Carrray as it similarly to a c string is terminated by the empty value but that could be confusing. i am open to further suggestions.

On further review, I do think "array" should be fine – it's not like there are any other sensible values to terminate it anyway, and if it was an array with sizing information added, well, you'd probably be using a vector for that, not a plain array. So I don't think there's significant risk of confusion.

2022-04-02 06:27 Updated by: alienvalkyrie
Comentario

Reply To dark-ether

When i have an idea like this should i
1. create a forum post to discuss
2. create a ticket to discuss
3. create a discussion in another platform i don't know about yet
4. not discuss and only post a patch when complete

4 seems wrong, and i am leaning towards 2, obviously i am only going to have some discussion tickets open at a time ,so would that be okay? specifically i plan 2 discussion tickets one that is my current overarching goal and another on what i am specifically doing in the moment.

For larger features, there's often a "meta-ticket" that tracks a number of related tickets, and where general discussion about how to go about it (and what new sub-tickets we need) can happen. For things that show up on their own, it can be helpful to start a ticket in a way that is entirely problem- or goal-oriented (i.e. what we want to achieve), and then figure out / discuss how to go about solving the problem / achieving the goal there.

Forum is of course a bit better-suited to long-form discussion, and reaches more people – especially for things that have direct effects outside the code, open questions about how something should be represented in ruleset files etc., that can be useful, since most people probably don't follow everything that happens over here. On the other hand, it splits discussion across two sites, which makes it harder to follow, requires people to register for another account, and which increases the likelihood of parts of the discussion being lost if one site goes down / has its database corrupted / is moved to a new backend without the option to migrate the data / etc.

There is also an IRC channel, I am told (the information is somewhere on the website or the wiki, I think?), but I can't attest to how active that is, seeing as I myself haven't even so much as looked in an IRC client's general direction in years.

2022-04-03 00:07 Updated by: cazfi
Comentario

Reply To alienvalkyrie

There is also an IRC channel, I am told (the information is somewhere on the website or the wiki, I think?), but I can't attest to how active that is, seeing as I myself haven't even so much as looked in an IRC client's general direction in years.

If someone goes there, let me know. The couple of times I've been on the channel after the move to Libera.CHAT, there has been no activity at all, so I rarely bother to start up the client myself. Of course it might be that somebody else is also checking it occasionally when I'm not there and also seeing as totally dead place.

2022-04-17 19:27 Updated by: cazfi
Comentario

For development related discussion that does not quite fit any ticket, the best option would probably be freeciv-dev mailing list. Forums might be an option in some cases where end-users may want to participate. freeciv-dev: https://www.freelists.org/list/freeciv-dev

2022-07-20 05:02 Updated by: dark-ether
Comentario

my definition of soon probably needs some work but here it is

2022-07-20 06:05 Updated by: alienvalkyrie
  • Propietario Update from (Ninguno) to alienvalkyrie
  • Resolución Update from Ninguno to Accepted
  • Hito Update from (Ninguno) to 3.2.0
Comentario

Looks good.

A note for the future: Commit messages should typically refer to the issue they're related to, e.g. "See osdn#44084"; if you do that already, it saves us the effort of having to amend the commit.
(There's no need to submit a new patch here, since I've already changed it – this is just for future reference.)

2022-07-21 21:25 Updated by: alienvalkyrie
  • Estado Update from Open to Cerrado
  • Resolución Update from Accepted to Fixed

Editar

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