Move the Dwarvish Miner unit to core #7324
Conversation
This also changes the tracking of what they're carrying to be a status instead of a role, which should be more portable to other use-cases.
github-actions
bot
added
Campaign (any)
|
Was the Miner's stats changed? It's not obvious from the diff. Edit- |
Yes, -1 MP when checking the file. |
|
The diff shows -1 MP too. I'm not sure if I actually made that change or if something else happened, for example:
|
|
It looks like the SoF miner has 4 movement while the SotA miner has 5 movement. |
|
Yeah, so that's probably what happened then – I copied the SoF one. |
|
@nemaara is the one movement difference an issue? |
|
Need to replay SotA to see, it's one of the campaigns I'm not quite as familiar with so I kinda forgor. |
SotA could just add the quick trait or inherit the unit like it is done with the bat line to fix the standing animation underneath the ship rigging. |
|
In case of SotA, I dont think 5 MP Dwarf Miner is required. The PR is just fine as its own. |
|
Yeah, even if it is required they can just be given the quick trait… but we still need to confirm if that's the case and give the trait if so. |
| [or] | ||
| role=has_gold | ||
| [/or] | ||
| status=has_coal,has_gold |
There was a problem hiding this comment.
This seems to only support two types of resource, and will need some workaround if a scenario has miners picking up crystal (#7378). That workaround might be as simple as "we use has_coal to mean crystal", but is there a nicer way to incorporate it into the unit?
There was a problem hiding this comment.
I was thinking about this too a little while ago. As it is right now I don't think there's a reason to call the status "has_xxx" instead of "loaded" or similar; however, I believe I chose this with the thought that the two statuses might have different graphics at some point in the future. But I'm not sure if there's a nice way to handle this generically in a way that would support other resources…
There was a problem hiding this comment.
maybe add a generalized "has_fullbag"?
There was a problem hiding this comment.
That falls under this:
"loaded" or similar
On the other hand, adding it as an exclusive to the existing two seems kind of pointless, as it doesn't actually help someone who wants them to be loaded with a different resource if the sprites actually reflect the resource being carried.
|
Can this be merged? |
|
Yeehaw go ahead. |
This allows addons to safely use it without having to copy the files into their own addon directory.
Also changes the tracking of what they're carrying to be a status instead of a role, which should be more portable to other use-cases.