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

Split "Locomotor" from Mobile #14893

Merged
merged 9 commits into from May 3, 2018

Conversation

@reaperrr
Copy link
Contributor

reaperrr commented Mar 9, 2018

This moves a lot of stuff from Mobile to Locomotor world trait(s).

The idea is to keep the number of locomotors limited and - in the future - cache/pre-calculate as many things as possible on them, to save performance and make further movement code refactors easier.

@reaperrr reaperrr force-pushed the reaperrr:movement-classes branch from b8320d7 to b2d6818 Mar 9, 2018

@PeterAntal

This comment has been minimized.

Copy link
Contributor

PeterAntal commented Mar 9, 2018

I'd be open to try help if I can better understand the intent.

Can you explain the desired relationship of Mobile, MobileInfo and MovementClassInfo in your design change here?

@PeterAntal

This comment has been minimized.

Copy link
Contributor

PeterAntal commented Mar 9, 2018

Are you manually testing, or have you been running the "make test" flow? I'm seeing a bunch non-blocking load failures getting logged, when instantiating the new MovementClassInfo at load time. These might be happening upstream of your crashes.

_Testing mods...
Testing Tiberian Sun mod MiniYAML...
Testing mod: Tiberian Sun
OpenRA.Utility(1,1): Error: Actor world is not constructible; failure: ActorInfo("world") failed to initialize because of the following:
Missing:
OpenRA.Mods.Common.Traits.MovementClassInfo
Unresolved:
OpenRA.Mods.Common.Traits.PathFinderInfo: { OpenRA.Mods.Common.Traits.MovementClassInfo }

OpenRA.Utility(1,1): Error: Actor type weedguy consumes conditions that are not granted: inside-tunnel
..._

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Mar 9, 2018

@PeterAntal I only migrated RA so far, so test errors in the other mods are to be expected.
RA passes make test, but still crashes at launch with the exception linked in the OP.

The idea here is to use a limited number of MovementClass (a different name like MovementType might be better, but this is what I went with, for now) traits residing on the world actor that cache their terrain speed and cost etc. at game start, which Mobile then only needs to access, instead of doing this for every actor with Mobile separately.

This is meant to be the first step of #14830. See also the discussions on IRC (log) from yesterday and around 19-20 days ago, or try to get hold of @pchote or @chrisforbes here or on IRC for any additional technical questions (I understand the concept, but I'm not good at explaining the details).

@reaperrr reaperrr force-pushed the reaperrr:movement-classes branch from b2d6818 to 56745b8 Mar 9, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Mar 9, 2018

Updated.

There are still issues with the TS-specific stuff (at least with Tunnel-, Jumpjet- and SubterraneanConditions), but the other 3 mods now pass all tests and seem to work fine.

@PeterAntal

This comment has been minimized.

Copy link
Contributor

PeterAntal commented Mar 10, 2018

Got it - so, to paraphrase, it sounds like the general aspiration is:
1/2- pre-compute a graphs summarizing the long distance pathfinding model, representing each kind of movement
3-Exercise long distance navigation through the model in place of present scheme.

I'll see if I can get context on what should be implementing traits of your new MovementClass.

public bool CanEnterCell(World world, Actor self, CPos cell, Actor ignoreActor = null, bool checkTransientActors = true)
{
if (MovementCostForCell(world, cell) == int.MaxValue)
var movementClassInfo = world.WorldActor.TraitsImplementing<MovementClass>().Single(mc => mc.Info.Class == MovementClass).Info;

This comment has been minimized.

@PeterAntal

PeterAntal Mar 10, 2018

Contributor

The worldActor does not itself include MovementClass Trait, which the mobile instances are looking for.

Are you sure a trait on the world the right mechanism for obtaining an instance?

Seems like you want something of a instance management mechanism in relation to the world actor, to request instantiation of the MovementClass, if it doesn't already exist(the world probably shouldn't need to declare what specific Movement Class instances are needed).

This comment has been minimized.

@pchote

pchote Mar 10, 2018

Member

OpenRA uses traits on the world to hold various types of global state, including palettes, faction definitions, and references for the in-game UI. Defining the movement types on the world also matches up with the original games (which called them "locomotors"), and makes it more explicit that these are things that should be limited in number.

This comment has been minimized.

@pchote

pchote Mar 10, 2018

Member

But please do this as world.WorldActor.Info.TraitInfos<MovementClassInfo>().Single(mc => mc.Class == MovementClass) in RulesetLoaded instead of going via the actual world actor, and use the cached version here.

public readonly int WaitAverage = 5;

public readonly int WaitSpread = 2;
public readonly string MovementClass = "default";

This comment has been minimized.

@pchote

pchote Mar 10, 2018

Member

