Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added widget showing the army for players in spec #16709

Merged
merged 3 commits into from Feb 9, 2020

Conversation

@teinarss
Copy link
Contributor

teinarss commented Jun 18, 2019

Closes #10497

Do we want tooltip?
Also we are out of F-keys for the spectate ui

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Jun 20, 2019

Icons are truncated if a player has more than 7 unique units:
Screenshot 2019-06-20 at 22 13 58

Do we want tooltip?

Definitely, yes.

@ghost

This comment has been minimized.

Copy link

ghost commented Jun 20, 2019

Would it be possible to remove/crop the label from the cameo?

Also we are out of F-keys for the spectate ui

We could add it to the army graph tab above or below the graph, and remove the player names that you put in #16702 left of the graph.

@teinarss

This comment has been minimized.

Copy link
Contributor Author

teinarss commented Jun 20, 2019

Is it sufficient with name and description in the tooltip?

@ghost

This comment has been minimized.

Copy link

ghost commented Jun 20, 2019

I would say yes.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Jun 21, 2019

Would it be possible to remove/crop the label from the cameo?

Please ignore/defer this request to a followup. For one, cropping the bottom causes graphic errors on the chisel edge of all cameos. For two, TS cameos are already untexted so any cropping would just break them instead. For three, there are no cropping on production and support power cameos either and so far it wasn't a problem.

If this is indeed a problem, the best course of action is to expose the cameos used for this in YAML and start using the lowres and/or include untexted variants in TD/RA.

@teinarss

This comment has been minimized.

Copy link
Contributor Author

teinarss commented Jun 21, 2019

@ghost

This comment has been minimized.

Copy link

ghost commented Jun 21, 2019

Would it be possible to remove/crop the label from the cameo?

Please ignore/defer this request to a followup. For one, cropping the bottom causes graphic errors on the chisel edge of all cameos. For two, TS cameos are already untexted so any cropping would just break them instead. For three, there are no cropping on production and support power cameos either and so far it wasn't a problem.

If this is indeed a problem, the best course of action is to expose the cameos used for this in YAML and start using the lowres and/or include untexted variants in TD/RA.

It was a question, not a request but whatever. I'll ignore the whole feature instead. What I wanted to know was, if you could for example just remove the text layer, so you don't have to crop it. Whatever.

@teinarss teinarss force-pushed the teinarss:spec_army branch from da9a156 to 0e40371 Jun 22, 2019
@teinarss

This comment has been minimized.

Copy link
Contributor Author

teinarss commented Jun 22, 2019

Updated: added tooltip and fixed overflow in this and the production widget

@pchote pchote added this to the Next Release milestone Jul 19, 2019
Copy link
Member

pchote left a comment

Unfortunately I think is going to need some more time to bake. Lets push this back to Next + 1 so we can do it proper justice.

@pchote pchote modified the milestones: Next Release, Next+1 Jul 20, 2019
@teinarss teinarss force-pushed the teinarss:spec_army branch from 0e40371 to cad0408 Jul 23, 2019
@teinarss

This comment has been minimized.

Copy link
Contributor Author

teinarss commented Jul 23, 2019

Updated and rebased

@teinarss

This comment has been minimized.

Copy link
Contributor Author

teinarss commented Jul 24, 2019

Some different layouts we have talked about.
image

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Jul 24, 2019

A maybe more useful variation on (2) would be to show multiple rows in the tooltip, one for each unit class.

@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Oct 13, 2019

Needs a rebase now.

@teinarss teinarss force-pushed the teinarss:spec_army branch from cad0408 to cd85a95 Oct 20, 2019
@teinarss

This comment has been minimized.

Copy link
Contributor Author

teinarss commented Oct 20, 2019

Updated

var topColor = Color.FromArgb(0, 0, 0, 0);
var bottomColor = Color.FromArgb(228, 0, 0, 0);

// WidgetUtils.FillRectWithColor(new Rectangle((int)iconTopLeft.X, (int)(iconTopLeft.Y + iconSize.Y / 2), (int)iconSize.X, (int)iconSize.Y / 2), topColor, topColor, bottomColor, bottomColor);

This comment has been minimized.

Copy link
@teinarss

teinarss Oct 20, 2019

Author Contributor

Keeping it here during the reviewing phase, uncomment to add the fading on the bottom of the icons to hide the text.

Copy link
Member

abcdefg30 left a comment

Looks good to me ingame. Two things:

  • ObserverArmyIconsWidget .cs has a space in its name
  • Imho we should consider always having the same (expanded) width for all players, as the difference between a player with little and one with many units looks a bit odd (especially when hovering)
@teinarss

This comment has been minimized.

Copy link
Contributor Author

teinarss commented Dec 14, 2019

Found a bug, crashing when units are present that are not from the building queue, ie in RA units from the paradrop. Thinking we need a dedicated trait instead of using Buildable as we do now.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Feb 8, 2020

The d2k army tab looks a bit weird because its icons are slightly smaller than the other mods - this is a problem on the other tabs too, but is more noticable now because we now have right-aligned text.

Screenshot 2020-02-08 at 10 28 06

Is it too much of a scope creep to ask for a new commit that duplicates the spectator yaml for d2k and fixes the IconWidth / IconHeight definitions? We shouldn't change the row height, but could give the icons a Y: 1 so they are more centered.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Feb 8, 2020

D2k's starport actors will also need to define OverrideActor so they aren't duplicated in the army tab!

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Feb 8, 2020

TS interleaves infantry with vehicles, which I guess means that the production queues aren't defining a display order:

Screenshot 2020-02-08 at 10 34 48

Copy link
Member

pchote left a comment

A few more polish nits, then LGTM

@teinarss teinarss force-pushed the teinarss:spec_army branch from 5f2617f to eaedb09 Feb 8, 2020
@teinarss

This comment has been minimized.

Copy link
Contributor Author

teinarss commented Feb 8, 2020

Updated

mods/d2k/rules/starport.yaml Outdated Show resolved Hide resolved
@teinarss teinarss force-pushed the teinarss:spec_army branch from eaedb09 to 6760399 Feb 8, 2020
Copy link
Member

pchote left a comment

Spotted another couple of issues in d2k. Sorry!

mods/d2k/chrome/ingame-observer.yaml Show resolved Hide resolved
mods/ra/chrome/ingame-observer.yaml Show resolved Hide resolved
@teinarss teinarss force-pushed the teinarss:spec_army branch from 6760399 to a183396 Feb 9, 2020
@teinarss

This comment has been minimized.

Copy link
Contributor Author

teinarss commented Feb 9, 2020

updated

@pchote
pchote approved these changes Feb 9, 2020
@pchote pchote added the PR: Needs +2 label Feb 9, 2020
Copy link
Member

abcdefg30 left a comment

Tooltips do not update when the icon below the cursor is pushed/pulled by another icon being added or removed.


public override void Tick()
{
if (lastIconIdx >= armyIcons.Count)

This comment has been minimized.

Copy link
@abcdefg30

abcdefg30 Feb 9, 2020

Member

I don't know what this is for. This prevents me from getting any more tooltips if I hovered over an icon with an index bigger or equal to the current amount of icons.

Copy link
Member

abcdefg30 left a comment

As discussed on IRC the icon breakage is beyond the scope of this PR.

@abcdefg30 abcdefg30 merged commit 817f75c into OpenRA:bleed Feb 9, 2020
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Feb 9, 2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.