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

Requires<> is bogus. #10907

Closed
GraionDilach opened this issue Mar 11, 2016 · 4 comments · Fixed by #10965
Closed

Requires<> is bogus. #10907

GraionDilach opened this issue Mar 11, 2016 · 4 comments · Fixed by #10965
Labels

Comments

@GraionDilach
Copy link
Contributor

As my recent modding experiments uncovered it, Requires<> turned out to be bogus. If the required trait has multiple instances on the actual actor and the trait which is required is placed between them, Requires<> will not force the followup instances to be loaded before the trait which requires them.

Testcase in bleed TS: add AutoTarget between the two Attack* traits to the Tick Tank (ttnk). The game won't crash. Add it to the end of the unit and the game will crash due to AutoTarget is still incapable of handling more Attack* traits. AutoTarget requires AttackBase.

@abcdefg30 abcdefg30 added this to the Next release milestone Mar 11, 2016
@RoosterDragon
Copy link
Member

To enforce this, you just need to strengthen the rules:

This line determines which traits met their requirements. Currently if one or more dependencies are present to satisfy a dependant, that is sufficient.

You are asking for all of the dependencies to be listed before the dependant. This can be achieved with an extra condition:

var more = unresolved.Where(u => u.Dependencies.All(d =>
    resolved.Exists(r => testResolve(d, r.Type)) &&
    !unresolved.Any(u1 => testResolve(d, u1.Type)))); // This condition here.

This causes some mods to fail to load though, as they can't meet this constraint. In both cases, UpgradeManagerInfo expects all upgradable traits to be listed first, but some upgradable traits have an indirect requirement for the UpgradeManager to be defined first.

This probably indicates the existing setups for these mods are somewhat kinda buggy, but until they are fixed I'm not sure what to do about them. This indicates this issue is probably not a regression, though it is perhaps a bug.

D2k

ActorInfo("siege_tank") failed to initialize because of the following:

Unresolved:
OpenRA.Mods.Common.Traits.UpgradeManagerInfo: { OpenRA.Mods.Common.Traits.IUpgradableInfo }
OpenRA.Mods.Common.Traits.GainsExperienceInfo: { OpenRA.Mods.Common.Traits.UpgradeManagerInfo }
OpenRA.Mods.D2k.Traits.CarryableInfo: { OpenRA.Mods.Common.Traits.UpgradeManagerInfo }
OpenRA.Mods.D2k.Traits.WithDecorationCarryableInfo: { OpenRA.Mods.D2k.Traits.CarryableInfo }

due to

public class WithDecorationCarryableInfo : WithDecorationInfo { }
public class WithDecorationInfo : UpgradableTraitInfo { }
public abstract class UpgradableTraitInfo : IUpgradableInfo { }

RA

ActorInfo("pbox") failed to initialize because of the following:

Unresolved:
OpenRA.Mods.Common.Traits.UpgradeManagerInfo: { OpenRA.Mods.Common.Traits.IUpgradableInfo }
OpenRA.Mods.Common.Traits.TimedUpgradeBarInfo: { OpenRA.Mods.Common.Traits.UpgradeManagerInfo }
OpenRA.Mods.Common.Traits.AutoTargetInfo: { OpenRA.Mods.Common.Traits.AttackBaseInfo }
OpenRA.Mods.Common.Traits.RenderRangeCircleInfo: { OpenRA.Mods.Common.Traits.AttackBaseInfo }
OpenRA.Mods.Common.Traits.CargoInfo: { OpenRA.Traits.IOccupySpaceInfo, OpenRA.Mods.Common.Traits.UpgradeManagerInfo }
OpenRA.Mods.Common.Traits.AttackGarrisonedInfo: { OpenRA.Mods.Common.Traits.CargoInfo }

due to

public abstract class AttackBaseInfo : UpgradableTraitInfo { }
public abstract class UpgradableTraitInfo : IUpgradableInfo { }

@GraionDilach
Copy link
Contributor Author

It wasn't me who claimed this being a regression but @pchote - http://logs.openra.net/?year=2016&month=03&day=09#12:14:03. In both of your example cases I'd put the blame on other traits: Cargo and Carryable requiring UpgradeManager, which is more likely to be the failure causes (although I guess D2k will break more, since all vehicles are carryable and that was just the first case.

@pchote pchote removed this from the Next release milestone Mar 12, 2016
@RoosterDragon
Copy link
Member

I agree that the current behaviour seems like a bug - but it seems like we'll need to work out a fix for the default mods first. Something like #8848 might be needed for that though. Blugh.

@Mailaender Mailaender added the Bug label Mar 14, 2016
@RoosterDragon
Copy link
Member

I haven't thought this through fully, but perhaps we can resolve the circular dependency in UpgradeMananger by having it do all the required trait lookup in Created instead and removing the requires for it.

If that's feasible, that would seem to incur the least amount of work..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants