Use vcpkg to install TinyXML

[DanRStevens]

Noticed the following:
https://github.com/microsoft/vcpkg/tree/master/ports/tinyxml

Perhaps we should consider using vcpkg to install the TinyXML dependency, rather than including it directly in the repository.

As a side benefit, the Codacy scanner should stop reporting TinyXML related warnings for the NAS2D project.

[ldicker83]

TinyXML isn't really a dependency anymore as it's been directly integrated into the NAS2D::Xml namespace and heavily modified. I wanted to continue modifications in the future to further reduce issues, further simplify it and take advantage of modern C++.

The intention behind the modifications I made was to strip away functionality that wasn't needed for NAS2D and wouldn't be needed for almost any project that uses NAS2D. E.g., no schema's (TinyXML didn't support it to begin with, not fully anyway), and there was a lot of other stuff I pulled out as well (it's been awhile, can't remember off the top of my head).

I'm not opposed to pulling XML entirely from NAS2D and instead opting to use an internally specified format. At the time I didn't want to build a text/string parser but with the latest additions to C++ this may now be a trivial matter.

[DanRStevens]

Hmm, perhaps we should approach this from the angle of allowing an alternative input format, and see where that leads. Maybe we use a custom parser. Maybe we continue using TinyXML, but reduce the coupling between Sprite and Configuration, and TinyXML.

[ldicker83]

I would be okay with writing custom parsers for both Config and Sprite. I also want to refactor Config a bit so that it becomes almost strictly a key-value pair 'dictionary' of sorts. This is something that should be very simple to do via text. XML makes it easier to verify that the formatting is valid but a simple 'ini style' setup could be used. E.g., key=value:

// line is ignored
# line is ignored
key=value

Sprite is a little more complex by nature. It needs to be able to define one or more 'sheets', a number of 'actions' that reference 'sheets' and a number of 'frames' that are associated with 'actions'. Each 'frame' has several parameters. XML at the time seemed ideal for this. It still does. I could be persuaded to use something else but I'd like to avoid pulling in another dependency for it (e.g., if json or yml are suggested, it would still require a parser in order to make it happen and that usually means it needs a library unless I feel like rolling my own which I really don't).

You see the problem here. Sprite is the more complex of the two. I could be fine with a binary format instead of XML but I really liked the human-readable XML without needing to build a separate tool to 'compile' sprites.

[cugone]

@ldicker83

Personally, I prefer XML over json to me XML is easier to read. Both have established libraries, but in my experience TinyXML/TinyXML2 is easier to integrate without hassle and it can be dropped directly into the codebase with no libraries to download or compile when cloning the repo.

I don't know if it'll be of any use but wrote a KeyValueParser for a Config class if you want to take a look at them:
https://github.com/cugone/Abrams2019/blob/master/Engine/Code/Engine/Core/Config.hpp
https://github.com/cugone/Abrams2019/blob/master/Engine/Code/Engine/Core/Config.cpp
https://github.com/cugone/Abrams2019/blob/master/Engine/Code/Engine/Core/KeyValueParser.hpp
https://github.com/cugone/Abrams2019/blob/master/Engine/Code/Engine/Core/KeyValueParser.cpp

It supports command-line arguments (multiple key-value pairs per line), shorthand booleans, and quoted strings as well.

[DanRStevens]

Sounds like Sprites have some degree of nested information, which is better supported by XML and JSON which have structure for nesting, as opposed to INI files which are flat. You can hack nesting into INI files, but you'd do it by referencing other sections from within a section.

In terms of JSON versus XML, I'd say they're pretty similar, and largely interchangeable in terms of what they can do, though they seem to have a slightly different focus. JSON is primarily for data, in a text based machine parsable format, intended primarily for machine processing. XML is more for documents, which have human readable data, with a bit of machine parsable tags to mark things up and help machines interpret certain semantics of human readable text. An example being a web page.

Anyway, as things are already standardized on XML, I think I'd rather just continue with that until we have reason to change. I would however like to reduce coupling of the data structures with XML dependencies. If we do change things later on, that would make it easier. In particular, instead of Sprite taking a filePath to an XML file, it might instead take a vector of frame data. The processXml method could then become a non-member method (or a static method), and return a vector of frame data, which could then be passed to a Sprite constructor.

---

@cugone: I was looking at the code you linked and was thinking, maybe a templated method could be used to return an appropriately typed value:

auto value1 = config.GetValue<char>("CharKeyName");
auto value2 = config.GetValue<int>("IntKeyName");

Could do something similar as well for the SetValue method to reduce duplication.

Although it may complicate the code slightly, I would be tempted to pass the primitive types by-value, rather than by const-reference. Large objects though would certainly be easier to pass by const-reference. Though as you need to be able to serialize the objects to a string, the only large objects I see supported in that code are std::string.

[cugone]

@DanRStevens

