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.

Sign up for GitHub

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

Jump to bottom

Backporting Garrison logic from romanovs-vengeance #681

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Backporting Garrison logic from romanovs-vengeance #681

wants to merge 12 commits into from

Conversation

Valkirie
Copy link

@Valkirie Valkirie commented Mar 19, 2020 •
edited
Loading

Backported all the Garrison logic from romanovs-vengeance, without custom content. Right now, e1 (GI) and e2 (Conscript) can garrison.
Should address issue #204.

Author: Devon Dieffenbach (darky)
Credit: DnAp (https://github.com/DnAp) - Original PR: OpenRA/OpenRA#12774

TODO:

  • 1:1 backport from Romanovs-vengeance.
  • Fix the fire animation.
  • Fix the fire sound.
  • Don't duplicate traits (AttackGarrisonRV has to be merged with AttackGarrison).

Lesueur Benjamin added 10 commits March 19, 2020 11:06
New Trait - Garrison
d8b3451 
Author: Devon Dieffenbach (darky)
Credit: DnAp (https://github.com/DnAp) - Original PR: OpenRA/OpenRA#12774
Some changes to garrison
9a1a898 
Adds garrison art.
Make some more buildings garrisonable.
Adjusts garrison amount to match original. (McBurger Kong and McRoo don'
match in original for some reason, i made Kong have 10 too.)
Add proper structure garrisoned/abandoned notification.
Complete garrisoning.
0700353
Complete garrisoning.
cc973c2
Make Garrisoned Structures auto target in defensive stance.
a397c47
Make Garrisonable Pauseable.
220ae25
Fix some civ buildings using snow garrisoned version.
79ac06b
Merge branch 'master' into master
d86a32d
Update civilian-structures.yaml
4dfcf15 
Removed -rubble on ^GarrisonableBuilding
Update soviet-infantry.yaml
ba8763a 
flakt and shk shouldn't be Garrisonable.
@Mailaender
Copy link
Member

./utility.sh --check-sequence-sprites
Tileset: Temperate
File not found: true.shp
File not found: true.shp
File not found: castl04.shp
File not found: carus01.shp
File not found: cachig04.shp
Tileset: Snow
File not found: true.shp
File not found: true.shp
File not found: castl04.shp
File not found: carus01.shp
File not found: cachig04.shp
Tileset: Urban
File not found: true.shp
File not found: true.shp
File not found: castl04.shp
File not found: carus01.shp
File not found: cachig04.shp

@Mailaender
Copy link
Member

This works surprisingly well in-game. To properly attribute someone elses code: use git commit -m "New Trait - Garrison " --author "Devon Dieffenbach <[email protected]>" on the first commit.

@Mailaender Mailaender added the PR: Fixup requested A minor change is required label Mar 19, 2020
Mailaender
Mailaender reviewed Mar 19, 2020
View reviewed changes
OpenRA.Mods.RA2/Traits/AttackGarrisonedRV.cs
}

[Desc("Garrisoners can fire their weapons out of fire ports.")]
public class AttackGarrisonedRVInfo : AttackFollowInfo, IRulesetLoaded, Requires<GarrisonableInfo>
Copy link
Member

@Mailaender Mailaender Mar 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't duplicate traits. Submit your changes to them first instead: https://github.com/OpenRA/OpenRA/blob/bleed/OpenRA.Mods.Common/Traits/Attack/AttackGarrisoned.cs

Copy link
Author

@Valkirie Valkirie Mar 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really that was a 1:1 port here. It's been way too long (years) since I have really looked into openRA. I have no idea how much impact replacing CargoInfo with GarrisonableInfo would cause here or how much energy not having what almost looks like a dupe Trait would be.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. https://gist.github.com/Mailaender/accd6464cb1b7a12ccad5d75256f1da2/revisions In that case the whole pull request should probably move to https://github.com/OpenRA/OpenRA.

Copy link
Author

@Valkirie Valkirie Mar 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They rejected the whole PR already... Right now openRA doesn't support multiple cargo types.

While not perfect I think it works quite fine already and keeping it RA2 centric right now allow us to move forward.

Copy link
Member

@Mailaender Mailaender Mar 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... Duplication isn't good. Rename it to AttackFromGarrison or something then at least.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surely I can rename it but I just wanted to let you know I might not have the engine knowledge to do any better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also can't think of a clever way of using inheritance. This might be the only way to go.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's next then ? What would you like to do and how ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pull request seems to differ from what was initially submitted at OpenRA/OpenRA#12774 so I suggest a new attempt at the engine project. You can rebase a copy to playtest-20200118 type in the commit into https://github.com/OpenRA/ra2/blob/master/mod.config#L7 and keep a test case with the .yaml changes here. See some of my pull requests with Engine Update Required as reference.

@sorcerer86pt
Copy link

Travis build failed due to :

OpenRA.Utility(1,1): Error: Actor type `cahse01` consumes conditions that are not granted: dmg_crit, chronodisable, build-incomplete
OpenRA.Utility(1,1): Error: Actor type `cahse02` consumes conditions that are not granted: dmg_crit, chronodisable, build-incomplete
OpenRA.Utility(1,1): Error: Actor type `cahse03` consumes conditions that are not granted: dmg_crit, chronodisable, build-incomplete
OpenRA.Utility(1,1): Error: Actor type `cahse04` consumes conditions that are not granted: dmg_crit, chronodisable, build-incomplete
...

Lesueur Benjamin added 2 commits March 19, 2020 14:57
Fixed bad indentions
49f7336
Update Garrisoner.cs
c69847b 
Removed useless else condition.
@penev92
Copy link
Member

penev92 commented Mar 22, 2023

Is it fair to say this one is dead?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mailaender Mailaender Mailaender left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
PR: Fixup requested A minor change is required PR: Rebase me
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Valkirie @Mailaender @sorcerer86pt @penev92