[Please Test] Tags

[Omega]

So, tags have been implemented, but I'm trying to improve the change so that in the list of affected units, you'll just see the tag name instead of the numerous units that have that tag. Thus, we have a set of units affected by the upgrade and a set of tags that are affected by the upgrades. Unfortunately, I'm faced with a strange bug that I'm not able to work around.

It's summed up in this StackOverflow post, but someone here may have a better idea of what could be wrong. For context, the changes up till this are on GitHub.

I don't see anything that should have gone out of scope and there's no undefined behavior that I'm aware of (unfortunately, the nature of undefined behavior in C++ makes it difficult to be sure). Compiler isn't warning me about anything. I haven't had any success with a debugger, mostly due to not being used to debugging C++ (the STL classes are almost unreadable).

Any help would be appreciated.

[MoLAoS]

I don't think you provided enough examples to find an error, but why not just use vectors or for_each? Its not like you are calling the code enough to be a performance issue.

[Omega]

I used sets to simplify the handling of duplicates (no need to manually check for duplicates -- sets ignore those). The original implementation found all unit types with a tag and threw them in the affects vector. Now that tags are separate from the affects, I guess it's not so needed. Still a strange issue, though.

Regarding `for_each`, I've never really cared for it. I'd love to use the C++11's range based for loops, but they require MSVS 2012 or later and I'm not sure if that's supported, yet. Although I'd have to check with Softcoder regarding the use of newer language features, anyway. Wouldn't want to break the build on some platform.

At any rate, the solution I used was a conditional to check if the set of unit tags is empty. Although I still don't understand why the inner for loop was even entered. The issue boiled down to `tags.empty()` being true while `tags.begin() != tags.end()` is also true (but it shouldn't be!).

[Yggdrasil]

As someone more or less mentioned on stackoverflow: Change getTags() to return a reference, not a value/copy.

Code Select Expand


const set<string> &getTags() const {return tags;}

Be aware that the return type is const, so use a const iterator.

Not sure if that's all, but you don't want a (deep) copy there, for sure. The iterator gets out of bounds because you check against the end of a different set. Every call to getTags() gets its own copy.

[Omega]

Hey Yggdrasil, long time, no see. Anyway, thank you for your reply. That worked perfectly. While I knew about passing by reference and value, I did not know that return types had similar semantics. The code is now working as expected.

Anyway, now that that's working as expected, I just a have a few things related to tags that I need to finish up. I need to verify that saving and loading the game works as expected, I want to refactor AttackBoost::isAffected (it has a horrible amount of code clones), and I want to make tags translatable (in the same manner as the rest of the techtrees).

EDIT: This makes me realize that there's a number of other places where we return a copy to a vector instead of a reference. They haven't been caught because they didn't use iterators like I did, but pose a threat creating of future bugs. Might be worth taking the time to convert these.

[MoLAoS]

Hey Yggdrasil, long time, no see. Anyway, thank you for your reply. That worked perfectly. While I knew about passing by reference and value, I did not know that return types had similar semantics. The code is now working as expected.

Anyway, now that that's working as expected, I just a have a few things related to tags that I need to finish up. I need to verify that saving and loading the game works as expected, I want to refactor AttackBoost::isAffected (it has a horrible amount of code clones), and I want to make tags translatable (in the same manner as the rest of the techtrees).

EDIT: This makes me realize that there's a number of other places where we return a copy to a vector instead of a reference. They haven't been caught because they didn't use iterators like I did, but pose a threat creating of future bugs. Might be worth taking the time to convert these.

How does saving work in MegaGlest? I spent a lot of time modifying GAE's save and I wonder if GAE and MG do it in the same way.

[Omega]

Done! Tags are working as expected. Rather than repeat myself, please see the pull request here. It details everything you need to know about tags, including syntax and translating.

How does saving work in MegaGlest? I spent a lot of time modifying GAE's save and I wonder if GAE and MG do it in the same way.

It dumps pretty much everything you'd need to know about the game state (eg, list of all units, stats that they have at a given time, etc) into an XML. When loading the game, that XML is read to determine what to load (eg, what units to load, their stats, etc). It largely works in a hierarchy, having factions call load methods of units, units call load methods of attack boosts, etc. The code is very messy, though. There's a great deal of commented out code. I think that's mostly because who ever implemented the save/load feature realized they don't have to save everything. Upgrade types aren't saved or loaded, for example, since they can't be changed in game, so can be loaded from the faction (although the upgrade manager and its list of in-progress/completed upgrades is saved/loaded).

EDIT: For the convenience of who ever has to merge this change, I added additional comments to explain design choices and places that aren't very clear in the diff. These are (as far as I know) only visible on GitHub.

EDIT2: For further convenience, I created a pull request for the megaglest-data change (just a single line in a language file).

[titi]

This is merged into branch develop now. please test!

[Carl the Great]

I assume this feature has not been tested for a long time. I want to try this, but where's the code?

I have read this entire thread already, but still no code. Also, watch the language.

[titi]

Did you see this too? https://github.com/MegaGlest/megaglest-source/pull/25

btw  @Ishmaru: die you  use this yet ?

[Carl the Great]

Sorry, I didn't click all the links in this thread. I had to skim through this thread looking for the code.

I'll place the code here so I don't need to click any links again.

"Tags are assigned with the  <tags>  element, which is a child of  <parameters>  in the unit XML. Units can have any number of tags. Tags are merely strings. Note that tags are case sensitive. The  <tags>  element is optional and may be empty."

"Tags should be restricted to letters, numbers, and underscores. As with units, underscores will be converted to spaces and the first letter of each word capitalized."

Code Select Expand

<parameters>
    <!-- ... -->
    <tags>
        <tag value="warrior"/>
        <tag value="burnable"/>
    </tags>
    <!-- ... -->
</parameters>

> Applying upgrades to tagged units:
"Tags can be added to the upgrade XML's existing  <effects>  element. They can be mixed with existing  <unit>  elements. The upgrade will thus be applied to all units in the player's faction which have that tag."

Code Select Expand

<upgrade>
    <!-- ... -->
    <effects>
        <unit name="roundtent"/>
        <tag name="test" />
        <unit name="archer"/>
        <tag name="warrior" />
    </effects>
    <!-- ... -->
</upgrade>

> Applying attack boosts to tagged units:

"Tags can also be added to the target of attack boosts. The syntax is similar to that of upgrades. The target value (foe, ally, all, etc) will be obeyed."

Code Select Expand

<skill>
    <!-- ... -->
    <attack-boost>
        <allow-multiple-boosts value="false" />
        <radius value="5" />
        <target value="all" include-self="false">
            <unit-type name="archer" />
            <tag name="warrior" />
        </target>
        <!-- ... -->
    </attack-boost>
    <!-- ... -->
</skill>

> Translating tags

Placed in the .lang file of the techtree. Has the source code been already implemented?

Code Select Expand

TagName_warrior=Guerrier

[titi]

Can you establish that all this works in Megaglest 3.12.0 ?

Do you have an idea where this should be added in the wiki ? ( https://docs.megaglest.org/XMLs)

[Ishmaru]

Tags seem to work fine 3.12.0  :)