The GetValue/SetValue functions' implementations are so different that templatizing/generalizing them would just be a waste of time. You'd end up writing a general case template that would never be called and requiring writing template specializations which overload resolution would resolve to anyway, putting you back to having a function for each type.

---

You could also have a sprite take a single XMLElement as one of the constructors, then it parses itself from there. This would facilitate Sprites being nested into other XML documents.

[DanRStevens]

Yes, that's a decent idea to split up XML processing so it can start at an XMLElement.

In terms of code reuse, I think there should be one base case of constructing from a vector. The XML processing construction could then be implemented in terms of that. Process XML into a vector, and then construct from the vector. To keep things efficient, the vector constructor can perhaps use move semantics for the vector. This is of course assuming an internal vector representation for the Sprite.

---

A little off topic, but I took a closer look at the GetValue/SetValue methods. Most of the SetValue methods are exactly the same, and so could be replaced with a template. The few that are different could perhaps have a custom to_string method added, which would make the template compatible with those types. Similarly for GetValue, most of the difference is in converting from a string to a specific type. If you had methods to do just that part of the conversion, the remaining methods bodies could all be collapsed into a single template:

template <typename Type>
void Config::GetValue(const std::string& key, Type& value) {
    auto found = _config.find(key);
    if (found != _config.end()) {
        value = GenericConversionFunction<Type>(found->second);
    }
}

[Brett208]

I'm having trouble following the conclusions of this issue. Is the intent still to pull tiny xml from NAS2D Core and add it as a vcpkg dependency? I'm assuming tiny xml would also need to be added to OPHD via VCPKG.

[ldicker83]

I'd like to ultimately remove TinyXML entirely as a dependency. Much of the 'need' for the XML in NAS2D was replaced with the Dictionary class. The general intent is to move toward that and, if any XML is used, to effectively hide that as an implementation detail versus a library interface.

At least that's the general idea that I got when we were talking about it.

[DanRStevens]

The TinyXML1 library source was incorporated directly into NAS2D, with local modifications. We would like to upgrade to TinyXML2. That is somewhat difficult though, as local changes and renames make the upgrade not so smooth. It's certainly not as easy as updating one line in a dependency file.

For external code, I would prefer using package managers to manage dependencies. For one, it's easier. Another reason is it tends to be faster, as package managers tend to deal in compiled binaries, so you're not recompiling the dependency code each time. Yet another reason, it discourages making local modifications, so you avoid some of the problems with maintaining and upgrading libraries that have local modifications.

Currently OPHD delegates to the NAS2D project for dependencies. There are no additional dependencies added by OPHD, so delegation is pretty easy. Any updates to dependencies in NAS2D will cause updates in how OPHD is built. If you check the .appveyor.yml file for OPHD, you'll see it references files right out of the NAS2D folder for dependency caching and installation.

cache:
  - C:\tools\vcpkg\installed\ -> nas2d-core\InstallVcpkgDeps.bat
install:
  - vcpkg integrate install
  - call nas2d-core\InstallVcpkgDeps.bat
  - set APPVEYOR_SAVE_CACHE_ON_ERROR=true

---

The Dictionary class doesn't fully replace the XML library. It merely provides an intermediate data format that decouples from the disk format. You could potentially write a loader for XML, or JSON, or really any file format, and load the data into Dictionary objects, which can then be processed by the code.

I have no immediate plans to remove XML support from NAS2D, as it still relies on loading XML data for the Configuration and Sprite code. It should be fairly decoupled now, so we could potentially rely on an alternate format, or rely on downstream code to handle parsing to Dictionary objects, and then processing from there. So yes, we could potentially remove XML support from NAS2D at some point in the future. That amount of code that directly depends on XML code in NAS2D is quite minimal now.

Downstream, the OPHD project still relies heavily on XML code from NAS2D. I would want to refactor the downstream OPHD code to decouple it from XML code before considering removing XML from NAS2D. As it stands now, we would horribly break OPHD if we tried to remove XML support from NAS2D.

[Brett208]

Thank you for the explanations. Makes good sense now.

Is there a desire to support an arbitrary number of nested dictionaries in saved data, or is it intentional to only allow something looking like std::map<std::string, std::string>?

[DanRStevens]

I had thought about nesting with dictionaries, though currently it's unsupported, and I never developed a clear plan on supporting it, or if nesting was even required. The Dictionary class is designed so that each value can be serialized as a string. Supporting nesting would mean the entire Dictionary class would need to be serialized as a string. I suppose that's doable.

Mostly I've been avoiding data structures with deep nesting.

[ldicker83]

It would break OutpostHD but OPHD could either replace the XML code with a different library or simply include the XML by itself. I do ultimately want to remove XML from the NAS2D interface as I really think it's beyond the scope of the library itself -- as an unexposed implementation detail it's fine but I'd still like to consider something far lighter weight to handle things like config and sprite data.