It would be good to define a [MovementClassReference] attribute that can be linted in the same way as actor, weapon, sequence, etc references.


public bool SharesCell = false;

This comment has been minimized.

@pchote

pchote Mar 10, 2018

Member

It would be cleaner to use the IRulesetLoaded to store a reference to the MovementClassInfo, the same way that we do for weapons in ArmamentInfo. This can then be removed.

This comment has been minimized.

@GraionDilach

GraionDilach Mar 10, 2018

Contributor

Shifting this towards movementclass might imply the plumbing/possibility for multi-cell units which sounds out-of-scope on this run to me though.

This comment has been minimized.

@pchote

pchote Mar 10, 2018

Member

I'm not sure what you mean. This is about code quality, not feature changes.

This comment has been minimized.

@GraionDilach

GraionDilach Mar 10, 2018

Contributor

I understand that aspect, but not all modders might follow us in that regard though.

This comment has been minimized.

@pchote

pchote Mar 10, 2018

Member

This variable holds a cached value of the SharesCell defined on MovementClassInfo. It is overwritten whenever CanEnterCell is called, so any values set in the yaml are completely ignored.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Mar 10, 2018

I'm not really pleased with the idea to tie crushing abilities to the locomotor tbh.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Mar 10, 2018

The ability to crush is one of the most important parameters in the pathing state. The pathfinder improvements can't happen without it moving.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Mar 10, 2018

Well, Crushable is conditional already so I guess this can go that way then and maybe it's just me who's seeing issues atm.

@reaperrr reaperrr force-pushed the reaperrr:movement-classes branch 4 times, most recently from 315b972 to b8a9fcd Mar 11, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Mar 11, 2018

Updated, managed to work out all the initial kinks (even tunnels, jumpjets and subterranean units work now). I think this looks good enough now that I can at least remove the help request label.

One thing I'd like to get feedback on, though:
Should we stick to MovementClass, or go for MovementType or just Locomotor (like in TS/RA2)?
I'm asking because I noticed there are some places in the Pathfinder code that already used the term MovementClass in a slightly different context.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Mar 14, 2018

My preference is for Locomotor because Movement* is misleading (this logic only applies to Mobile, not the other movement types).

@MustaphaTR

This comment has been minimized.

Copy link
Member

MustaphaTR commented Mar 14, 2018

I don't think what this does and what Locomotor in original games does is the same thing. This is more like the SpeedType. Not sure if is a good name, but i don't think Locomotor is good either.

For reference, here is Locomotor's ModEnc page: https://www.modenc.renegadeprojects.com/Locomotors

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Mar 14, 2018

I don't think what this does and what Locomotor in original games does is the same thing.
This is more like the SpeedType. Not sure if is a good name, but i don't think Locomotor is good either.

That may be the case right now, but there are at least two things to consider:

  • making the trait name more accurate would make it longer, too (GroundMovementPropertyClass, GroundMovementBehaviorType or something along these lines would be a bit long)
  • the idea isn't to leave the trait like it is now (which already goes quite beyond the Westwood SpeedType), but rather to move more stuff from Mobile here later, turning Mobile more into a simpler, "carries some properties, some interfaces, and calls some external methods" kind of trait, instead of doing most locomotor-like stuff by itself.
@pchote
Copy link
Member

pchote left a comment

Would you mind splitting this up a bit further? These changes should go a long way towards allowing modders to define their own custom movement types without having to modify the common logic.

[RequireExplicitImplementation]
public interface INotifyChangedCustomLayer
{
void JumpjetLaunch(Actor self);

This comment has been minimized.

@pchote

pchote Mar 14, 2018

Member

Can we please simplify this down to a single CustomLayerChanged(CustomMovementLayerType old, CustomMovementLayerType new)?

public object Create(ActorInitializer init) { return new GrantConditionOnCustomLayer(init, this); }
}

public class GrantConditionOnCustomLayer : INotifyCreated, INotifyChangedCustomLayer

This comment has been minimized.

@pchote

pchote Mar 14, 2018

Member

and then split this into separate traits for Tunnel/Subterranean/Jumpjet. You could define a single abstract class that is given the CustomLayerMovementType to check in its ctor from the subclass. The CustomLayerChanged method in each can then check this value and grant/revoke the condition when needed.

tunnelToken = conditionManager.RevokeCondition(self, tunnelToken);
if (toCell.Layer == CustomMovementLayerType.Tunnel && fromCell.Layer != CustomMovementLayerType.Tunnel)
foreach (var n in notifyCustomLayerChange)
n.TunnelEnter(self);

This comment has been minimized.

@pchote

pchote Mar 14, 2018

Member

A lot of the logic here can then go away.

public readonly bool MoveIntoShroud = true;

[Desc("Can this unit move underground?")]
public readonly bool Subterranean = false;

