Page MenuHomeWildfire Games

Rallypoints should be rendered in player colors
ClosedPublic

Authored by Stan on Jun 10 2017, 1:22 AM.

Details

Reviewers
elexis
Vulcan
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP19957: Display rallypoints in the playercolor instead of plain blue.
Trac Tickets
#4618
Summary

As reported by cc, the rallypoints are always rendered in blue, but it would be more consistent if it would use color of the owner of the building.

The color is currently a property of the template, so it would have to be removed.

The changes have to be done in CCmpRallyPointRenderer.cpp. The player color can be fetched via cmpPlayer->GetColor() as in other C++ components.

Test Plan

./

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Stan created this revision.Jun 10 2017, 1:22 AM
elexis requested changes to this revision.Jun 10 2017, 2:22 AM

Thanks for the patch, looks nice!

There is also the red color for lines in fog of war.
Red implies that something is not valid.
This actually seems to be the case.
Paths in the fog of war don't take into account actual terrain passability until it's explored
(confirm by setting some waypoints on a river map, then explore the map and see the segments changing while exploring).
So seems correct to keep the red color.
Still wondering why that's in the template, as it should be the same for all entities then, but whatever.

binaries/data/mods/public/art/actors/props/special/common/waypoint_flag.xml
85 ↗(On Diff #2501)

Not needed for this patch, and if I noticed correctly, doesn't work for all flags either

binaries/data/mods/public/simulation/templates/template_structure.xml
97 ↗(On Diff #2501)

0.3 seems too fat, 0.25 might be better.
#763 will make yellow a bit darker, the other colors come out well already

source/simulation2/components/CCmpRallyPointRenderer.cpp
219 ↗(On Diff #2501)

We do reset the rallypoint, but it would be more solid to recompute the color here.

i.e. that function mentioned below should be called on init and here

447 ↗(On Diff #2501)

1.0f for all of them

753 ↗(On Diff #2501)

Shouldn't break this block that only copies properties. Better move it to a separate function, then you can keep the returns too.

759 ↗(On Diff #2501)

returning here isn't a good idea

This revision now requires changes to proceed.Jun 10 2017, 2:22 AM
Vulcan edited edge metadata.Jun 10 2017, 8:43 AM

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://jw:8080/job/phabricator/1526/ for more details.

Stan updated this revision to Diff 2507.Jun 10 2017, 10:55 AM
Stan edited edge metadata.

Fix elexis comments

Stan marked 4 inline comments as done.Jun 10 2017, 10:56 AM
elexis added inline comments.Jun 10 2017, 12:29 PM
source/simulation2/components/CCmpRallyPointRenderer.cpp
434 ↗(On Diff #2507)

without the CCmpRallyPointRenderer:: and put it near UpdateMarkers?

555 ↗(On Diff #2507)

UpdateLineColor sounds more fitting

558 ↗(On Diff #2507)

Why not an early return here too?

564 ↗(On Diff #2507)

Sure about m_Color? I guess we can remove the comment altogether instead of fixing the copypasta

Stan updated this revision to Diff 2509.Jun 10 2017, 1:10 PM

Rename a method, remove a wrong prefix, use early returns remove a comment

Stan marked 4 inline comments as done.Jun 10 2017, 1:10 PM
elexis added inline comments.Jun 13 2017, 5:50 AM
source/simulation2/components/CCmpRallyPointRenderer.cpp
783 ↗(On Diff #2509)

Is there a reason to update the line color each call?

Stan added inline comments.Jun 13 2017, 7:49 AM
source/simulation2/components/CCmpRallyPointRenderer.cpp
783 ↗(On Diff #2509)

Can't call it in init it doesn't work. It would just display a black line

Does it need to be player color? Why not just white or gray, currently not any player color? I agree the blue line is kind of an odd choice.

Stan added a comment.Jun 13 2017, 12:23 PM

I don't know I have no strong opinions on this. Would make it coherent with the fact flags have no player Color. What do you think @elexis ?

Does it need to be player color? Why not just white or gray, currently not any player color? I agree the blue line is kind of an odd choice.

Why not? Did you try it? It looks nice and expected IMO

source/simulation2/components/CCmpRallyPointRenderer.cpp
783 ↗(On Diff #2509)

OnGlobalInitGame?

Stan added a comment.Jun 13 2017, 2:28 PM

I mean calling it in void CCmpRallyPointRenderer::Init(const CParamNode& paramNode)

elexis added inline comments.Jun 13 2017, 2:34 PM
source/simulation2/components/CCmpRallyPointRenderer.cpp
783 ↗(On Diff #2509)

I didn't say that (it wouldn't work for buildings constructed after gamestart).
But I tried on Init and it works perfectly for me. Are you sure you're not just updating the line color and then overwrite it with black?

Stan added a comment.Jun 13 2017, 3:51 PM

Maybe it's just broken in atlas then

In D623#25916, @elexis wrote:

Does it need to be player color? Why not just white or gray, currently not any player color? I agree the blue line is kind of an odd choice.

Why not? Did you try it? It looks nice and expected IMO

I honestly don't expect the line to be any specific color as a player. It shows me a vaguely correct pathing line for the rally point and that's it. I don't expect it to give me player color detail because it's not a world object, just a meta thing. No strong feeling here, just giving an alternate viewpoint.

There is no hard requirement that we use the player color only for ingame models, it's also used for the territory, selection rings, 2D panels, chat, range visualization for example and the rallypoints can't be confused with a building or unit. I can post some screenshots later, IMO it looks really awesome.

source/simulation2/components/CCmpRallyPointRenderer.cpp
783 ↗(On Diff #2509)

So the UpdateLineColor call is only here to make the rallypoint color update if the playercolor was changed after a a building was placed in atlas and rallypoints were set?
That case might not be relevant as it's broken everywhere otherwise too, for which I have created a ticket #4643 and uploaded a wip.
Or are there other cases too?

vladislavbelov added inline comments.
binaries/data/mods/public/simulation/templates/template_structure.xml
97 ↗(On Diff #2501)

Agree with @elexis, lines are too fat.

source/simulation2/components/CCmpRallyPointRenderer.cpp
555 ↗(On Diff #2509)

Does it make sense to set the line color to any special value? Then we'll easier catch UpdateLineColor early error return.

783 ↗(On Diff #2509)

I agree with @elexis, it does make sense to not call this each time.

I think it'd be nice to have rally point lines and flags with player color.

Stan updated this revision to Diff 2713.EditedJun 26 2017, 10:42 PM
<elexis> Stan`: I think your color patch is ready if you do the update only on init and ownership change
<elexis> iirc I had posted a question there
<elexis> (the question must have been whether there were any other wrong-color bugs other than changing the civ in atlas)

Done. I haven't seen any bug related to colors, except maybe the one in the actor viewer when you set a player higher than 8, or reduce the number of players etc.

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://jw:8080/job/phabricator/1636/ for more details.

Can't find any atlas bugs atm since I'm always running into #4657.

On green ground, the green player color is a bit hard to notice. Perhaps it makes sense to change the white outline of the texture to black, like territory?

Stan added a comment.Jun 27 2017, 5:40 PM

Best would be black and white but that starts to be out of this ticket. Only issue I see with black is map where the ground is black or dark

elexis accepted this revision.Aug 8 2017, 2:37 PM

Thanks for the patch!
It looks great IMO

source/simulation2/components/CCmpRallyPointRenderer.cpp
219 ↗(On Diff #2501)

When fixing the capitalization of the first word, might want to fix the grammar of the comment as well.
Bit odd to have a comment for the second but not first statement. They could have one comment that works for both statements.

This revision is now accepted and ready to land.Aug 8 2017, 2:37 PM
This revision was automatically updated to reflect the committed changes.