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

Refactored cursors.yaml to use palettes from rules. #15652

Merged
merged 1 commit into from Oct 7, 2018

Conversation

Projects
None yet
4 participants
@IceReaper
Copy link
Contributor

IceReaper commented Sep 28, 2018

This completely replaces the hacky palettes in cursors.yaml with proper rules based palettes.
This supports any palette loader class, instead of enforcing the default VGA palette for cursors.

@IceReaper IceReaper force-pushed the IceReaper:cursor_palettes branch 3 times, most recently from 344a086 to 9e2e9db Sep 28, 2018

@pchote
Copy link
Member

pchote left a comment

Looks like a straightforward win to me. Just a couple of style nits:

@@ -266,6 +267,7 @@ public interface IRenderModifier
IEnumerable<Rectangle> ModifyScreenBounds(Actor self, WorldRenderer wr, IEnumerable<Rectangle> r);
}

public interface ICursorPalette : ITraitInfoInterface { string Palette { get; } ImmutablePalette ReadPalette(IReadOnlyFileSystem fileSystem); }

This comment has been minimized.

@pchote

pchote Sep 28, 2018

Member

This interface has more than one thing in it, so please use multiple lines for this definition.

This comment has been minimized.

@pchote

pchote Sep 28, 2018

Member

We probably want to call this ICursorPaletteInfo to signify that this is an *Info interface.

Array.Resize(ref shadowIndex, shadowIndex.Length + 1);
Exts.TryParseIntegerInvariant(nodesDict["ShadowIndex"].Value,
out shadowIndex[shadowIndex.Length - 1]);
}

var palettes = new Dictionary<string, ImmutablePalette>();

This comment has been minimized.

@pchote

pchote Sep 28, 2018

Member

This whole block can simplify down to something like:

var pals = modData.DefaultRules.Actors["world"].TraitInfos<ICursorPalette>()
	.ToDictionary(p => p.Palette, p => p);

Palettes = nodesDict["Cursors"].Nodes.Select(n => n.Value.Value)
	.Distinct()
	.ToDictionary(p => p, p => pals[p].ReadPalette(modData.DefaultFileSystem))
	.AsReadOnly();
@@ -35,6 +36,12 @@ class PaletteFromFileInfo : ITraitInfo
public readonly bool AllowModifiers = true;

public object Create(ActorInitializer init) { return new PaletteFromFile(init.World, this); }

public string Palette { get { return Name; } }

This comment has been minimized.

@pchote

pchote Sep 28, 2018

Member

Style nit: prefer explicit interface implementations (and likewise for other traits).

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Sep 28, 2018

We'll need to mention this in the next set of update notes on the SDK wiki, but does not need/want an update rule.

@IceReaper IceReaper force-pushed the IceReaper:cursor_palettes branch 3 times, most recently from 631b837 to 07ec296 Sep 28, 2018

Show resolved Hide resolved OpenRA.Game/Graphics/CursorProvider.cs Outdated
@@ -266,6 +267,13 @@ public interface IRenderModifier
IEnumerable<Rectangle> ModifyScreenBounds(Actor self, WorldRenderer wr, IEnumerable<Rectangle> r);
}

[RequireExplicitImplementation]
public interface ICursorPaletteInfo : ITraitInfoInterface

This comment has been minimized.

@pchote

pchote Sep 29, 2018

Member

Sorry for continuously changing my mind on the naming here, but I now think we should use IProvidesCursorPaletteInfo for consistency with IProvidesAssetBrowserPalettes.

@IceReaper IceReaper force-pushed the IceReaper:cursor_palettes branch 9 times, most recently from 8bbfe04 to 7cffbc3 Sep 29, 2018

@IceReaper IceReaper force-pushed the IceReaper:cursor_palettes branch from 7cffbc3 to 0eca497 Sep 30, 2018

@pchote
Copy link
Member

pchote left a comment

One final request, then 👍

palettes.Add(p.Key, new ImmutablePalette(fileSystem.Open(p.Value.Value), shadowIndex));
var pals = modData.DefaultRules.Actors["world"].TraitInfos<IProvidesCursorPaletteInfo>()
.Where(p => p.Palette != null)
.ToDictionary(p => p.Palette, p => p);

This comment has been minimized.

@pchote

pchote Oct 1, 2018

Member

This will throw an exception if there are duplicate p.Palettes... we probably want to do this manually

// Overwrite previous definitions if there are duplicates
var pals = new Dictionary<string, IProvidesCursorPaletteInfo>();
foreach (var p in modData.DefaultRules.Actors["world"].TraitInfos<IProvidesCursorPaletteInfo>())
	if (p.Palette != null)
		pals[p.Palette] = p;

Its a shame that LINQ doesn't support something like this directly.

@IceReaper IceReaper force-pushed the IceReaper:cursor_palettes branch from 0eca497 to 8762831 Oct 1, 2018

@pchote

pchote approved these changes Oct 2, 2018

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

@drogoganor

This comment has been minimized.

Copy link
Contributor

drogoganor commented Oct 5, 2018

I tried this out and was able to implement IProvidesCursorPaletteInfo on a palette trait info and use it successfully for the cursor palette. FWIW.

@abcdefg30 abcdefg30 merged commit 450dc70 into OpenRA:bleed Oct 7, 2018

2 checks passed

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

This comment has been minimized.

Copy link
Member

abcdefg30 commented Oct 7, 2018

@IceReaper IceReaper deleted the IceReaper:cursor_palettes branch Oct 20, 2018

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