This comment has been minimized.

@pchote

pchote Mar 14, 2018

Member

Can you please define subclasses for SubterraneanMovementClassInfo and the other custom types?

The checks for if (mc.Subterranean) can then be replaced by if (mc is SubterraneanMovementClassInfo) or

var smc = mc as SubterraneanMovementClassInfo;
if (smc != null)

if the Subterranean specific info is needed.

@reaperrr reaperrr force-pushed the reaperrr:movement-classes branch 2 times, most recently from 80093a0 to 230a9ee Mar 16, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Mar 16, 2018

Updated.

@reaperrr reaperrr changed the title Movement classes Split "Locomotor" from Mobile Mar 16, 2018

@reaperrr reaperrr force-pushed the reaperrr:movement-classes branch from 8a2c84f to 208e068 Mar 17, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Mar 17, 2018

Updated again.

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Apr 29, 2018

Updated and rebased.

@pchote
Copy link
Member

pchote left a comment

LGTM 👍 Any regressions that may be hiding inside here will hopefully be shaken out during playtesting.

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

void CheckLocomotors(ActorInfo actorInfo, Action<string> emitError, Ruleset rules, string locomotor)
{
var worldActor = rules.Actors["world"];
var locomotorInfos = worldActor.TraitInfos<LocomotorInfo>();

This comment has been minimized.

@abcdefg30

abcdefg30 May 2, 2018

Member

Minor: You could pull this out of all the foreach loops probably, as this isn't changing during the lint run?

{
var passable = mi.GetMovementClass(world.Map.Rules.TileSet);
if (!domainIndex.IsPassable(source, target, mi, (uint)passable))
if (!domainIndex.IsPassable(source, target, li))

This comment has been minimized.

@abcdefg30

abcdefg30 May 2, 2018

Member

Neglectible: I'd be nice if this was combined into the if check above.

var grantNode = new MiniYamlNode("GrantConditionOnTunnelLayer", "");
grantNode.AddNode("Condition", tunnelConditionNode.Value.Value);
addNodes.Add(grantNode);
mobileNode.Value.Nodes.Remove(tunnelConditionNode);

This comment has been minimized.

@abcdefg30

abcdefg30 May 2, 2018

Member

This applies to the entire file: You should use the respective Remove and Add methods instead of using .Value.Nodes.

This comment has been minimized.

@reaperrr

reaperrr May 2, 2018

Author Contributor

Done, but only In places I was confident about (this rule was pretty much written by @pchote).

if (!string.IsNullOrEmpty(Info.SubterraneanTransitionSound))
Game.Sound.Play(SoundType.World, Info.SubterraneanTransitionSound);
// The first time SetVisualPosition is called is in the constructor before creation, so we need a null check here as well
if (notifyVisualPositionChanged == null || !notifyVisualPositionChanged.Any())

This comment has been minimized.

@abcdefg30

abcdefg30 May 2, 2018

Member

Neglectible: I don't think the .Any check is needed. When the collection is empty, we'll just loop zero times.

internal readonly TerrainInfo[] TerrainInfos;
internal WorldMovementInfo(World world, MobileInfo info)
var locomotorInfos = rules.Actors["world"].TraitInfos<LocomotorInfo>();
LocomotorInfo = locomotorInfos.SingleOrDefault(li => li.Name == Locomotor);

This comment has been minimized.

@abcdefg30

abcdefg30 May 2, 2018

Member

The part below won't work. SingleOrDefault will already crash when there are multiple locomotors with the same name.

This comment has been minimized.

@reaperrr

reaperrr May 2, 2018

Author Contributor

I rewrote the entire check. SingleOrDefault didn't return a meaningful error, so I changed it to FirstOrDefault and check the Count later instead.

@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented May 2, 2018

Needs a rebase. Looks good to me otherwise. 👍 /

reaperrr added some commits Mar 14, 2018

Split Locomotor trait from Mobile
Add GrantConditionOn*Layer traits

This allows to
- drop some booleans from Locomotor
- drop a good part of the subterranean- and jumpjet-specific code/hacks from Mobile
- grant more than 1 condition per layer type (via multiple traits)
- easily add more traits of this kind for other layers
Don't pass movement class via IsPassable directly
Let IsPassable get that info from the locomotor instead.

@reaperrr reaperrr force-pushed the reaperrr:movement-classes branch from 21dd914 to db54999 May 2, 2018

@reaperrr reaperrr removed the PR: Rebase me! label May 2, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented May 2, 2018

Rebased and updated.

@abcdefg30 abcdefg30 merged commit 0795701 into OpenRA:bleed May 3, 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 May 3, 2018

@reaperrr reaperrr deleted the reaperrr:movement-classes branch May 7, 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.