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

Filter out Abstract Actors + Use Constant for Support Directory #14859

Merged
merged 1 commit into from Mar 21, 2018

Conversation

Projects
None yet
6 participants
@PeterAntal
Copy link
Contributor

PeterAntal commented Feb 26, 2018

This exercises @pchote suggestion of skipping partial templates, to avoid need for compensating for errors on incomplete cases, which is inelegant.

This approach ignores all abstract actor templates, in MergeOrDefault(...) process.

As part of the change I added a constant for the support directory symbol, to provide semantic context for "^".

Testing revealed no regressions - If there's particular arrangements of data on rulesets which could be adversely affected, I'd be happy to sanity check.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Feb 26, 2018

You broke the unit tests.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Feb 27, 2018

Let me try to understand the point of this PR - if this PR is merged, then templates themselves don't end up treated as additional ActorInfo entries in the ruleset?

@@ -87,16 +87,19 @@ public class Ruleset

public IEnumerable<KeyValuePair<string, MusicInfo>> InstalledMusic { get { return Music.Where(m => m.Value.Exists); } }

static IReadOnlyDictionary<string, T> MergeOrDefault<T>(string name, IReadOnlyFileSystem fileSystem, IEnumerable<string> files, MiniYaml additional,
IReadOnlyDictionary<string, T> defaults, Func<MiniYamlNode, T> makeObject)
static IReadOnlyDictionary<string, T> MergeOrDefault<T>(string name,

This comment has been minimized.

@pchote

pchote Feb 27, 2018

Member

IMO a cleaner approach here would be to pass in a Func<MiniYamlNode, bool> filterNode argument, then have the callers pass in either n => n.StartsWith("^", StringComparison.Ordinal) (for actors) or _ => true (everything else).

This comment has been minimized.

@PeterAntal

PeterAntal Feb 28, 2018

Author Contributor

Makes sense. I went with a default behavior of no-op rather than requiring explicit behavior on the other callers.

/// </summary>
private static bool IsConcreteNode(MiniYamlNode node)
{
return !SymbolicSupportDirectory.IsPathRelativeToSupportDirectory(node.Key);

This comment has been minimized.

@pchote

pchote Feb 27, 2018

Member

The ^ convention for actor names is completely separate to the convention for using ^ to denote the support directory in file paths. Mixing the two here isn't a good idea!

This comment has been minimized.

@PeterAntal

PeterAntal Feb 28, 2018

Author Contributor

@pchote - Yeah, in hindsight I should have just gone with a minimal change on the impl.

How do you feel about definition of "abstractActorPrefix" string constant on ActorInfo to disambiguate that case, vs dropping the constants business and sticking with the string literals?

@PeterAntal

This comment has been minimized.

Copy link
Contributor Author

PeterAntal commented Feb 28, 2018

@GraionDilach - Yes, to your question - This change is about avoiding exception handling or later pre-emption of errors during handling of the template actors.

Thanks

@@ -108,7 +117,8 @@ public static Ruleset LoadDefaults(ModData modData)
Action f = () =>
{
var actors = MergeOrDefault("Manifest,Rules", fs, m.Rules, null, null,
k => new ActorInfo(modData.ObjectCreator, k.Key.ToLowerInvariant(), k.Value));
k => new ActorInfo(modData.ObjectCreator, k.Key.ToLowerInvariant(), k.Value),
filterNode: n => n.Key.StartsWith(ActorInfo.AbstractActorPrefix, StringComparison.Ordinal));

This comment has been minimized.

@PeterAntal

PeterAntal Feb 28, 2018

Author Contributor

I went with explicit param identifier for readability, as the actor case is the only one opting into the filter pattern, and I didn't want to spam the others with boilerplate.

@PeterAntal PeterAntal force-pushed the PeterAntal:FilterAbstractNodes branch from 62cc4c9 to a7e95d0 Feb 28, 2018

return new ReadOnlyDictionary<string, T>(result);
// Optionally, the caller can filter out elements from the loaded set of nodes. Default behavior is unfiltered.
if (filterNode != null)
{

This comment has been minimized.

@abcdefg30

abcdefg30 Feb 28, 2018

Member

No braces around single-line statements, please. ^^

This comment has been minimized.

@PeterAntal

PeterAntal Mar 1, 2018

Author Contributor

Thanks - old habit. I'll see if I can find a linter or editor extension to help me catch these.

@@ -39,7 +39,7 @@ void IUtilityCommand.Run(Utility utility, string[] args)
Console.WriteLine("Tileset: " + ts.Name);
var sc = new SpriteCache(modData.DefaultFileSystem, modData.SpriteLoaders, new SheetBuilder(SheetType.Indexed));
var nodes = MiniYaml.Merge(modData.Manifest.Sequences.Select(s => MiniYaml.FromStream(modData.DefaultFileSystem.Open(s), s)));
foreach (var n in nodes.Where(node => !node.Key.StartsWith("^")))
foreach (var n in nodes.Where(node => !node.Key.StartsWith(SymbolicSupportDirectory.Identifier)))

This comment has been minimized.

@abcdefg30

abcdefg30 Feb 28, 2018

Member

This looks like we're mixing directory and actor name identifiers. (?)

This comment has been minimized.

@PeterAntal

PeterAntal Mar 1, 2018

Author Contributor

That's right I missed that one, confirmed actors are on those nodes. Fixed.

@pchote
Copy link
Member

pchote left a comment

Looks sensible overall, but I haven't tested it. Just a couple of minor things with the support dir changes:

/// <summary>
/// Responsible for constants and common logic of dealing with the Symbolic support directory path.
/// </summary>
public class SymbolicSupportDirectory

This comment has been minimized.

@pchote

pchote Mar 3, 2018

Member

Please split these support directory changes into their own commit.
OpenRA conventions are to keep distinct changes in distinct commits!

This comment has been minimized.

@PeterAntal

PeterAntal Mar 5, 2018

Author Contributor

Ack'ed- I'll separate this facet to a separate PR.

/// </summary>
public class SymbolicSupportDirectory
{
public const string Identifier = "^";

This comment has been minimized.

@pchote

pchote Mar 3, 2018

Member

This would be better defined as public const string SupportDirPrefix on the Platform class.

This comment has been minimized.

@PeterAntal

PeterAntal Mar 5, 2018

Author Contributor

Ack'ed - Makes sense.

{
public const string Identifier = "^";

public static bool IsPathRelativeToSupportDirectory(string path)

This comment has been minimized.

@pchote

pchote Mar 3, 2018

Member

This would also be better in Platform, which is where the rest of our support dir path related code lives.

@PeterAntal PeterAntal force-pushed the PeterAntal:FilterAbstractNodes branch from 86070c1 to 0e62ee2 Mar 5, 2018

@PeterAntal

This comment has been minimized.

Copy link
Contributor Author

PeterAntal commented Mar 5, 2018

PR is updated to omit support Dir Prefix aspects, which have been moved to separate PR.

@pchote
Copy link
Member

pchote left a comment

LGTM 👍

Not strictly a blocker, but it would be good if you could change the first line of the commit message to just "Allow each caller on MergeOrDefault to discard nodes" so that it is not truncated in the git shortlog.

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

@PeterAntal PeterAntal force-pushed the PeterAntal:FilterAbstractNodes branch from 0e62ee2 to 3169416 Mar 11, 2018

@PeterAntal

This comment has been minimized.

Copy link
Contributor Author

PeterAntal commented Mar 11, 2018

Updated the commit message per suggestion.

@@ -39,7 +39,7 @@ void IUtilityCommand.Run(Utility utility, string[] args)
Console.WriteLine("Tileset: " + ts.Name);
var sc = new SpriteCache(modData.DefaultFileSystem, modData.SpriteLoaders, new SheetBuilder(SheetType.Indexed));
var nodes = MiniYaml.Merge(modData.Manifest.Sequences.Select(s => MiniYaml.FromStream(modData.DefaultFileSystem.Open(s), s)));
foreach (var n in nodes.Where(node => !node.Key.StartsWith("^")))
foreach (var n in nodes.Where(node => !node.Key.StartsWith(ActorInfo.AbstractActorPrefix)))

This comment has been minimized.

@RoosterDragon

RoosterDragon Mar 17, 2018

Member

Specify StringComparison.Ordinal here.

{
if (additional == null && defaults != null)
return defaults;

var result = MiniYaml.Load(fileSystem, files, additional)
.ToDictionaryWithConflictLog(k => k.Key.ToLowerInvariant(), makeObject, "LoadFromManifest<" + name + ">");
var yamlNodes = MiniYaml.Load(fileSystem, files, additional) as IEnumerable<MiniYamlNode>;

This comment has been minimized.

@RoosterDragon

RoosterDragon Mar 17, 2018

Member

IEnumerable<MiniYamlNode> yamlNodes = MiniYaml.Load(fileSystem, files, additional); would suffice.

@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Mar 20, 2018

Can you squash the commits, please?

Allow each caller on MergeOrDefault to discard nodes.
Use filtering ActorInfo's to drop template actors.

@PeterAntal PeterAntal force-pushed the PeterAntal:FilterAbstractNodes branch from ded3d26 to 3e54ac8 Mar 21, 2018

@PeterAntal

This comment has been minimized.

Copy link
Contributor Author

PeterAntal commented Mar 21, 2018

Commits squashed :)

@abcdefg30 abcdefg30 merged commit d2ff5b4 into OpenRA:bleed Mar 21, 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 Mar 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.