Xml improvements

[Brett208]

Must be paired with assets update to compile: OutpostUniverse/ophd-assets#1

It looks like the assets are a submodule of this repo, but I had trouble linking them on my machine for some reason.

Related to #830. Not quite a string dictionary but tried to consolidate some of the xml related code.

The last commit is pretty big. I had trouble teasing it into smaller chunks due to the serialization involved. I was experimenting with template functions and enum lookups. Hopefully it looks ok...

This PR will allow someone to change the planet attributes without recompiling the source code.

[ldicker83]

Looked over it. Generally no objections, looks pretty good. I made a few notes of areas where brace placement is inconsistent with the rest of the code. With that corrected this PR should be good to go.

[DanRStevens]

I really like the idea of moving that data to a data file.

I had a few suggestions and observations looking at the last commit, though they were all pretty minor.

[DanRStevens]

I kind of feel like destination should be a return value, rather than an out parameter.

[Brett208]

This is probably a premature optimization. I want to move some large strings (definitions of the planets) into the xml file in the near future and I figured we may not want to pass large chunks of data in the return.

I'll make this a return for now as suggested and we can revisit if anyone ever notices a slowdown...

[Brett208]

EDIT: I was attempting to keep function names the same between parsing an enum and parsing a basic value. Since type deduction is not possible based on return values of a function, I think using a return value here isn't possible. The alternative if we want a return value would be to provide separate names for each function. I was hoping to keep function names the same to possibly allow collapsing if/else statement logic cases in future iterations.

[DanRStevens]

As of C++17, the compiler is guaranteed to do the optimization for you:
https://en.cppreference.com/w/cpp/language/copy_elision

When returning large objects, the compiler will effectively pass in a hidden parameter with the address of the variable being copied to, and will instead construct the value in place. Hence no copy during the return.

---

You can potentially use template methods to parameterize the return type. If the functions have different implementations, this can be handled with template specialization. Though often, just giving different functions different names may be easier and sometimes makes more sense. I'd need to look over the code again to consider this.

[Brett208]

The unnamed namespace work is a bit hairy due to the underlying structure. There are really 2 different sets of information being represented in Planet.h (the attributes of a planet and how to draw/manipulate in the UI a planet on screen). That logic is partially split between PlanetSelectState and Planet, making it even more complex. I think disentagling that is probably not a good idea in this thread though.

Made the easier style changes in addition to the anonymous namespace. I'll work the other points later.

Thanks for the thorough and quick feedback.

[Brett208]

I wasn't thrilled with the results of adding the anonymous namespace. Having to prepend the :: qualifier to some of the functions may cause noise later.

I am complete with updates unless the additions I made need further refinement before merging.

[DanRStevens]

I think this is good to merge. I just want to take a bit of a closer look at the namespace scope issue, and why :: is needed in a few places. Maybe there's another workaround.

[DanRStevens]

Not that it matters, though you could potentially have put this in the first unnamed namespace, possibly even replacing the function prototype. Though there is nothing wrong about re-opening an unnamed namespace and keeping the implementation separate.

[DanRStevens]

Hmm, yes, I see the discrepancy with the ::. It seems you have two functions parseElementValue, defined in different namespaces. That could get confusing.

[Brett208]

Thank you for the review. My goal was to keep the function names the same. I reasoned it might be possible to use a table to match argument names to locations in the structure to remove the if / else logic statements to place each value. I thought doing this would be easier if function overloads / templates were used. It gets complicated because of the detailed knowledge required about the enum to parse it properly, so the local module will need to define overloads for which enums it wants to support parsing. If this seems silly, I'm OK if the enum parse functions have custom names...

I think it might be worth trying to limit enums when possible when parsing to simplify. We could just represent hostility as a scale of 1 to 10 using an int value instead of 'low' 'medium' 'high' for example which would cut out the custom parsing logic.

[DanRStevens]

The problem is the different attributes have different types, and selecting which function overload or template instantiation to call is a compile time thing. It's not compatible with a runtime loop. Trying to use a table would require some kind of compile time iteration.

One of the things I was thinking about was being able to map an enum type to the lookup table used to map strings to enum values. It is possible to use templates to map types to values:
https://gpfault.net/posts/mapping-types-to-values.txt.html

I was thinking if we used such a solution, we could update the stringToEnum function to not require the first parameter:

template<typename T>
T stringToEnum(const std::unordered_map<std::string, T>& table, std::string value)

Instead, the table could be looked up internally using such a mapping template on the template parameter.

Taking that idea further, it could be combined with the parseElementValue function:

template<typename T, std::enable_if_t<!std::is_enum<T>::value, bool> = true>
void parseElementValue(T& destination, const NAS2D::Xml::XmlElement* element)

You could potentially remove the std::enable_if guard, and instead use a if constexpr to select whether to call parseElementValue or stringTo. And you might as well, since there would no longer be an external table parameter to pass in, so it would be very easy to call in such a context.

Would then make it easy to use a return value, rather than an out parameter.

Of course, that all hinges on being able to map a type to a value. I gave it a bit of a try, though if you define the type mapping in a different namespace than it's used, that seems to cause issues.

---

// Specialize this for each EnumType with a lookup table (not shown)
template <typename EnumType>
struct EnumValueName { static const std::unordered_map<std::string, EnumType> table; };

template<typename EnumType>
T stringToEnum(std::string value)
{
    const auto& table = EnumValueName<EnumType>::table;
    // Old implementation ...
}

template<typename T>
T parseElementValue(const NAS2D::Xml::XmlElement* element)
{
    try 
    {
        const auto value = element->getText();
        if constexpr (std::is_enum_v<T>)
        {
            return stringToEnum<T>(value);
        }
        else
        {
            return NAS2D::stringTo<T>(value);
        }
    }
    // Old implementation ...
}

[DanRStevens]

Yeah, maybe I'll look into that as a post-merge thing.