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

WithDecoration: fixed crash when Palette is null #20896

Merged
merged 1 commit into from Jun 2, 2023

Conversation

michaeldgg2
Copy link
Contributor

@michaeldgg2 michaeldgg2 commented Jun 1, 2023

In current code, when Palette is null, WorldRenderer.Palette is invoked with empty string and in conjunction with how palettes are retrieved in HardwarePalette:

public IPalette GetPalette(string name)
{
	if (mutablePalettes.TryGetValue(name, out var mutable))
		return mutable.AsReadOnly();
	if (palettes.TryGetValue(name, out var immutable))
		return immutable;
	throw new InvalidOperationException($"Palette `{name}` does not exist");
}

results in crash. This PR changes how the palette name is constructed so that with Palette == null it doesn't result in empty string.

@PunkPun
Copy link
Member

PunkPun commented Jun 1, 2023

wouldn't it be better for WorldRenderer.Palette to check string.IsNullOrEmpty instead?

@michaeldgg2
Copy link
Contributor Author

It would be a better solution, but my intention was to make just the fix for WithDecoration (as it's currently blocking us). Adding check to WorldRenderer.Palette means that the null check, that is currently used throughout the code base, would be redundant and thus should be removed. That means bigger PR, which means longer review time.

But if you want, I can close this and make that bigger PR.

@PunkPun
Copy link
Member

PunkPun commented Jun 1, 2023

It wouldn't really be a bigger PR. And I mean replacing the null check with a is null or empty check

@Mailaender Mailaender merged commit dac35a6 into OpenRA:bleed Jun 2, 2023
3 checks passed
@Mailaender
Copy link
Member

Changelog

@michaeldgg2 michaeldgg2 deleted the WithDecoration-NullPaletteFix branch June 23, 2023 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants