Generalized auxiliary parameters in is_req_active()

What would you like to see in Freeciv? Do you have a good idea what should be improved or how?
Ignatus
Elite
Posts: 492
Joined: Mon Nov 06, 2017 12:05 pm
Location: St.Petersburg, Russia
Contact:

Generalized auxiliary parameters in is_req_active()

Postby Ignatus » Mon Feb 15, 2021 7:05 pm

A post mostly about the developer's side. The function is_req_active(), that stays behind all the requirements system, has now uncomfortably many parameters. Additionally to target player, city, unit and tile, there are other local parameters that specify objects in relation to which the requirement is calculated: other player, output type, building, specialist, unit type, action. And we can't introduce a requirement that e.g. select a specific tech to reduce its cost for some player without reworking a buttload of code that requires all these parameters and none else.

The idea is that selecting parameters never are supplied all at once for any requirement vector. E.g. "specialist" is used only for non-worker citizens' output effects, and "building" and "output_type" never go along. So, let the function, additionally to the four main parameters, have only three unspecific ones:
int index - for output type [for turn-break activities] and action number [for actions],
void *subjrel - for other player [generally] and specialist [for citizen iterator],
void *typerel - for building [for building iterator] and unit type [generally].
For cities in action enablers both unit types and buildings are possible, but not simultaneously (the city's production), so we can resolve it with an index-dependent requirement "ProductionKind".

Introduction of such a system will require a buttload of work once but will probably serve Freeciv project as long as it uses current requirement vector encoded rules (the system has many flaws main of which is inopacity of what is on the input but I doubt jumping from it to anything but natural language interpreted rules is genuinely possible). Seven parameters is what an average natural intellect operates with, some compound values should be constructed and passed if we ever need more. For example, a player-ranged effect of a reduced tech cost may easily use this system supplying tech as either subjrel or typerel, what fits the semantics better (?); or we can use extra id in index to fully describe the ability of building a specific extra in an action enabler, and it will require only local code changes.

The buggy matter is casting of void pointers, but we can do one thing to mostly remove the danger at least for a softmodder (a.k.a. ruleset author): each function that reads a requirement vector from file goes with a specifier on what role each variable parameter is used in, and there is a filter on requirements that throws up an error when you use a wrong requirement.

User avatar
Caedo
Elite
Posts: 512
Joined: Sun Feb 10, 2013 10:21 pm
Location: Stuttgart, Germany

Re: Generalized auxiliary parameters in is_req_active()

Postby Caedo » Tue Feb 16, 2021 12:00 pm

Better idea: Let's put all of that stuff into a struct.

The problem I have with your solution is twofold – firstly, as you said, void* are lost type safety, which I'd very much like to keep, thank you very much. Secondly, we're losing generality – if we ever want to expand the system in any way that might need more, we can't, without running into exactly the same problem we have now, and also questions of how to maybe redistribute different current parameters.

But of course, it still sucks if any change to possible requirement evaluation parameters requires updating countless places in the code where that new parameter isn't even used. So – let's put all of them into a struct.
This requirement evaluation parameter struct would have one field for each of those parameters we currently have (including player, city, unit and tile). Whenever we need to evaluate requirements, we get ourselves an all-nulls copy of that struct, fill the fields relevant to us with our values, leave everything we don't care about as-is, and then pass a pointer to that struct into the requirements system.
That way, if we wanna add a new requirement evaluation parameter, we just have to add a new field to the struct, with no additional effort for all the places we already have where we're not going to be using it. As an added bonus, when assigning the different fields, you can immediately see what goes where without having to look up the order of parameters in is_req_active().
~ A.V. L.

Ignatus
Elite
Posts: 492
Joined: Mon Nov 06, 2017 12:05 pm
Location: St.Petersburg, Russia
Contact:

Re: Generalized auxiliary parameters in is_req_active()

Postby Ignatus » Tue Feb 16, 2021 9:00 pm

Well, yes, I'm a mostly amateur programmer and can't yet get rid of "it can be encoded in 3 bytes, why do we use 4" logic :roll:

Surely struct can reduce the amount of recoding, though each one exotic requirement factor will intristically still have global impact (but I hope the struct does not affect web protocol so not a big problem). The alternative is witching to dynamic statement structures but it will turn the dumb power of requirement system into some slow (and probably unpredictably slow) interpreter... that we maybe need any way for a fuzzy-logic AI...

User avatar
Caedo
Elite
Posts: 512
Joined: Sun Feb 10, 2013 10:21 pm
Location: Stuttgart, Germany

Re: Generalized auxiliary parameters in is_req_active()

Postby Caedo » Thu Feb 18, 2021 4:01 pm

Ignatus wrote:Well, yes, I'm a mostly amateur programmer and can't yet get rid of "it can be encoded in 3 bytes, why do we use 4" logic :roll:

Hah, tell me about it. It took me years, until a university software engineering class, to get rid of that mindset – as it stands, however, both anecdotal practical experience and empirical studies generally show that doing things "properly" and focusing on expandability and maintainability not only saves effort/cost in the long run, but even measurably reduces the number of bugs in the code. Hence, me mentioning the "adding a new parameter is less work" and "understanding the code is easier" angles.
I guess that's the upside of having to take four different classes that, in part, just repeat the same stuff – ya learn to focus on that stuff automatically.

Ignatus wrote:Surely struct can reduce the amount of recoding, though each one exotic requirement factor will intristically still have global impact (but I hope the struct does not affect web protocol so not a big problem).

I'm not too familiar with the internal workings of C, but I don't actually think there will be any real global impact – between having an additional layer of indirection, not having to copy the parameters with every call, and the compiler's own optimization, there might be some effect on performance by switching to a struct in the first place, but after that, the only impact I can see happening from an additional parameter would be having to write an additional NULL when preparing the empty struct – something that would already happen in the current code when passing that additional NULL to the requirements system.

Ignatus wrote:The alternative is witching to dynamic statement structures but it will turn the dumb power of requirement system into some slow (and probably unpredictably slow) interpreter... that we maybe need any way for a fuzzy-logic AI...

Yeah, let's cross that bridge when we get there. I mean, I'd love to be able to do more complex logic with requirements, but a full-on dynamic interpreter feels like biting off more than we can chew right now.
~ A.V. L.