ruleset dumping for testing (modding & coding)

[atlimit8]

I created a pull request (PR758) for debugging mods and changes to the ruleset.
When using the added command line option, it dumps the composite ruleset loaded by OpenXcom to the specified file.
This can be used to check that changes the the ruleset structure are loaded correctly.
It can also be used when making or mixing mods, since it shows the resulting ruleset.
If having it wrapped in ifdef blocks for conditional compilation is preferred, I can do that.
If this feature is unwanted, I will close the pull request.
Comments and change requests are welcome.

[moriarty]

that sounds like a very useful feature to me, especially for testing if several mods are compatible

I suppose it's not easily possible to somehow tag the ruleset parts that have been changed?

[atlimit8]

void dumpYamlToFile(const std::string &filename) const could be changed to void dumpYamlToFile(const std::string &filename, const std::string &baseFilename) const where baseFilename is a base YAML file loaded into a base tree for comparison.
The DUMP_BASE top level node would be used for chaining differential ruleset dumps.
The DUMP_DELETED top level node could be used for identifying anything removed without a special delete tag, which could help check for lost nodes when dumping due to incorrect/outdated code by checking it against the original ruleset.

The -dumprulesetbase command line option would specify the baseFilename.

Another option that can also be combined with the previous is recursive differential dumping in void Game::loadRuleset().
The -dumprulesets command line option would specify the use of recursive differential dumping of the ruleset using its PATH parameter as a prefix with incremental numbers followed by ".rul" appended for each dump.
The -dumprulesetbase command line option could be extended to support +<number>, where <number> is the ofset into the list of rulesets from where to start.

Code: [Select]

openxcom -dumprulesets diff_ -dumprulesetbase +1would skip the first entry in rulesets and output diff_0.rul, diff_1.rul, ...


The code would probably be O(n^2) for dumping, but would have almost no effect on normal operation of OpenXcom.

[atlimit8]

Defaults change the YAML tree making comparisons of YAML trees inaccurate.
The Ruleset and Rule* objects will have to be copy-able and have their own difference dump functions, but this would provide a better match with the ruleset model used by the OpenXcom including any new defaults.
This would allow Xcom1Ruleset.rul to be used as a base for differential dumping removing extraneous rules from the dump(s), but would be more work.

[SupSuper]

Or you could just do a file diff.

Anyways I'm kinda 50:50 on this feature. On one hand, being able to dump the ruleset is useful for end users to test their stuff, in fact we had this feature way back before the great yaml-cpp upgrade.

On the other hand it's also a maintenance headache. Moddability is still largely in flux, so with every change we also had to update all the associated loads/saves/etc, and the feature often went buggy and outdated because it was rarely used and tested, so it was just dropped during the upgrade to save us the trouble. After all every feature someone adds is another feature we have to maintain.

I'll see what other people have to say on the subject.

[atlimit8]

I agree that it adds another function to each rule class to maintain; however, a save function can be next to the load function, updated in unison, and followed by using a differential dump to check for typos in the load & save code as well as the ruleset.

Actually, I copied the code from the load functions and used regular expression replacement to transform most of the code from the load functions.

[atlimit8]

What if I design some template functions that allow one template function to be written for both loading and saving?
This would remove the extra maintenance by allowing one template function specialize for a load stub and a save stub.
Templates are already used for shading in OpenXcom.

[Yankes]

I think that is possible without polluting class definition with template function that need be compiled everywhere.
This is example how it could work:

Code: [Select]

https://**************
https://in Test.h
https://**************

class Test
{
int _i;
template<typename Y>
friend void func(Y&, Test&);
};
class Test2
{
Test testA;
Test testB;
template<typename Y>
friend void func(Y&, Test2&);
};

https://**************
https://in Test.cpp
https://**************

https://two classes that should have symmetrical API
struct LoadYAML { int z; };
struct SaveYAML { int z; };

template<typename Y>
void func(Y& i, Test& t)
{
t._i = i.z;
}
template<>
void func<LoadYAML>(LoadYAML& i, Test2& t)
{
func(i, t.testA);
func(i, t.testB);
}
template<>
void func<SaveYAML>(SaveYAML& i, Test2& t)
{
func(i, t.testA);
func(i, t.testB);
}

int main(int argc, char** argv)
{
Test a;
Test2 b;
LoadYAML l;
SaveYAML s;
func(l, a);
func(s, a);
func(l, b);
func(s, b);
}
All templete code will be in cpp files. Its still need some fix because `func<SaveYAML>(SaveYAML& i, Test2& t)` require definition of `func(Y& i, Test& t)`. But this can be fixed using some indirection.
This approach allow use templates without slowing down compilation, because templates will be parsed and instanced only in one compilation unit.

Btw if we want drop YAML in favor of binary format this change would help, because it will limit modifications required to change it.

[atlimit8]

It could also mix binary cache and some other format , such as YAML.
Then a timestamp check could be used to see if the binary cache needs to be updated.
The list of used rulesets would also be in the binary cache.
This would allow faster loading when using the same rulesets in the same order.
However, checking time stamps usually requires platform dependent code.

Lastly, I do plan on having load and save stubs in the cpp files that would call the template functions which will also be defined the the cpp files.
As long as the template functions are not defined in the header and not used anywhere else, that should have no more than a negligible effect on compilation.

Otherwise, SomethingFor<type> structs can be specialized for the job where Something is something else.

[atlimit8]

Here is my current model:
in header:

Code: [Select]

struct Example
{
int x, y;
template<MemberMappingOp op, bool required, typename Node>
bool map(Node& node, const typename MemberMappingNodeKey<Node, Example>::type *key = 0, const Example *oldValue = 0);

https:/// Loads the example from YAML.
void load(const YAML::Node& node, std::string *key = 0);
https:/// Saves the example to YAML.
YAML::Node save(const std::string *key = 0, const Example *oldValue = 0) const;
};

cpp file:

Code: [Select]

template<MemberMappingOp op, bool required, typename Node>
bool Example::map(Node& node, const typename MemberMappingNodeKey<Node, Example>::type *key, const Example *oldValue)
{
bool result = true;
if (!MemberMapping<op, required>::map(*this, &Example::x, node, "x", original)) result = !required;
if (!MemberMapping<op, required>::map(*this, &Example::y, node, "y", original)) result = !required;
return result;
}

void Example::load(const YAML::Node& node, std::string *key)
{
map<MemberMappingOps::load, true, YAML::Node>(node, key);
}

YAML::Node Example::save(const std::string *key = 0, const Example *oldValue)
{
YAML::Node node;
map<MemberMappingOps::save, true, YAML::Node>(node, key, oldValue);
return node;
}



[atlimit8]

How acceptable are macros?

This code can be cleaner, more readable by using macros.
bool Example::map(Node& node, const typename MemberMappingNodeKey<Node, Example>::type *key, const Example *oldValue) using macros:

Code: [Select]

bool result = true;
MAP_MEMBER(op, required, result, Example, x, node, "x", oldValue)
MAP_MEMBER(op, required, result, Example, y, node, "y", oldValue)
return result;
where MAP_MEMBER is defined by:

Code: [Select]

#define MAP_MEMBER(op, required, result, Type, member, ...) \
if (!MemberMapping<op, required>::map(*this, &Type::member, __VA_ARGS__)) \
{ \
if(op == MemberMappingOps::equals) return false; \
result = !required; \
}