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

Soviet Soldier Volkov & Chitzkoi fully ported #15549

Merged
merged 1 commit into from Sep 28, 2018

Conversation

Projects
None yet
6 participants
@AngryBirdz
Copy link
Contributor

AngryBirdz commented Aug 21, 2018

Hi there, I've just completed my porting of the Counterstrike expansion mission named "Soviet Soldier Volkov & Chitzkoi" (scu35ea). The map is fully operational but it might need some playtest by someone else than myself just in case there's still some bugs and / or balance issue that I did'nt noticed.

Have a nice day :)

Inherits: DogJaw
Range: 5c512
Warhead@1Dam: SpreadDamage
-InvalidTargets: Ant

This comment has been minimized.

@reaperrr

reaperrr Aug 21, 2018

Contributor

There aren't any ants in this mission, so these 2 lines shouldn't be necessary.

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Aug 21, 2018

It looks like our lua syntax checker can't handle the '&' in the folder name, maybe just write 'and' instead.

It would also be nice if you could list the lua scripts in https://github.com/OpenRA/OpenRA/blob/bleed/OpenRA.sln, some reviewers might use that for debugging.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Aug 21, 2018

The & character has special meaning (put job in background) on Linux and macOS, and so can't be used unescaped in file paths. Yes, please replace it with and or something else.

@AngryBirdz

This comment has been minimized.

Copy link
Contributor

AngryBirdz commented Aug 21, 2018

Alright guys I did everything you asked and I think I did'nt forget anything.

UPDATE
-Added myself in the authors file (I forgot the first time)
-Removed the special character in the filename
-Listed my lua scripts in OpenRA.sln
-Removed two useless line in weapons.yaml (just a leftover of me fooling around with some stuff)

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Aug 21, 2018

From Travis error checks:

OpenRA.Utility(1,1): Error: Allies contains player Goodguy that is not in list.

@AngryBirdz AngryBirdz referenced this pull request Aug 21, 2018

Open

Add Red Alert campaign #4989

6 of 8 tasks complete
@AngryBirdz

This comment has been minimized.

Copy link
Contributor

AngryBirdz commented Aug 21, 2018

Can you explain to me why this has happened? I seriously don't know. I double checked my code and the comp "Goodguy" is listed both in the map.yaml and the lua script. What did I miss?

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Aug 21, 2018

Player names are case sensitive. See GoodGuy vs Goodguy.

AUTHORS Outdated
@@ -104,6 +104,7 @@ Also thanks to:
* Michael Rätzel
* Michael Silber (frühstück)
* Michael Sztolcman (s1w_)
* Mike Gagné (AngryBirdz)

This comment has been minimized.

@pchote

pchote Aug 21, 2018

Member

Please use 4 spaces here instead of a tab.

Prerequisites: ~syrd, ~techlevel.high

C2:
Inherits@AUTOTARGET: ^AutoTargetGroundAssaultMove

This comment has been minimized.

@pchote

pchote Aug 21, 2018

Member

Have you tried defining Inherits@2: ^ArmedCivilian instead? This would save a lot of duplication.

This comment has been minimized.

@AngryBirdz

AngryBirdz Aug 21, 2018

Contributor

Yeah I'll upload the new file soon

Range: 10c0
Demolition:
Mobile:
-Voice: Move

This comment has been minimized.

@pchote

pchote Aug 21, 2018

Member

Why do these voices need to be removed?

This comment has been minimized.

@AngryBirdz

AngryBirdz Aug 21, 2018

Contributor

Because these voices can't be used with the GenericVoice voiceset. It make the game crash.

This comment has been minimized.

@reaperrr

reaperrr Aug 21, 2018

Contributor

It would be better to set them to Voice: Action then.

@Mailaender

This comment has been minimized.

Copy link
Member

Mailaender commented Sep 10, 2018

A very minor issue:

image

The mines display the multiplayer user name on hover.

@Mailaender

This comment has been minimized.

Copy link
Member

Mailaender commented Sep 10, 2018

Another polishing issue:

image

The lab (and hospital) still shows it's multiplayer function in the tooltip.

local Rally = Utils.Random(AlliedWarFactRally)
Utils.Do(AlliedWarFact, function(fact)
fact.RallyPoint = Rally.Location
end)

This comment has been minimized.

@Mailaender

