Page MenuHomeWildfire Games

+ shaped healer selection marker; more 2D art
ClosedPublic

Authored by Nescio on Sep 8 2017, 12:53 PM.

Details

Reviewers
wowgetoffyourcellphone
elexis
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Owners Package(Owns No Changed Paths)
Commits
rP20374: New unit selection textures for the healer and mods/future usage.
Summary

Added three more selection markers (plus, rhombus, and square shapes), and applied the plus (+) shaped selection marker to the healer, to distinguish it from other units, and to fit its aura range.

The images are my own work; I hereby release them under CC BY-SA 3.0.

For comparison, heroes (star) and champions (arrow) also have different selection markers (unchanged).

Update:

  • Two more selection marker shapes: {8/2} octagram, hash
  • Added new chevron icons for ranks 0 to 12

None of these new images is used, however, they are potential useful in the future or for mods.

Also considered many other selection marker shapes, but rejected them either because they're too similar and approach the circle (e.g. {5/1} pentagon, {6/1} hexagon, {7/1} heptagon, {8/1} octagon, etc), or because they are beyond my limited drawing skills (e.g. regular {12/5} dodecagram; narrow, wide, and regular {8/3} octagrams; regular {3/1} triangle; etc.).

Test Plan

Little to test

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 3082
Build 5328: Vulcan BuildJenkins
Build 5327: arc lint + arc unit

Event Timeline

Nescio created this revision.Sep 8 2017, 12:53 PM
Owners added a subscriber: Restricted Owners Package.Sep 8 2017, 12:53 PM
Stan added a subscriber: Stan.Sep 8 2017, 1:45 PM

Where did you get those images from ? (They need to have a compatible license)

Nescio added a comment.Sep 8 2017, 2:02 PM

Those I made myself; where and which license do they need?

Nescio edited the summary of this revision. (Show Details)Sep 8 2017, 2:13 PM
elexis added a subscriber: elexis.Sep 8 2017, 2:15 PM

Can you upload some screenshots?

Nescio added a comment.Sep 8 2017, 2:57 PM

