Page MenuHomeWildfire Games

delete duplicate and deprecated unit icons
ClosedPublic

Authored by Nescio on Mon, Jan 20, 4:48 PM.

Details

Reviewers
Stan
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP23422: Delete duplicate and deprecated unit icons.
Summary

This patch is a follow-up to D985/rP20677, rP22748, and D2582/rP23421.

  • It deletes unused icons that have been deprecated.
  • It deletes icons that were duplicates
  • It moves hele icons that are only used by athen
  • It renames several others for consistency
  • It replaces Themistocles's new icon with a very old portrait (have a look at them).
  • It updates the affected templates accordingly (checked with perl checkrefs.pl --validate_templates)

Similar: D2551

Test Plan

Check that the icons are actually moved or deleted.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Nescio created this revision.Mon, Jan 20, 4:48 PM
Owners added a subscriber: Restricted Owners Package.Mon, Jan 20, 4:48 PM
Nescio added inline comments.Mon, Jan 20, 4:51 PM
binaries/data/mods/public/simulation/templates/units/athen_infantry_marine_archer_b.xml
25 ↗(On Diff #11122)

Yes, mace.

binaries/data/mods/public/simulation/templates/units/brit_hero_cunobelin.xml
9 ↗(On Diff #11122)

Because it's an infantry icon.

binaries/data/mods/public/simulation/templates/units/brit_infantry_slinger_b.xml
15 ↗(On Diff #11122)

Yes, gaul.

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1617/display/redirect

Stan added a comment.Mon, Jan 20, 5:09 PM

For the ones using an incorrect civ, wouldn't it make more sense to duplicate the icon to have both brit and celt, mace and athen etc?

For the ones using an incorrect civ, wouldn't it make more sense to duplicate the icon to have both brit and celt, mace and athen etc?

Maybe, though that would mean having two exactly identical images. Wasn't it your intention to try reducing the download size?

Nescio edited the summary of this revision. (Show Details)Mon, Jan 20, 5:17 PM
Nescio added inline comments.
binaries/data/mods/public/simulation/templates/units/ptol_infantry_archer_b.xml
23 ↗(On Diff #11122)

Yes, mace. See line 30.

Stan added a comment.Mon, Jan 20, 5:21 PM

For the ones using an incorrect civ, wouldn't it make more sense to duplicate the icon to have both brit and celt, mace and athen etc?

Maybe, though that would mean having two exactly identical images. Wasn't it your intention to try reducing the download size?

Only by removing unused things. We might want to check if delenda_est doesn't have an updated icon too.

Keep in mind many ship, siege, and support units currently share a single icon for templates of different civs.

Stan added a comment.Mon, Jan 20, 5:27 PM

Keep in mind many ship, siege, and support units currently share a single icon for templates of different civs.

True. I was suggesting that because we are changing it :)

Personally I think icons ought to match the actor. The ptol archer uses the mace actor, which looks the same as the athen actor, so it would make sense for all three to use the same icon.
Both brit and gaul slinger deserve a new icon, I believe their actors were separated and updated last year.

Stan accepted this revision.Mon, Jan 20, 6:22 PM

Looks good to me :) Thanks for the patch.

This revision is now accepted and ready to land.Mon, Jan 20, 6:22 PM
This revision was automatically updated to reflect the committed changes.