Page MenuHomeWildfire Games

rename some structure icons
Needs ReviewPublic

Authored by Nescio on Wed, Jan 8, 11:43 PM.

Details

Reviewers
Stan
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Summary

This patch renames nine structure icons for consistency and updates the templates that use them.

  • celtic_embassy.pngembassy_celtic.png
  • iberian_embassy.pngembassy_iberian.png
  • italian_embassy.pngembassy_italic.png
  • palace.pngpers_palace.png (because it's an explicitly Persian symbol)
  • pers_stable.pngstable.png (because it's also used for non-Persian stables)
  • defense_tower.pngtower_large.png
  • sentry_tower.pngtower_small.png
  • gate.pngwall_gate.png
  • tower.pngwall_tower.png
Test Plan

Check for mistakes and omissions.

Event Timeline

Nescio created this revision.Wed, Jan 8, 11:43 PM
Owners added a subscriber: Restricted Owners Package.Wed, Jan 8, 11:43 PM

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/986/display/redirect

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

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/82/display/redirect

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

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

Nescio edited the summary of this revision. (Show Details)Tue, Jan 14, 8:06 PM
Stan added a subscriber: Stan.Wed, Jan 15, 3:05 PM

The rest look fine, but I'm not sure about the tower changes. They are indeed small medium big wall artillery. Is there something wrong with wood scout sentry defense wall?

The rest look fine, but I'm not sure about the tower changes. They are indeed small medium big wall artillery. Is there something wrong with wood scout sentry defense wall?

Right now we have the following tower icon names:

defense_tower.png
sentry_tower.png
tower_artillery.png
tower_bolt.png

which is inconsistent. Because they're all towers and to allow for future additions, it makes sense to follow the naming of the recently added files (tower_*.png).
As for the two older towers, they use different (art) file names to refer to the same thing, which is rather confusing (see also D2552):

  • wood(en) tower, sentry tower, and outpost stone for the small tower
  • scout tower, defense tower, and stone tower for the large tower.

I recommend switching to “small” and “large”, because those are clear, meaningful, descriptive terms.

Nescio updated this revision to Diff 11032.Wed, Jan 15, 3:39 PM
Nescio edited the summary of this revision. (Show Details)

Two more:

  • gatewall_gate.png
  • towerwall_tower.png

Because they're wall segments first and foremost.

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/1049/display/redirect

Nescio edited the summary of this revision. (Show Details)Wed, Jan 15, 3:41 PM

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

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/145/display/redirect

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

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

Stan added a subscriber: LordGood.Wed, Jan 15, 3:51 PM

wooden tower were originally wooden before @LordGood made them pretty. I assume they should have been renamed then. You can still find the original wooden actor.
embassy_italic like strings are always a bit problematic because technically it could be rome_embassy, iber_embassy, gaul/brit_embassy
I think hanging_gardens should be deleted if it is unused. The icon looks ugly.

Nescio added a comment.EditedWed, Jan 15, 3:56 PM

technically it could be rome_embassy, iber_embassy, gaul/brit_embassy

Except that they are actually used by cart civ structures.
[EDIT] If they do not belong specifically to one civ (i.e. they could conceivably be used by others as well), then the name shouldn't start with {civ}_; cf. pers icons.

I think hanging_gardens should be deleted if it is unused. The icon looks ugly.

It is unused. Should I delete it here or exclude it from this patch so you can delete it separately? There are some other unused icons as well.

Stan added a comment.Wed, Jan 15, 3:59 PM

Except that they actually belong to the cart civ.

Maybe they should be generic like the other mercenary camps. But that's balancing :/

It is unused. Should I delete it here or exclude it from this patch so you can delete it separately? There are some other unused icons as well.

I'll delete it unless you want to make a separate patch for all the icons. Up to you.

Maybe they should be generic like the other mercenary camps.

What do you mean? Other buildable mercenary-training structures (athen/spart stoa, kush camps, pers hall) aren't generic either.

I'll delete it unless you want to make a separate patch for all the icons. Up to you.

Go ahead then.

For your information, these six structure icons appear to be unused, at least in the public simulation folder:

colony.png
gardens.png
ishtar_gate.png
lamassu.png
monument.png
shipyard.png
Stan added a comment.Wed, Jan 15, 4:30 PM

For your information, these six structure icons appear to be unused, at least in the public simulation folder:

colony.png
gardens.png
ishtar_gate.png
lamassu.png
monument.png
shipyard.png

Checkrefs.pl would tell you.

Checkrefs.pl would tell you.

Undoubtedly it would, but grep works for me, that perl script doesn't:

[b@p50 entity]$ perl checkrefs.pl --check-unused
Can't locate JSON.pm in @INC (you may need to install the JSON module) (@INC contains: /usr/local/lib64/perl5/5.30 /usr/local/share/perl5/5.30 /usr/lib64/perl5/vendor_perl /usr/share/perl5/vendor_perl /usr/lib64/perl5 /usr/share/perl5) at checkrefs.pl line 6.
BEGIN failed--compilation aborted at checkrefs.pl line 6.

And yes, I've installed about every perl package (sudo dnf install perl*) available.

Stan added a comment.Wed, Jan 15, 4:52 PM

apt-get install libjson-perl ? :) At least it's not the same error. Attached at P194 is a list of the current unused file. Ideally there would be close to none.

Nescio updated this revision to Diff 11033.Wed, Jan 15, 4:56 PM
Nescio edited the summary of this revision. (Show Details)

now without gardens

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/1050/display/redirect

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

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/146/display/redirect

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

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

apt-get install libjson-perl ?

Different Linux distributions have different package managers and names. That one is Debian-based. I'm using Fedora, so I'm unsure how the missing package(s) are called on my end.

Stan added a comment.Wed, Jan 15, 5:10 PM

IIRC for perl you can also install something called CPAN

Don't ask me why, but somehow dnf remove perl (and not re-installing it) solved the errors, so now the script works for me.

Stan added a comment.Wed, Jan 15, 5:31 PM

Don't ask me why, but somehow dnf remove perl (and not re-installing it) solved the errors, so now the script works for me.

Nice!

Nescio added a reviewer: Stan.Mon, Jan 20, 8:10 PM