Mailaender Sep 10, 2018

Member

Indention look wrong here.

elseif (OreTruck01.IsDead and OreTruck02.IsDead) and greece.Resources <= 2399 then
return
end
NavalYard01.RallyPoint = waypoint26.Location

This comment has been minimized.

@Mailaender

Mailaender Sep 10, 2018

Member

You seem to be using a mix of tabs and spaces for indention. Please just use tabs.

@Mailaender
Copy link
Member

Mailaender left a comment

Looking good otherwise. Please squash all fixup commits together when you are done.

@AngryBirdz

This comment has been minimized.

Copy link
Contributor

AngryBirdz commented Sep 12, 2018

I'm a noob at this. Can you explain to me what this "PR: Needs +2" label mean? Did I forgot to do something?

@Smittytron

This comment has been minimized.

Copy link
Contributor

Smittytron commented Sep 12, 2018

OpenRA requires two approval reviews to merge a PR. The tag just means there's one approval.

@AngryBirdz

This comment has been minimized.

Copy link
Contributor

AngryBirdz commented Sep 12, 2018

Ok thx for the info.

@abcdefg30
Copy link
Member

abcdefg30 left a comment

The tooltip of the alloy factory needs to be hidden/adjusted:
grafik

Otherwise, well done! The mission plays nicely ingame. Only issue I had: The allied forces often only damage the concrete walls of your base and you still get a "Our base is under attack" warning which I found to be a bit annoying/distracting (for just concrete...). Do you think this should be changed?

Name: Creeps
NonCombatant: True
Faction: england
Enemies: Greece, GoodGuy, Spain, France, USSR

This comment has been minimized.

@abcdefg30

abcdefg30 Sep 14, 2018

Member

This still uses spaces for indentation.

This comment has been minimized.

@AngryBirdz

AngryBirdz Sep 18, 2018

Contributor

For the tooltip description i have two questions:

  1. How do you make it show ingame? Can't find the setting.
  2. Do you know wich trait point to it? Because I'm having a hard time finding it.
    Thx!

This comment has been minimized.

@abcdefg30

abcdefg30 Sep 18, 2018

Member

The tooltip should just appear when you hover with your mouse over the building. The trait is TooltipDescription iirc.

DefaultCash: 0
LobbyPrerequisiteCheckbox@GLOBALBOUNTY:
Enabled: False
Locked: True

This comment has been minimized.

@abcdefg30

abcdefg30 Sep 14, 2018

Member

I think this should live in campaign-rules.yaml, ideally.

This comment has been minimized.

@AngryBirdz

AngryBirdz Sep 18, 2018

Contributor

Are you sure that LobbyPrerequisiteCheckbox@GLOBALBOUNTY is pre-defined in campaign-rules.yaml? Because I don't see it.

This comment has been minimized.

@pchote

pchote Sep 18, 2018

Member

@abcdefg30 Do we need it? Bounties default to off in the default game rules.

This comment has been minimized.

@abcdefg30

abcdefg30 Sep 18, 2018

Member

No, we don't necessarily need this. However, we usually lock the options in the campaign missions.

Show resolved Hide resolved mods/ra/maps/soviet-soldier-volkov-n-chitzkoi/scu35ea-AI.lua Outdated
Show resolved Hide resolved mods/ra/maps/soviet-soldier-volkov-n-chitzkoi/scu35ea-AI.lua Outdated
Show resolved Hide resolved mods/ra/maps/soviet-soldier-volkov-n-chitzkoi/scu35ea-AI.lua Outdated
Show resolved Hide resolved mods/ra/maps/soviet-soldier-volkov-n-chitzkoi/scu35ea-AI.lua Outdated
Show resolved Hide resolved mods/ra/maps/soviet-soldier-volkov-n-chitzkoi/scu35ea-AI.lua Outdated
Show resolved Hide resolved mods/ra/maps/soviet-soldier-volkov-n-chitzkoi/scu35ea.lua Outdated
Show resolved Hide resolved mods/ra/maps/soviet-soldier-volkov-n-chitzkoi/scu35ea.lua Outdated
ProduceInfantry() --Greece will start infantry production once this unit is dead just like in the original mission
end)

--Kill the ore trucks when the two allied refinery are destroyed

This comment has been minimized.

@abcdefg30

abcdefg30 Sep 14, 2018

Member

Why is this necessary? (It looks a bit odd to me.)

This comment has been minimized.

@AngryBirdz

AngryBirdz Sep 18, 2018

Contributor

The vanilla mission work that way.

This comment has been minimized.

@abcdefg30

abcdefg30 Sep 18, 2018

Member

Can you please remove it (still)? I would consider that an improvement over the original mission. (It does not make sense to me why the ore trucks should blow up. As player, I would probably be rather confused by this.)

@AngryBirdz AngryBirdz force-pushed the AngryBirdz:bleed branch from 1c45aa1 to e12d4f8 Sep 19, 2018

@AngryBirdz

This comment has been minimized.

Copy link
Contributor

AngryBirdz commented Sep 20, 2018

I tried to fix the Tooltip Description with the trait you told me to mod but the check failed somehow?

GenericName: Hospital
GenericVisibility: Enemy, Ally, Neutral
GenericStancePrefix: false
TooltipDescription: Destroy this hospital to get healing equipment.

This comment has been minimized.

@Mailaender

Mailaender Sep 20, 2018

Member

This is indented wrong. TooltipDescription is it's own trait:

	TooltipDescription:
		Description: Destroy this hospital to get healing equipment.

should work.

This comment has been minimized.

@AngryBirdz

AngryBirdz Sep 20, 2018

Contributor

notworking
Not sure if by its own trait you mean outside of tooltip but if I'm doing it like that the game wont even load the map.

working
On the other hand if I indent it like you told me but inside tooltip the map load. Can you confirm this will work before I update the commit? Thanks you.

This comment has been minimized.

@pchote

pchote Sep 20, 2018

Member

TooltipDescription is a different trait to Tooltip. When you indent the definition as in your second example the game tries to load a TooltipDescription field on the Tooltip trait, which does not exist and is therefore ignored.

The lint check (which you can view from the "Details" link in the "Some checks were not successful" panel below) reports this more explicitly:

OpenRA.Utility(1,1): Error: rules.yaml:75 refers to a trait field `TooltipDescription` that does not exist on `Tooltip`.
OpenRA.Utility(1,1): Error: rules.yaml:101 refers to a trait field `TooltipDescription` that does not exist on `Tooltip`.

This comment has been minimized.

@Mailaender

Mailaender Sep 20, 2018

Member

Not sure if by its own trait you mean outside of tooltip but if I'm doing it like that the game wont even load the map.

How do you test this map? Your local OpenRA installation or this branch compiled from source?

This comment has been minimized.

@AngryBirdz

AngryBirdz Sep 20, 2018

Contributor

My local OpenRA installation.

This comment has been minimized.

@pchote

pchote Sep 20, 2018

Member

Is your local installation release-20180307? The TooltipDescription trait was added after that release - make sure you are testing on the latest playtest, or, ideally, the development branch.

GenericName: Alloy Facility
GenericVisibility: Enemy, Ally, Neutral
GenericStancePrefix: false
TooltipDescription: This facility produce our stolen armor plating.

This comment has been minimized.

@Mailaender

Mailaender Sep 20, 2018

Member

Same here.

This comment has been minimized.

@abcdefg30

abcdefg30 Sep 20, 2018

Member

While you are at it, please change produce to produces.

@AngryBirdz AngryBirdz force-pushed the AngryBirdz:bleed branch from e12d4f8 to 28f56ac Sep 21, 2018

@AngryBirdz

This comment has been minimized.

Copy link
Contributor

AngryBirdz commented Sep 25, 2018

I've managed to fix it by using the playtest build. Just tell me if I forgot something.

end
end)
if Map.LobbyOption("difficulty") == "hard" then
Trigger.ClearAll(RiflemanGuard01)

This comment has been minimized.

@abcdefg30

abcdefg30 Sep 26, 2018

Member

This crashes when RiflemanGuard01 is already dead.

@AngryBirdz AngryBirdz force-pushed the AngryBirdz:bleed branch from 28f56ac to 205531d Sep 26, 2018

@AngryBirdz

This comment has been minimized.

Copy link
Contributor

AngryBirdz commented Sep 27, 2018

Ok I've fixed the crash related to RiflemanGuard01. Tell me if there's something else.

@abcdefg30 abcdefg30 merged commit 6194578 into OpenRA:bleed Sep 28, 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 Sep 28, 2018

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