OK, then I release these images under the Creative Commons Attribution-Share Alike 3.0 Unported license. (Although I think it's a bit ridiculous to claim copyright on simply geometric shapes.)

And here is a screenshot:

Nescio edited the summary of this revision. (Show Details)Sep 8 2017, 2:58 PM
elexis added a subscriber: wowgetoffyourcellphone.

I was dubious about these (I certainly don't think that there must be a consistency between the aura range visualization and the selection icon),
but on the screenshot it looks nice actually.

Having some other selection icons to pull out of a hat might come in handy some time when adding more special units (we could have used one when relics were added)

What would @wowgetoffyourcellphone say?

Nescio updated this revision to Diff 3582.Sep 8 2017, 5:15 PM

credits (as requested by bb)

Owners added a subscriber: Restricted Owners Package.Sep 8 2017, 5:15 PM
Vulcan added a subscriber: Vulcan.Sep 8 2017, 7:18 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jenkins-master:8080/job/phabricator/1996/ for more details.

Nescio updated this revision to Diff 3635.Sep 11 2017, 2:52 PM
Nescio retitled this revision from + shaped healer selection marker to + shaped healer selection marker; more 2D art.
Nescio edited the summary of this revision. (Show Details)
Nescio edited the test plan for this revision. (Show Details)

Included two more selection marker shapes (hash, octagram), and more rank chevrons

Nescio edited the summary of this revision. (Show Details)Sep 11 2017, 3:14 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jenkins-master:8080/job/phabricator/2016/ for more details.

Tried the patch. The healer's changed selection is nice. It works because it is a totally different class of unit from the rest, so should stand out from the rest as well. Looked good. Approved. Also, the b/a/e chevrons worked as promised after the filename change.

This revision is now accepted and ready to land.Sep 16 2017, 10:39 AM

As for the other shapes, they may be useful in the future. Honestly, I think a selection of rectangles for mechanical units would be nice, specifically siege, perhaps lozenge shaped, rectangles with rounded corners.

Nescio added a comment.EditedSep 16 2017, 2:12 PM

To clarify: if the unit's footprint is oblong (longer than it's wide), then the “circle”, “rhombus”, and “square” actually become an ellipse, lozenge, and rectangle, respectively:


It might be nice in a mod, but I'm not sure it would be a good idea to change the selection marker of siege weapons in the main distribution (because then ships, traders, females, catafalques, etc could also need new shapes).

And a comparison of the new and old available selection marker shapes:

Hi, sorry. This is what I mean when I say "lozenge":

https://maxcdn.icons8.com/Share/icon/ios7/Editing//rounded_rectangle_stroked1600.png

The stretched circle for siege and ships always bothered me. Didn't look "polished" to me. I think a variety of rounded rectangles like that image would be very nice to have for those types of "long and narrow" units. Or perhaps "tubes":

https://i.stack.imgur.com/QN5I9.png

What do you think?

Nescio added a comment.EditedSep 16 2017, 8:49 PM

That is a rounded rectangle. This is a lozenge: ◊.

Personally I think the “stretching” is a very efficient solution which saves us from creating dozens of similar selection marker shapes. However, I do get your point; the line is thicker at the short side and thinner in the middle of the long side; unfortunately this can not be solved by creating another selection marker shape: the game engine always automatically reshapes the selection marker to match the unit's footprint. This is quite noticeable with the circle, but less so with the square (the lengthening to a rectangle does not affect the thickness of the lines) and rhombus. I suppose a rounded square shape could be created, however, then you would still have stretching at the corners, so I'd recommend using the normal “square” instead (admittedly, those corners aren't beautiful either).
What you're looking for is a superellipse.

EDIT: attempted a squircle:


Note: the lines are very thin, because I didn't create an appropiate image, but just borrowed a svg from wikipedia for a quick test: https://upload.wikimedia.org/wikipedia/commons/thumb/5/58/Squircle2.svg/1024px-Squircle2.svg.png

Update: a squircle is too difficult for me to create, but I could do a rounded rectangle:


Please have a critical look at the corners. As you can see it has the same issue as the elongated circle. I would recommend against it, but if you insist, I could add this shape as well.

In D889#35586, @Nescio wrote:

Update: a squircle is too difficult for me to create, but I could do a rounded rectangle:


Please have a critical look at the corners. As you can see it has the same issue as the elongated circle. I would recommend against it, but if you insist, I could add this shape as well.

Indeed, it only looks good as a square. For rectangles it would need custom textures. You don't have to have them for this particular patch. I might play around with it at a later date though.

Unless you create separate shapes for each and every individual footprint, you'll always have issues with imperfect resizing.

Nescio added a comment.Oct 8 2017, 9:17 AM

This diff has been reviewed and accepted over three weeks ago and is “ready to land” since then; when will it be implemented?

elexis added reviewers: Restricted Owners Package, Restricted Owners Package.Oct 8 2017, 5:50 PM
In D889#37123, @Nescio wrote:

This diff has been reviewed and accepted over three weeks ago and is “ready to land” since then; when will it be implemented?

Since you have sent me a PM with the same question, answering here.
There are hundreds of patches disregarded in the review queue and the number increases steadily since 0AD source was published - even with multiple developers having exclusively worked more than full time on the review queue for years.
I personally don't see the reason to continue that behavior, given that none of the users of the product even notice the majority of improvements.
The only thing that gets a patch committed is a team member that stops working on his own feature to review something. That often means repeated begging.
I don't think Michael will use his commit access, so someone else of the team will still have to review this.
His review is still useful to ensure that people who are more focused on visual design also agree with the change.
In contrast to many other patches, this one is easy to review and seems somewhat rewarding though, so seems more probable to be committed.

Nescio added a comment.Oct 8 2017, 8:00 PM

Sorry, I don't intend to be rude, but I fear I don't understand your answer.

Happen to have the images around and could upload them as an archive? (Otherwise have to download them one by one.)

binaries/data/mods/public/gui/session/selection_details.js
509 ↗(On Diff #3635)

D717 will rename these to Basic, Advanced and Elite, so I propose to just leave this diff out and just add the new material here.

Which images do you want as an archive?

I suppose I could also update this patch to exclude the files affected by D717.

In D889#38739, @Nescio wrote:

Which images do you want as an archive?

The ones you want committed.
Would really help if the folders match the textures folder layout, so I don't have to sort things.
About the rank icons, perhaps the filenames should contain the color, as the numbers would seem to indicate an order otherwise.
rank0 seems empty? Would be unneeded I think.

I suppose I could also update this patch to exclude the files affected by D717.

If it's only that one hunk I can revert that, no real need to reup.

Nescio updated this revision to Diff 4007.Oct 29 2017, 1:54 AM
Nescio edited the summary of this revision. (Show Details)

Updated

Build is green

Updating workspaces...
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDAnimation]’:
FCollada/FCDocument/FCDLibrary.cpp:149:30:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
  const T* cptr = ((const FCDLibrary<T>*)l1)->GetEntity(0);
           ^
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDAnimationClip]’:
FCollada/FCDocument/FCDLibrary.cpp:150:34:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDCamera]’:
FCollada/FCDocument/FCDLibrary.cpp:151:27:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDController]’:
FCollada/FCDocument/FCDLibrary.cpp:152:31:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDEffect]’:
FCollada/FCDocument/FCDLibrary.cpp:153:27:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDEmitter]’:
FCollada/FCDocument/FCDLibrary.cpp:154:28:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDForceField]’:
FCollada/FCDocument/FCDLibrary.cpp:155:31:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDGeometry]’:
FCollada/FCDocument/FCDLibrary.cpp:156:29:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDImage]’:
FCollada/FCDocument/FCDLibrary.cpp:157:26:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDLight]’:
FCollada/FCDocument/FCDLibrary.cpp:158:26:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDMaterial]’:
FCollada/FCDocument/FCDLibrary.cpp:159:29:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDSceneNode]’:
FCollada/FCDocument/FCDLibrary.cpp:160:30:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDPhysicsModel]’:
FCollada/FCDocument/FCDLibrary.cpp:161:33:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDPhysicsMaterial]’:
FCollada/FCDocument/FCDLibrary.cpp:162:36:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ s

