Feature/expand structure icon tool tip #1435
Conversation
|
I was getting error Sprite::play called on undefined action: construction (structures/air_shaft.sprite). It was being caused by
It would happen whenever you placed your cursor over the Robominer in the FactoryProduction Panel. Which mIconItemList[mHighlightIndex].name produces "RoboMiner" vs "Robominer" defeating
|
OPHD/UI/IconGrid.cpp
Outdated
| const auto& tempStructure = Structure::Structure(Structure::StructureClass::Undefined, (StructureID)mIconItemList[mHighlightIndex].meta); | ||
| const auto& popNeeds = tempStructure.populationRequirements(); | ||
| const auto& cost = StructureCatalogue::costToBuild((StructureID)mIconItemList[mHighlightIndex].meta); |
There was a problem hiding this comment.
Similar to the last comment, this might be easier to lookup when setting the grid data, rather than during display.
oscar139
force-pushed
the
Feature/Expand-Structure-Icon-Tool-Tip
branch
from
76b2d10
to
f1ab62a
Compare
| for(auto& resourceCost: resourceCostIterator->second) | ||
| { | ||
| toolTipBoxWidth = std::max(toolTipBoxWidth, mFont.width(std::get<0>(resourceCost) + ": " + std::to_string(std::get<1>(resourceCost)))); | ||
| } |
There was a problem hiding this comment.
You can potentially use structured bindings to unpack the tuple in the ranged-for, without having to use std::get<index>.
for(const auto& [resourceName, resourceCost] : resourceCostIterator->second)
{
toolTipBoxWidth = std::max(toolTipBoxWidth, mFont.width(resourceName + ": " + std::to_string(resourceCost)));
}| for(auto& resourceCost: resourceCostIterator->second) | ||
| { | ||
| renderer.drawText(mFont, std::get<0>(resourceCost) + ": " + std::to_string(std::get<1>(resourceCost)), position + NAS2D::Vector{2, startOffset + (-15 + mFont.height() * currentLine)}, NAS2D::Color::Black); | ||
| currentLine++; | ||
| } |
There was a problem hiding this comment.
Same note about structured bindings here.
Additionally, the std::string here is being created again, even though it matches the previously generated value. That doubles up on both the dynamic memory allocation, and the string concatenation. Granted it would be awkward to somehow re-use the previously generated value with the current structure.
More thoughts on this below.
| IconItemList mIconItemList; /**< List of items. */ | ||
| std::map<std::string, std::vector<std::tuple<std::string, int>>> mResourceCosts; /**< List of resources required to build the item. */ |
There was a problem hiding this comment.
This is an interesting and slightly surprising data structure to accomplish the given task. I can see how this would work, and be fairly general. It also seems a bit heavy in some ways.
The std::vector portion contains multiple std::string, and those's strings are likely to be identical for every std::vector instance, yet there is no sharing. Each std::string is a separate copy taking up additional memory.
On the other hand, by storing the std::string and the int separately means we need to dynamically generate concatenated strings during both sizing and display. If the full string was generated up front, that would eliminate the need for dynamic creation during display. Additionally, the string sizes could potentially be calculated up front as well, so they wouldn't need to be recalculated every frame.
As a further minor note, although there would be just as many strings as before, the combination of the resource name and cost are more likely to be unique, so in that sense there would be less duplication. There's no savings here, rather, there's more justification for the current consumption.
One downside to generating the strings early, is if you wanted to format the resource name differently from the resource cost, it would then be harder to do. Perhaps the names and cost would be in a different color, or font size, or have different alignment.
As for why I found the solution surprising, I think I expected the entries of IconItemList to contain the extra data, rather than a separate lookup structure. That would simplify the looping, since all data would be stored together in the same structure, so there would be no need for an additional lookup, and no need for error checking against a failed lookup.
Perhaps the other surprising bit was the custom data structure using only standard library types. I was perhaps expecting StorableResources and PopulationRequirements as the types for additional fields. This idea of course being more natural if they are added to IconItemList, rather than the std::map structure.
Even if the data is stored as separate components (and dynamically generated during display, rather than storing a pre-generated std::string), the sizing can still be done up front. The needed size would be known as soon as the IconItemList field is set/updated. Here I'm kind of assuming it would be set up front, and not really modified after. I haven't really checked the source for that point. It's probably not actually written with that in mind.
Actually, I'm wondering about the design having an addItem function. Maybe there should only be the option to construct IconGrid with all items at once. It's harder to optimize these things when the data is mutable, and in many cases, mutability isn't actually needed. The game knows early on what all the options and costs are, with changes not being common.
Expanded the Structure icon tool tip to now include the cost to build it and the workers and scientist needed to operate it.
I don't know if this is a too ad hoc solution but I made a temporary Structure Object to gain access to the population requirements.
const auto& tempStructure = Structure::Structure(Structure::StructureClass::Undefined, (StructureID)mIconItemList[mHighlightIndex].meta);const auto& popNeeds = tempStructure.populationRequirements();I would also like to direct your attention to the HEADS UP comment. Again this feels ad hoc.