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

Fixed unnecessary crash if RallyPoint palette is not used. #15530

Merged
merged 1 commit into from Aug 16, 2018

Conversation

Projects
None yet
4 participants
@IceReaper
Copy link
Contributor

IceReaper commented Aug 16, 2018

Fetching the palette causes a crash if not found. While this is ofcourse intended behavior, it does not make sense, if no circle or flag is used. In this cases, the palette isnt used at all, and should not cause a crash.

@@ -97,7 +97,7 @@ IEnumerable<IRenderable> IEffectAboveShroud.RenderAboveShroud(WorldRenderer wr)

IEnumerable<IRenderable> RenderInner(WorldRenderer wr)
{
var palette = wr.Palette(rp.PaletteName);
var palette = circles != null && flag != null ? wr.Palette(rp.PaletteName) : null;

This comment has been minimized.

@pchote

pchote Aug 16, 2018

Member

Instead of defaulting an unused variable to an unused value, how about we change the code below the target line to something like:

if (circles != null || flag != null)
{
	var palette = wr.Palette(rp.PaletteName);
	if (circles != null)
		// and so on...

?

@GraionDilach
Copy link
Contributor

GraionDilach left a comment

👍

@GraionDilach
Copy link
Contributor

GraionDilach left a comment

Can't dismiss my own review, so I'll just go this way to supplement @pchote.

@IceReaper IceReaper force-pushed the IceReaper:rallypoint_crashfix branch from 6f8360e to a2aba58 Aug 16, 2018

@IceReaper

This comment has been minimized.

Copy link
Contributor

IceReaper commented Aug 16, 2018

Already changed :-P

@GraionDilach
Copy link
Contributor

GraionDilach left a comment

👍

@pchote
Copy link
Member

pchote left a comment

Code LGTM, but not tested.

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Aug 16, 2018

@IceReaper Could you please change


to if (!string.IsNullOrEmpty(rp.Info.Image)) to make it easier to disable, while you're at it?
👍 after that.

Edit: Turns out setting 'Image: ' in yaml is actually counted as null, so ^ isn't necessary.

@reaperrr reaperrr merged commit 8c5caaf into OpenRA:bleed Aug 16, 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 Aug 16, 2018

@IceReaper IceReaper deleted the IceReaper:rallypoint_crashfix branch Sep 27, 2018

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