Configuration refactoring

[stm2]

Bei den ganzen Gerede über Konstanten neulich, habe ich mir mal überlegt, wie man das besser machen könnte und das kam dabei heraus:
develop...stm2:6628b2d7805c13f4d8be8b597ed3162a1b93e41b

Statt

int multi = get_param_int(global.parameters, "rules.trollbelt.multiplier", STRENGTHMULTIPLIER);

schreibt man jetzt

    int multi = eressea_parameter_int(E_RULES_TROLLBELT_MULTIPLIER);

Die Parameter mit Defaults landen in settings.h (falls ich die Archtitektur da richtig verstehe).
Außerdem sollen natürlich alle #defines, angefangen bei denen in config.h, auch zu Parametern werden und in settings.h landen.

Zwei Vorteile:

• Die Parameter sind alle an einem Ort anstatt irgendwo im Code herumzuschwirren, was die Dokumentation für Spielleiter leichter macht.
• Der Zugriff ist effizienter und die Duplizierung der Caching-Logik mit statics ist aufgelöst.

Fragen:
Was ist der Unterschied zwischen Variablen in config.h und in settings.h und ist diese Unterscheidung sinnvoll?

Ist dagegen softwaretechnisch was einzuwenden?

Es schafft zwar eine gewisse Kopplung, aber das Problem ist im Grunde jetzt schon vorhanden und man muss das, glaube ich, getrennt lösen.

[ennorehling]

Das ärgerliche an so einem enum wie parameter_name_t ist, das man da parallel dazu ein Array pflegen muss in settings.c, und dass man settings.h über kurz oder lang in jeder .c Datei einbinden muss, was bei einer Änderung (neuer Parameter) dazu führt, das man alles neu kompiliert. Das ist eine Menge Koppelung, die es vorher (absichtlich) nicht gab. Da finde ich den lookup per String schöner, wie er vorher war, auch wenn er etwas langsamer ist.

[stm2]

Ich finde, das mit dem Kompilieren sollte kein Argument sein. Allerdings auch nicht, dass der Zugriff etwas schneller wäre. Das könnte man mit einer anständigen Datenstruktir für String-Lookup auch hinkriegen.

Die Kopplung mit dem Array ist schon ein Problem. Wie wäre es stattdessen, wenn mal alle Config-Strings vor Benutzung registrieren muss? Das würde wahrscheinlich so laufen, dass in eressea.c:game_init für jedes Modul ein Initializer aufgerufen wird. Dann könnte man zwar nicht zur Compilezeit, aber immerhin zur Laufzeit eine Liste aller Strings ausgeben.

[stm2]

Das sähe dann jetzt etwa so aus:
develop...stm2:2ab3951d6043b9493c71150c492852a71302d2d8

[ennorehling]

So eine Registrierung wie register_config_parameter ist ja auch nur noch mehr Boilerplate. Ich mag so etwas nicht gerne, es haben jetzt schon zu viele Module ein Paar von _open und _close Funktionne, um Zeug (callbacks, attribute, etc) zu registrieren. Dass man alle attrib_type structs registrieren muss, ist eigentlich auch ätzend, aber geht wohl kein Weg drum herum.

Kannst Du noch einmal erklären, warum Du hier überhaupt etwas ändern willst? Abgesehen davon, dass Konfiguration nicht mit #define definiert werden soll, was ist denn an get_param & co verkehrt? Der String-Lookup ist wohl eher langsam (Hui, ich dachte, das wäre anders gelöst), und die Werte sind intern alle Strings, die dann als bool oder int oder whatever interpretiert werden, aber das sind alles Probleme, die man im Rahmen der alten Funktionen lösen kann, oder?

[ennorehling]

Zu meinem Argument mit dem Kompilieren noch einmal: Ich finde das schon prinzipiell ein Problem. Computer sind so schnell geworden, dass es für Eressea kaum noch eine Rolle spielt [1], aber ich habe auch schon an Projekten gearbeitet, wo ein make clean all 2 Stunden dauerte, und man ständig alles neu kompilierte, weil so eine all_the_enums.h Datei überall inkludiert wurde (insbesondere in anderen Headern), und sich mit quasi jedem commit geändert hat. Es ist schlechter Stil, ich finde es hässlich, und ich möchte es nicht in "meinem" Code sehen :-)

