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

Visualising waypoints 4 #16549

Merged
merged 4 commits into from Aug 5, 2019

Conversation

@tovl
Copy link
Contributor

commented May 17, 2019

Fixes #2598
Fixes #12019

This is completing the work that @Turupawn started and @forcecore continued in #14384 .
I've rebased it on top of #16481 to complete the general activity rework and added a few modifications of my own:

  • Made target lines thicker (Maybe it's because of my high-resolution monitor or my eyes are just bad, but I found the single pixel lines hard to read when planning queued orders)
  • Omitted target lines from autotarget attacks and AttackMove/Guard attacks.
  • Changed some colors of target lines:
    • AttackMove and Guard get yellow (because they mix move -> green and attack -> red, yellow seems intuitive)
    • 'Special' non-aggressive actions changed from yellow to blue.

In general only actions that directly correspond to given orders should have wapoint lines rendered. I may possibly reintroduce special target lines for autotarget, AttackMove/Guard and opportunity fire targets in a future PR. These target lines would be completely seperate from the waypoint lines, not part of the chain and rendered in thinner lines.

@pchote

This comment has been minimized.

Copy link
Member

commented May 17, 2019

In general only actions that directly correspond to given orders should have wapoint lines rendered.

IMO the rule of thumb here should be that we show the line for the parent activity, and ignore any children - so AttackMove would show the line to the move target, Guard to to the unit that is being guarded, the various things that use MoveAdjacentTo to the center of the actor that is being moved next to, etc.

@pchote

This comment has been minimized.

Copy link
Member

commented May 17, 2019

The visual changes (color/thickness) are likely going to be controversial, so can be please defer those to a followup to avoid bogging this important PR down with visual bikeshedding?

@pchote

This comment has been minimized.

Copy link
Member

commented May 17, 2019

I thought we had an issue about the target lines getting out of sync, which this will fix, but can't find it.

@pchote pchote added this to the Next Release milestone May 20, 2019

@dragunoff

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

I agree with pchote that the visual changes are controversial and are best left to follow-ups. The only visual change that I'd suggest here is adding a small square/dot at each node in the path.

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

The color changes here are not about aesthetics though, but rather just basic functionality. The whole point here is to give players a visual reminder of the orders they have queued. I think that at the very least they should be able to distinguish attack, move and attack-move orders. Giving them three different colors is the most straightforward way to do that without getting too deep into aesthetic details.

It then also makes sense to have a fourth color for orders that do not really correspond to either move/attack/attack-move. Which color that is doesn't really concern me as long as it's easily distinguishable; Blue seems as good as any.

@pchote

This comment has been minimized.

Copy link
Member

commented May 22, 2019

I like the idea of expanding the color palette, but I think we're going to spend some time hashing out the specific scheme - for example, the previous convention of gold for repair/guard/heal was based on the gold wrench / yellow enter icon for engineers, while we have a blue cursor for other generic abilities.

Something about the wide lines with missing end dots really feels wrong to me, so I'd also like to iterate on this further too. I don't think going back to the old lines is necessarily the best option though - it would be nice if we could e.g. show striped red/green lines for attackmove and i'm not sure that would work with only a 1px line.

But again, these cosmetic things really IMO shouldn't drag down the core work here, which we need to fix the target line regressions introduced in the last release.

@matjaeck

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

The color changes here are not about aesthetics though, but rather just basic functionality. The whole point here is to give players a visual reminder of the orders they have queued. I think that at the very least they should be able to distinguish attack, move and attack-move orders. Giving them three different colors is the most straightforward way to do that without getting too deep into aesthetic details.

It is not useful though for colorblind people unless you allow users to define the line style themselves. Specifically green/red is a bad combination when you have no other indicators.

The only visual change that I'd suggest here is adding a small square/dot at each node in the path.

it would be nice if we could e.g. show striped red/green lines for attackmove and i'm not sure that would work with only a 1px line.

I'm not sure if it would look good, but we could try to show an overlay with the command bar icon that represents the queued activity at the line endings.

@pchote

This comment has been minimized.

Copy link
Member

commented May 29, 2019

I created #16616 with additional context and my own thoughts on the line visuals for discussion.

@pchote

This comment has been minimized.

Copy link
Member

commented Jun 29, 2019

It would be good if we can still get the TargetLineNodes part of this PR in for the next release (to fix the target line mismatch bugs that exist on the current release), but in the interests of time we should probably defer the queued renderering and other visual changes to Next + 1.

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2019

We should get #16481 merged first before it makes sense to continue working on this.

@pchote

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

#16481 has been merged.

@Turupawn

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

Just letting you guys know that I'm rooting very hard for you on this 🏄‍♀️
Great to see how it's progressing

@tovl tovl force-pushed the tovl:visualising_waypoints3 branch from c551c7a to 3f4ce3e Jul 4, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

Rebased.

@pchote pchote removed the PR: Rebase me! label Jul 4, 2019

OpenRA.Game/Activities/CallFunc.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Cnc/Activities/Infiltrate.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Activities/Demolish.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Activities/EnterTransport.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/OpenRA.Mods.Common.csproj Outdated Show resolved Hide resolved

@tovl tovl force-pushed the tovl:visualising_waypoints3 branch from 3f4ce3e to 8e97adb Jul 10, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

Removed the unnecessary lines.

@teinarss

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2019

No lines when sending units for repair. Not sure if there is a connection to #16721

@tovl tovl force-pushed the tovl:visualising_waypoints3 branch from 8e97adb to 3edf250 Jul 11, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

No lines when sending units for repair.

Fixed and rebased. Note that move commands queued after repair are still executed in the wrong order, but this is a separate issue that will hopefully be fixed by #16721.

fixed

@pchote

This comment has been minimized.

Copy link
Member

commented Jul 27, 2019

Re minelayers: would it be too much to sneak something like 42d7ae2 in here?

@pchote

This comment has been minimized.

Copy link
Member

commented Jul 27, 2019

This is generally looking good to me, so we may want to merge this sooner rather than later and push any remaining bug fixes to followup PRs.

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2019

would it be too much to sneak something like 42d7ae2 in here?

That looks like a good change, but considering your next remark it seems more prudent to me to make that a follow-up PR as well.

This is generally looking good to me, so we may want to merge this sooner rather than later and push any remaining bug fixes to followup PRs.

I think that would be a good idea. This is probably the last big-ticket feature for next release, so with this out of the way we can concentrate on the regression whack-a-mole. It would be a good idea to make an issue for the harvester bug then; It might take a while to figure out such a hard to replicate bug.

@teinarss

This comment has been minimized.

Copy link
Contributor

commented Jul 28, 2019

Think that i can reproduce it now, queue 5 harvest orders and one move.
harv_waypoint

@Punsho

This comment has been minimized.

Copy link
Contributor

commented Jul 28, 2019

@teinarss you can hold alt to keep those lines visible

@tovl tovl force-pushed the tovl:visualising_waypoints3 branch from e00b752 to 361e054 Jul 28, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Jul 28, 2019

The harvester bug should be fixed now.

@abcdefg30 abcdefg30 removed the PR: Needs +2 label Jul 28, 2019

@teinarss
Copy link
Contributor

left a comment

Good work!

@abcdefg30

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

Needs a rebase.

pchote and others added 4 commits Jul 24, 2019
Overhaul target line rendering:
- Targets are now defined by the activities
- Queued activities are shown
- Support custom attack colors

@tovl tovl force-pushed the tovl:visualising_waypoints3 branch from 361e054 to c7f8219 Aug 4, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2019

Rebased.

@pchote
pchote approved these changes Aug 5, 2019
Copy link
Member

left a comment

The one remaining big issue I found was that this disables the target lines for spectators / ally players.

I have a PR lined up to bring this back with some additional tweaks, so no need to block this further.
Followup refinement and fixes can/will come in followup PRs.

@pchote pchote merged commit 58bb7fc into OpenRA:bleed Aug 5, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Turupawn

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

Awesome guys! Congrats! 🎉 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.