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

Add RequiresSpecificOwner trait and lint check #15461

Merged
merged 3 commits into from Nov 21, 2018

Conversation

Projects
None yet
5 participants
@reaperrr
Copy link
Contributor

reaperrr commented Aug 4, 2018

The purpose of this trait is to allow Lint and the map editor to check for and enforce specific owner(s) for actors where any other owner could cause issues (case in point: #15457).

In practice, this is only used to enforce Neutral on RA/TD trees for now, because their Targetable trait makes AI squads consider them as valid targets when owned by the 'enemy' Creeps.
The fact that the AI only looks for the Targetable trait and not whether the target type is actually valid for the squad leader is certainly another issue, but this PR has its own merits regardless of that.

@reaperrr reaperrr added this to the Next + 1 milestone Aug 4, 2018

@reaperrr reaperrr force-pushed the reaperrr:lint-owner branch 2 times, most recently from 3fdc296 to ce0f5ef Aug 4, 2018

@reaperrr reaperrr force-pushed the reaperrr:lint-owner branch from ce0f5ef to 6b31ba1 Aug 22, 2018

@reaperrr reaperrr changed the title Add RequiresSpecialOwner trait and lint check Add RequiresSpecificOwner trait and lint check Aug 22, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Aug 22, 2018

Updated and rebased.

@Mailaender
Copy link
Member

Mailaender left a comment

Looking good.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Nov 18, 2018

What's the status here @reaperrr?

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Nov 19, 2018

  1. Query the rules for all actors with the RequiresSpecificOwnerInfo traitinfo
  2. Build a dictionary of actor name → required owner
  3. Enumerate actor definitions
  4. If actor type exists in the dictionary parse the owner and check that it matches

None of these steps are are so 100% clear to me that I wouldn't first have to get my head around them and look for probable examples, and then trial and error my way through them. That's always a somewhat time-consuming and tiresome process, so I haven't found the motivation for it yet.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Nov 19, 2018

Ok. I can put a prototype together for that if you'd like.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Nov 19, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Nov 20, 2018

Updated with ^ prototype (with tiny adjustment to lint error message, and removal of hardcoded mpspawn check split to separate commit as discussed on IRC).

@reaperrr reaperrr force-pushed the reaperrr:lint-owner branch 2 times, most recently from b531958 to ebcfb6a Nov 20, 2018

Show resolved Hide resolved mods/ra/rules/defaults.yaml Outdated
Show resolved Hide resolved OpenRA.Mods.Common/Traits/RequiresSpecificOwners.cs Outdated

reaperrr added some commits Aug 3, 2018

Add RequiresSpecificOwners trait
To enforce specific owners via Lint rules,
and possibly other means later.
This is for cases where accidentally setting an
unfitting owner via editor could cause issues.

Example: AI might try to attack Creeps-owned trees
and get stuck.
Enforce required owner in map editor
It can easily happen that mappers forget to set the
current player to Neutral before placing more trees,
for example, so we force the editor to set a valid owner.
Remove hardcoded mpspawn owner lint check
Use RequiresSpecificOwner to enforce the owner
that owns the world instead.

Require 'Neutral' in the official mods accordingly.

@reaperrr reaperrr force-pushed the reaperrr:lint-owner branch from ebcfb6a to 409b0cc Nov 21, 2018

Updated

@pchote

pchote approved these changes Nov 21, 2018

@pchote pchote merged commit 10e51db into OpenRA:bleed Nov 21, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment