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

Rename EmitInfantryOnSell and remove default actor type. #15171

Merged
merged 1 commit into from Jun 2, 2018

Conversation

Projects
None yet
5 participants
@matjaeck
Copy link
Contributor

matjaeck commented May 25, 2018

To fix #15077 and probably close #15161. I tested adding "ActorTpes" for the case that only the default is used (our mods do not use the default anywhere) and it seemed to work.

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented May 25, 2018

@lawando I guess all the oramaps were showing up as having been "updated" by the update rule, but for oramap files that's actually a false alarm caused by the tool touching the file, even when no changes are applied.
Any maps saved as .oramap never contain custom rules (in our official mods, at least), so it's safe to discard the "updated" oramaps from this PR.

@matjaeck

This comment has been minimized.

Copy link
Contributor Author

matjaeck commented May 25, 2018

Updated. Removed maps and renamed to "SpawnActorOnSell".

{
get
{
return "The EmitInfantryOnSell trait was renamed to SpawnActorOnSell and the default \n" +

This comment has been minimized.

@GraionDilach

GraionDilach May 25, 2018

Contributor

You don't need the line-ending spaces in the output.

This comment has been minimized.

@matjaeck

matjaeck May 25, 2018

Author Contributor

Fixed.

@GraionDilach
Copy link
Contributor

GraionDilach left a comment

👍

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented May 30, 2018

Needs a rebase now.

@matjaeck

This comment has been minimized.

Copy link
Contributor Author

matjaeck commented May 30, 2018

Rebased.

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented May 30, 2018

ActorTypes still has e1 as internal default. When you remove it, don't forget to remove the reference to it in the description as well.
I also think it then makes sense to add [FieldLoader.Require] to the property, to ensure that modders don't accidentally forget to define any actors.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented May 30, 2018

@reaperrr I think the idea was to leave that part to #15161 so as to not close a contributors first PR

@matjaeck

This comment has been minimized.

Copy link
Contributor Author

matjaeck commented May 30, 2018

Updated, removed the default actor type.

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented May 31, 2018

Needs another rebase now, sorry.

@matjaeck

This comment has been minimized.

Copy link
Contributor Author

matjaeck commented May 31, 2018

Rebased, also included FieldLoader.Require and added Actor type to spawn on sell. to the description (like it is in SpawnActorOnDeath).

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented May 31, 2018

One last thing I unfortunately only noticed now: unlike SpawnActorOnDeath, this trait can spawn multiple actors at once, so "SpawnActorsOnSell" would be more accurate.

Regarding the discussion on IRC about ActorTypes vs. Actors: We have some cases of both and not really decided on a convention yet, so maybe it's better to leave it as it is, and then later fix all property names across all affected traits in one fell swoop once we've decided on a convention.

@matjaeck

This comment has been minimized.

Copy link
Contributor Author

matjaeck commented Jun 1, 2018

Updated.

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Jun 2, 2018

I consider Graion's +1 still valid since the changes made since his review were minor, so LGTM.

@reaperrr reaperrr merged commit 7dd6444 into OpenRA:bleed Jun 2, 2018

2 checks passed

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

This comment has been minimized.

Copy link
Contributor

reaperrr commented Jun 2, 2018

@matjaeck matjaeck deleted the matjaeck:eiosupdate branch Jun 2, 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.