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

Added Scott's Allies04 co-op multiplayer mission as "Infiltration" #11702

Merged
merged 3 commits into from
Sep 4, 2016

Conversation

Mailaender
Copy link
Member

@Mailaender Mailaender commented Jul 24, 2016

@ScottNZ's original mission converted from the C# mission code found in release-20131223. My favorite one as it tells a good story without relying on too much text and has a nice attention to detail. The intro sequence is very hard, but it works well in co-operative settings with one player already guiding the newbie. It is also exciting (and hilarious) for spectators to watch.

Regarding features it was way ahead of it's time, but relied a lot on non-reusable hacks and suffered from missing engine features. It even had civilians walking around randomly which we just introduced in #11520 for all maps and mods. I managed to replace all expensive checks with triggers and made use of the global palette effect from #7863 to create the dawn lighting.

This should be very faithful to the original. The only behavioral change I introduced is to let the spy "hotwire" (externally capture) the truck. Afterwards all spies can enter regularly. This avoids fragile script workarounds. Capturing would destroy the spy actor and LoadPassenger will duplicate it, leading to an inconsistent world state.

@Mailaender Mailaender mentioned this pull request Jul 24, 2016
2 tasks
@pchote
Copy link
Member

pchote commented Jul 24, 2016

We used to have logic in place that lets a spy infiltrate an actor without being destroyed.

@@ -207,6 +207,13 @@ public void Debug(string text)
Game.Debug(text);
}

[Desc("Write a message into debug.log")]
public void DebugLog(string text)
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this kind of redundant to the normal Debug (see method above)?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, that uses Game.Debug which displays it in-game. If you want to dump a large amount of data the chat log would get spammed and you can't search it.

Copy link
Member

Choose a reason for hiding this comment

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

Isnt this redundant to the print builtin that we overrode for this purpose?

Copy link
Member

Choose a reason for hiding this comment

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

As discussed in IRC, please remove this and use print() instead.

@abcdefg30
Copy link
Member

abcdefg30 commented Jul 24, 2016

I think we should mention that you need to capture a supply truck. Maybe in the description with just something like The best way to get in for our spies is to hijack a supply truck.?

@abcdefg30
Copy link
Member

Or even add an extra objective for it.


LST.Unselectable.UnloadOnly:
Inherits: LST
Buildable:
Copy link
Member

Choose a reason for hiding this comment

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

What is this for?

Copy link
Member

Choose a reason for hiding this comment

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

Seems like you forgot to add Prerequisites: ~disabled below.

@Mailaender Mailaender force-pushed the infiltration-mission branch 2 times, most recently from 227aef3 to 98bdbf6 Compare July 24, 2016 16:07
@Mailaender
Copy link
Member Author

Fixed all the remarks.

@@ -59,6 +59,9 @@ public Player Owner

set
{
if (value == null || !(value is Player))
Copy link
Member

Choose a reason for hiding this comment

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

Do you have a repro case of this being called with !(value is Player)? That shouldn't be possible.

Copy link
Member Author

@Mailaender Mailaender Jul 24, 2016

Choose a reason for hiding this comment

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

This is a wild guess to be honest. Even the null check. You suggested on IRC to add this. I can't really prove this fixes anything and I believe the Lua bridge will crash on a lower level part if you throw wrong parameters on it "can't convert stupid scripter typo without naming even the variable just telling `nil was received to LuaValue in line number 30 in sandbox.lua". I also tried to improve the stacktrace a bit in those situations as it is really annyoing, but failed. Will remove this and hope for @Unit158 to finish #9086.

@obrakmann
Copy link
Contributor

Here you'll want to set the Cooperative flag on MissionObjectives as well, for the same reason as in Allies03.

@Mailaender
Copy link
Member Author

Mailaender commented Aug 1, 2016

I had trouble with the Cooperative flag on #11679 as it doesn't seem to work well with shared/duplicated objectives, which is the case here as well I assume.

@obrakmann
Copy link
Contributor

What problems exactly? It should just work.

@obrakmann obrakmann added this to the Next release milestone Aug 25, 2016
public SellableProperties(ScriptContext context, Actor self)
: base(context, self)
{
sellable = self.Trait<Sellable>();
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't really need to cache this, it will only be used once for every actor.

Copy link
Member

Choose a reason for hiding this comment

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

What if we sell many actors at once? Wouldn't that create a tiny lag without caching?
What I actually want to says, now that we already cache it, do we really want to revert that back?

Copy link
Member

Choose a reason for hiding this comment

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

Arguing over micro-optimizations is a bit silly, but I think its fair to say that an actor will be sold on average zero times (or at least much closer to zero than one). Caching it here means doing this lookup for all buildings, but doing it in Sell means doing it only on the buildings that you sell.

Copy link
Member

Choose a reason for hiding this comment

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

Ah well. Then nvm my comment.

@obrakmann
Copy link
Contributor

Looks otherwise fine so far. The mission is fucking annoying in the beginning, though, with all the dogs around and you having no idea where to go. I eventually looked at the map in the editor and then knew where I had to go, but still never got there due to the dogs. How about removing some on easier skill levels?

@abcdefg30
Copy link
Member

How about removing some on easier skill levels?

We could also add cameras, like in allies05a on easier difficulties.

@abcdefg30
Copy link
Member

I left my remarks at Mailaender#59.

@Mailaender
Copy link
Member Author

Thanks. Squashed them in.

@abcdefg30
Copy link
Member

Looks good to me now (after travis is fixed). 👍 / ✅

@abcdefg30
Copy link
Member

Mailaender#60 fixes travis.

@Mailaender
Copy link
Member Author

And another crash fixed Mailaender#61. Thanks.


Utils.Do(sovietbuildings, function(sovietbuilding)
Trigger.OnDamaged(sovietbuilding, function(building)
if building.Health < building.MaxHealth * 3/4 then
Copy link
Contributor

@obrakmann obrakmann Sep 4, 2016

Choose a reason for hiding this comment

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

This needs an owner check. if building.Owner ~= soviets then return

@obrakmann
Copy link
Contributor

👍 after those things have been addressed

@Mailaender
Copy link
Member Author

Updated.

@obrakmann obrakmann merged commit a32a95a into OpenRA:bleed Sep 4, 2016
@obrakmann
Copy link
Contributor

Changelog

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

Successfully merging this pull request may close these issues.

4 participants