http://jenkins-master:8080/job/phabricator/2183/ for more details.

I can't apply the patch with Phabricator. Always get 0 byte images.
With archive I meant a zip/rar/tar/7z/... file containing the images, preferably with the correct directory structure, if that isn't too much hassle.

Nescio updated this revision to Diff 4010.Oct 29 2017, 11:14 AM

Updated

Sorry, I don't really understand phabricator, but here is a zip containing all new and changed files:

Build is green

Updating workspaces...
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDAnimation]’:
FCollada/FCDocument/FCDLibrary.cpp:149:30:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
  const T* cptr = ((const FCDLibrary<T>*)l1)->GetEntity(0);
           ^
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDAnimationClip]’:
FCollada/FCDocument/FCDLibrary.cpp:150:34:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDCamera]’:
FCollada/FCDocument/FCDLibrary.cpp:151:27:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDController]’:
FCollada/FCDocument/FCDLibrary.cpp:152:31:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDEffect]’:
FCollada/FCDocument/FCDLibrary.cpp:153:27:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDEmitter]’:
FCollada/FCDocument/FCDLibrary.cpp:154:28:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDForceField]’:
FCollada/FCDocument/FCDLibrary.cpp:155:31:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDGeometry]’:
FCollada/FCDocument/FCDLibrary.cpp:156:29:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDImage]’:
FCollada/FCDocument/FCDLibrary.cpp:157:26:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDLight]’:
FCollada/FCDocument/FCDLibrary.cpp:158:26:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDMaterial]’:
FCollada/FCDocument/FCDLibrary.cpp:159:29:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDSceneNode]’:
FCollada/FCDocument/FCDLibrary.cpp:160:30:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDPhysicsModel]’:
FCollada/FCDocument/FCDLibrary.cpp:161:33:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDPhysicsMaterial]’:
FCollada/FCDocument/FCDLibrary.cpp:162:36:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ s

http://jenkins-master:8080/job/phabricator/2184/ for more details.

elexis accepted this revision.Oct 29 2017, 3:06 PM
In D889#38803, @Nescio wrote:

Sorry, I don't really understand phabricator, but here is a zip containing all new and changed files:

Neither do I. It's still development in progress. Dunno why it bugs with binary files and file moves, maybe I'm using the command line tool arc incorrectly.

Thanks

Thanks for the nice images!

binaries/data/mods/public/art/textures/ui/session/icons/ranks/bronze_1.png
1 ↗(On Diff #4010)

Thanks for the rename, much better than 1 to 12, because I'm not sure if the player understands that N orange arrows are better or worse than M yellow arrows if these rank icons were used in this order.
The ones using these images can determine their interpretation.

binaries/data/mods/public/art/textures/ui/session/icons/ranks/none.png
1 ↗(On Diff #4010)

Excluding this as I don't know what it would be used for. One can remove the sprite by setting it to emptystring afaik.

binaries/data/mods/public/gui/credits/texts/art.json
71

(whitespace and non-alphabetical order)

binaries/data/mods/public/simulation/templates/template_unit_support_healer.xml
34

Ok to keep the hero icon for hero healers, since the hero property is more important.

Stan awarded a token.Oct 29 2017, 3:09 PM
This revision was automatically updated to reflect the committed changes.

@wowgetoffyourcellphone, I'm currently playing around with svg and making more selection shapes, including some with a 1:2 ratio (useful for cavalry and elephants). Which rounded rectangle do you prefer?

or
? Also, here's an ellipse:
.