Page MenuHomeWildfire Games

rename some structure icons
Needs ReviewPublic

Authored by Nescio on Jan 8 2020, 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.Jan 8 2020, 11:43 PM
Owners added a subscriber: Restricted Owners Package.Jan 8 2020, 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)Jan 14 2020, 8:06 PM
Stan added a subscriber: Stan.Jan 15 2020, 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.Jan 15 2020, 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)Jan 15 2020, 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.Jan 15 2020, 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.EditedJan 15 2020, 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.Jan 15 2020, 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.Jan 15 2020, 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.Jan 15 2020, 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.Jan 15 2020, 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.Jan 15 2020, 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.Jan 15 2020, 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.Jan 20 2020, 8:10 PM
Nescio added a comment.EditedJan 26 2020, 6:51 PM

Anything that needs to be changed, @Stan?

Stan added a comment.Sun, Mar 8, 12:12 PM

Still not sure about tower_small medium large, I guess it's consistent with walls, and also we don't have any wooden towers like we did in the past, yet I haven't quite made up my mind on whether it's an improvement. The rest is fine I guess.

The current situation:

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

is not really consistent. The proposal is:

tower_artillery.png
tower_bolt.png
tower_large.png
tower_small.png
wall_tower.png
Stan added a comment.Sun, Mar 8, 12:51 PM

The current situation:

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

is not really consistent. The proposal is:

tower_artillery.png
tower_bolt.png
tower_large.png
tower_small.png
wall_tower.png

Yeah I understand. Maybe include that change in that patch and leave the rest here.