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

Improve RenderShroudCircle #15419

Merged
merged 1 commit into from Dec 24, 2018

Conversation

Projects
None yet
7 participants
@chrisforbes
Copy link
Member

chrisforbes commented Jul 29, 2018

Also align this implementation with other circles.

  • Unhardcoded colors
  • support for multiple CreatesShroud traits
  • cache range at actor creation

@chrisforbes chrisforbes requested review from pchote , atlimit8 and abcdefg30 Jul 29, 2018

@chrisforbes chrisforbes added this to the Next + 1 milestone Jul 29, 2018

@abcdefg30
Copy link
Member

abcdefg30 left a comment

Looks good to me otherwise.

@pchote
Copy link
Member

pchote left a comment

I have force pushed over this to rebase and cache the enumerable.

The code LGTM, but i'm not convinced about the functionality: we don't disable the range circles on low-power base defenses, so why disable them on the gap generator?

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Nov 1, 2018

The code LGTM, but i'm not convinced about the functionality: we don't disable the range circles on low-power base defenses, so why disable them on the gap generator?

It would be a shame for the other improvements to stay unmerged over this, so how about adding a ShowDisabledShroudRanges boolean with the corresponding alternate caching, and let it default to true for now?

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Nov 3, 2018

How about we drop the disabled check so that we can merge the other improvements without changing the behaviour? Discussion can then continue in #15393.

@pchote pchote force-pushed the chrisforbes:rendershroudcircle branch from cc92989 to 6cdba79 Nov 3, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Nov 3, 2018

Ok, done. 👍 on my part, but lets get another pair of eyes on my changes.

@pchote pchote added the PR: Needs +2 label Nov 3, 2018

@reaperrr reaperrr changed the title Consider disabled status in RenderShroudCircle Improve RenderShroudCircle Nov 3, 2018


[Desc("Contrast color of the circle.")]
public readonly Color ContrastColor = Color.FromArgb(96, Color.Black);

public IEnumerable<IRenderable> Render(WorldRenderer wr, World w, ActorInfo ai, WPos centerPosition)
{
var localRange = new RangeCircleRenderable(
centerPosition,
ai.TraitInfo<CreatesShroudInfo>().Range,

This comment has been minimized.

@reaperrr

reaperrr Nov 3, 2018

Contributor

This actually needs to be updated as well, else it crashes when more than 1 CreatesShroud trait is present.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Dec 23, 2018

Pushed a rebase and update.

@pchote pchote force-pushed the chrisforbes:rendershroudcircle branch from 8c0660d to 9e2f083 Dec 23, 2018

@reaperrr reaperrr merged commit e292e88 into OpenRA:bleed Dec 24, 2018

2 checks passed

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

This comment has been minimized.

Copy link
Contributor

reaperrr commented Dec 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment