Xml improvements #831
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,13 @@ | ||
| #include "Planet.h" | ||
|
|
||
| #include "../Constants.h" | ||
| #include "../XmlSerializer.h" | ||
|
|
||
| #include <NAS2D/Utility.h> | ||
| #include <NAS2D/EventHandler.h> | ||
| #include <NAS2D/Renderer/Renderer.h> | ||
| #include <NAS2D/Renderer/Rectangle.h> | ||
| #include <NAS2D/Xml/Xml.h> | ||
|
|
||
| #include <stdexcept> | ||
|
|
||
|
|
@@ -66,3 +68,118 @@ void Planet::update() | |
| const auto spriteFrameOffset = NAS2D::Point{mTick % 8 * PlanetSize.x, ((mTick % 64) / 8) * PlanetSize.y}; | ||
| renderer.drawSubImage(mImage, mPosition, NAS2D::Rectangle<int>::Create(spriteFrameOffset, PlanetSize)); | ||
| } | ||
|
|
||
| namespace | ||
| { | ||
| Planet::Attributes parsePlanet(const NAS2D::Xml::XmlElement* xmlNode); | ||
|
|
||
| void parseElementValue(Planet::PlanetType& destination, const NAS2D::Xml::XmlElement* element); | ||
| void parseElementValue(Planet::Hostility& destination, const NAS2D::Xml::XmlElement* element); | ||
|
|
||
| const std::unordered_map<std::string, Planet::PlanetType> planetTypeTable | ||
| { | ||
| {"None", Planet::PlanetType::None}, | ||
| {"Mercury", Planet::PlanetType::Mercury}, | ||
| {"Mars", Planet::PlanetType::Mars}, | ||
| {"Ganymede", Planet::PlanetType::Ganymede} | ||
| }; | ||
|
|
||
| const std::unordered_map<std::string, Planet::Hostility> hostilityTable | ||
| { | ||
| {"None", Planet::Hostility::None}, | ||
| {"Low", Planet::Hostility::Low}, | ||
| {"Medium", Planet::Hostility::Medium}, | ||
| {"High", Planet::Hostility::High} | ||
| }; | ||
|
|
||
|
|
||
| void parseElementValue(Planet::PlanetType& destination, const NAS2D::Xml::XmlElement* element) | ||
| { | ||
| destination = stringToEnum(planetTypeTable, element->getText()); | ||
| } | ||
|
|
||
|
|
||
| void parseElementValue(Planet::Hostility& destination, const NAS2D::Xml::XmlElement* element) | ||
| { | ||
| destination = stringToEnum(hostilityTable, element->getText()); | ||
| } | ||
| } | ||
|
|
||
|
|
||
| std::vector<Planet::Attributes> parsePlanetAttributes() | ||
| { | ||
| const std::string rootElementName("Planets"); | ||
| auto xmlDocument = openXmlFile("planets/PlanetAttributes.xml", rootElementName); | ||
|
|
||
| std::vector<Planet::Attributes> planetAttributes; | ||
|
|
||
| auto rootElement = xmlDocument.firstChildElement(rootElementName); | ||
| for (const auto* node = rootElement->iterateChildren(nullptr); | ||
| node != nullptr; | ||
| node = rootElement->iterateChildren(node)) | ||
| { | ||
| std::string elementName("Planet"); | ||
| if (node->value() != elementName) | ||
| { | ||
| throw std::runtime_error(xmlDocument.value() + " missing " + elementName + " tag"); | ||
| } | ||
| planetAttributes.push_back(parsePlanet(node->toElement())); | ||
| } | ||
|
|
||
| return planetAttributes; | ||
| } | ||
|
|
||
|
|
||
| namespace | ||
| { | ||
| Planet::Attributes parsePlanet(const NAS2D::Xml::XmlElement* xmlNode) | ||
| { | ||
| Planet::Attributes attributes; | ||
|
|
||
| for (const auto* node = xmlNode->iterateChildren(nullptr); | ||
| node != nullptr; | ||
| node = xmlNode->iterateChildren(node)) | ||
| { | ||
| const auto* element = node->toElement(); | ||
|
|
||
| if (element->value() == "PlanetType") | ||
| { | ||
| parseElementValue(attributes.type, element); | ||
| } | ||
| else if (element->value() == "ImagePath") | ||
| { | ||
| ::parseElementValue(attributes.imagePath, element); | ||
| } | ||
|
Comment on lines
+145
to
+152
There was a problem hiding this comment. Hmm, yes, I see the discrepancy with the There was a problem hiding this comment. 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. There was a problem hiding this comment. 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: I was thinking if we used such a solution, we could update the 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 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 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 ...
} |
||
| else if (element->value() == "Hostility") | ||
| { | ||
| parseElementValue(attributes.hostility, element); | ||
| } | ||
| else if (element->value() == "MaxDepth") | ||
| { | ||
| ::parseElementValue(attributes.maxDepth, element); | ||
| } | ||
| else if (element->value() == "MaxMines") | ||
| { | ||
| ::parseElementValue(attributes.maxMines, element); | ||
| } | ||
| else if (element->value() == "MapImagePath") | ||
| { | ||
| ::parseElementValue(attributes.mapImagePath, element); | ||
| } | ||
| else if (element->value() == "TilesetPath") | ||
| { | ||
| ::parseElementValue(attributes.tilesetPath, element); | ||
| } | ||
| else if (element->value() == "Name") | ||
| { | ||
| ::parseElementValue(attributes.name, element); | ||
| } | ||
| else if (element->value() == "MeanSolarDistance") | ||
| { | ||
| ::parseElementValue(attributes.meanSolarDistance, element); | ||
| } | ||
| } | ||
|
|
||
| return attributes; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| #include "XmlSerializer.h" | ||
| #include <NAS2D/Utility.h> | ||
| #include <NAS2D/Filesystem.h> | ||
|
|
||
| using namespace NAS2D; | ||
|
|
||
|
|
||
| Xml::XmlDocument openXmlFile(std::string filename, std::string rootElementName) | ||
| { | ||
| Xml::XmlDocument xmlDocument; | ||
| xmlDocument.parse(Utility<Filesystem>::get().open(filename).raw_bytes()); | ||
|
|
||
| if (xmlDocument.error()) | ||
| { | ||
| throw std::runtime_error(filename + " has malformed XML: Row: " + std::to_string(xmlDocument.errorRow()) + | ||
| " Column: " + std::to_string(xmlDocument.errorCol()) + " : " + xmlDocument.errorDesc()); | ||
| } | ||
|
|
||
| const auto* xmlRootElement = xmlDocument.firstChildElement(rootElementName); | ||
| if (!xmlRootElement) { | ||
| throw std::runtime_error(filename + " does not contain required root tag of <" + rootElementName + ">"); | ||
| } | ||
|
|
||
| return xmlDocument; | ||
DanRStevens marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.