Vielleicht mache ich mal einen Branch mit meiner eigenen Verbesserung der existierenden Funktionen, das könnte Spaß machen, und vielleicht instruktiv sein.

[1] Eressea's make clean all dauert auf meinem Raspberry Pi derzeit knapp unter 13 Minuten, das ist eine absolute Ewigkeit.

[ennorehling]

Alternative Implementation, mit schnellerem Lookup und gleichem Interface: 06f8ba9

Das ist ganz schön fieselig mit critbit so ein string->string Dictionary zu bauen, aber ich glaube, es stimmt.

[stm2]

Ich glaube, dass kann man auch in language.c irgendwo nachlesen.

Kannst Du noch einmal erklären, warum Du hier überhaupt etwas ändern willst?

Danke, dass du fragst. Das hat mich gezwungen, noch mal genauer nachzudenken. Ich habe da ein paar Probleme:

1. Effizienz des Zugriffs -- Kann man, wie demonstriert, auch anders lösen, aber mit einem array wäre es noch schneller und leichter verständlich.
2. Existenz einer globalen Variable, die auch noch global heißt. -- Kann man teils durch Umbenennen lösen, teils ist es wohl in C nicht gut anders zu machen ohne Dependency Injection.
3. Teilweise ungekapselter Zugriff auf diese Variable (global.cookie sticht da besonders ins Auge) -- vielleicht ist man als C-Programmierer mehr darauf sensibilisiert da keinen Unfug anzustellen, ich finde das unschön.
4. Codeduplizierung (if (value < 0) value = get_param(global.settings...) teils mit, teils ohne gamecookie = global.cookie etc. kommt ungefähr 57 mal im Code vor.)
5. Unklare Aufgabe dieses struct settings: Da gibt es Informationen über den Report (settings.gamename, settings.attribs), über die Server-Konfiguration (global.inifile) und über die Regelkonfiguration (settings.producexpchance_!)
6. Geht weiter damit, dass diese Regelkonfiguration auf verwirrend viele Arten gelöst wird (#defines, settings.parameter, settings.global_functions, Funktionen in config.h entertainmoney()).
7. Fehlende Typsicherheit bei den Regelparametern (damit meine ich "rules.ship.drift_damage" etc)
8. Nicht vorhandene Dokumentation der Regelparameter
9. Und als größter Punkt, mangelnde Modularität des gesamten Codes.

Die letzten drei Punkte sind die einzigen, die größere Änderungen nötig machen. Mein jetziger Versuch ist da auch noch nicht richtig im Ziel, um diese Probleme zu lösen, wie ich inzwischen sehe. Ich glaube man müsste ungefähr das machen:

• Die Caching-Logik für Regelparameter kapseln und zentralisieren.
• Den (das?) struct settings aufteilen in mehrere Objekte mit klaren Zuständigkeiten
• Die Spielparameter (also Dinge wie #define HERBS_ROT, aber auch rule_transfermen() und auch int entertainmoney(), raus aus der config.c in die einzelnen Module oder in eigene Module.
• Die Regelparameter (mit Defaults und Typ) registrieren. Ja, das erfordert etwas "Boilerplate-Code", aber ich glaube, das bringt genug Vorteile mit. Erstens könnte man dann auf Knopfdruck eine Liste aller Optionen ausgeben (mit Defaults, Typen und am besten noch mit in irgend einer xml-Datei hinterlegter Erklärung).

Zweitens würde ich gerne mehr dahin, dass man sich sein eigenes Eressea zusammenstecken kann und das ist im Moment sehr, sehr schwierig. Und das ist eigentlich der Punkt, der mir hier am wichtigsten ist, auch wenn mir das selbst am Anfang nicht klar war. Und bis dahin ist es ein sehr weiter Weg.

Aber ich bin mit meinem Verständnis der jetztigen Architektur auch noch nicht ganz so weit, da richtig klar zu sehen. Es gibt zum Beispiel bestimmt eine interessante Erklärung, warum es eine config.h und eine settings.h gibt und beide anscheinend teilweise das gleiche tun. Aber ich bin nicht sicher, ob ich sie hören will. ;)

[ennorehling]

1. Die massiv duplizierte Cache-Logik mit den statischen Variablen ist mir auch ein Dorn im Auge, und ist eine schlechte Lösung für das Problem, dass get_param ineffizient implementiert war (Liste von strings, durch die man einzeln strcmp machen musste). Das ist technical debt, und ich habe es in letzter Zeit eher abgebaut, als neue Fälle hinzu zu fügen. Es macht auch das Testen regelmäßig kaputt, wegen der statischen Variablen. Das sollte ich bei Gelegenheit mal in einem clean sweep alles killen.
2. Dass in config.c eine Menge Kram ist, der dort nicht hin gehört habe ich heute erst wieder beim Testen gemerkt (config.test.c testet entsprechend auch Dinge, die nicht mit Konfiguration zu tun haben, das war lästig). Bin ich völlig einig, dass man das mal aufräumen muss.
3. Das struct settings ist noch aus einer Zeit, als es die parameter nicht gab, und alles was konfigurierbar war, einzeln aus einer .ini Datei gesetzt wurde. Deshalb sind da so Dinge wie gamename und producexpchance drin, die eigentlich auch mit get_param gelöst sein sollten.
4. Es gibt immer Teile des Codes, die hinter dem Soll-Zustand hinterher hinken, man kann nicht jedes Mal, wenn sich der ändert, alles neu schreiben. Deshalb gibt es in config.h noch #defines, variablen im struct settings, und booleans, die mit get_param_int direkt abgefragt werden, statt in ihrem Modul in einer Funktion gekapselt zu sein. Modularisierung auf Basis der Regeln ist generell auch etwas, das noch weiter getrieben werden kann.
5. Typsicherheit der Regelparameter ist bisher noch nicht so sehr ein Problem gewesen, kann man aber auch im aktuellen Rahmen etwas mit machen (get_param_int kann schreien, wenn der Wert kein Integer ist, get_param_bool einführen, usw).
6. Dokumentation der Stellschrauben gibt es in der Tat nicht, hat es aber bisher auch wenig Bedarf für gegeben, weil die Anzahl der Leute, die ein Eressea mit eigenen Regeln machen wollen, auf mich und E3 beschränkt war. Wenn da Bedarf besteht, kann man das aber mal anfangen, und eine Basis-Doku aus dem Code automatisch erstellen, die man dann mit Erklärungen erweitert.
7. Es gibt eine config.h, weil da früher alle #defines drin waren, und außerdem waren da verschiedene Compiler und Betriebssysteme drin abhehandelt. Letzteres ist nach platform.h gewandert, und ersteres sollte mal in settings.h gehen, aber dann war die Idee, das nicht mehr mit #defines zu machen, sondern über Konfigurationsdateien, und der Kram ist aus irgend einem Grund in config.h gelandet, wo traditionell eine Menge Mist drin steckt: getunitpeasants und read_unitid sind so Fälle, die ich auch nur schwer erklären kann...
8. Zuletzt gibt es da noch den Versuch, den Code in Blöcken zu trennen. src/kernel/* soll z.B. kein #include aus src/* machen, nur anders herum. Das macht es an einigen Stellen kompliziert, wenn beispielsweise Parser, Einheitennummern und das TEMP Schlüsselwort aufeinander treffen (siehe oben, read_unitid). So richtig sauber ist das nie geworden, aber ich versuche halt, es durchzuhalten.

Fazit: config.c ist ein Clusterfuck, und es gehört so gut wie nichts dort hinein. Vielleicht sollte der get_param und set_param Kram einfach nach settings.h verschwinden? Ich glaube nicht, dass es hilft, das Interface zur Konfiguration noch einmal zu ändern, denn der aktuelle konfuse Zustand ist wohl primär geschuldet, dass das in der Vergangenheit mehrfach passiert ist, und ich bin mit der aktuellen Soll-Lösung noch immer zufrieden (genug).

Nachwort: Ich war früher auch total heiß darauf, dass Eressea super flexibel ist, und man beliebig an den Regeln ändern kann, aber inzwischen habe ich drei Spiele mit zwei Regelwerken im Gang, und kann mich kaum noch erinnern, welche Regeln wo gelten und wie funktionieren. Das muss ich jedes mal im Regel-Wiki nachsehen. Noch eines mache ich auf absehbare Zeit nicht, aber wenn sich ein Spielleiter findet, der das möchte, und der ein Konzept hat, dann ist es vielleicht wertvoll, die Arbeit dafür zu machen.