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

Add a time limit lobby option and Lua API #16317

Merged
merged 9 commits into from May 16, 2019

Conversation

@obrakmann
Copy link
Contributor

commented Mar 16, 2019

Works towards closing #10367

@obrakmann obrakmann added this to the Next Release milestone Mar 16, 2019

@pchote

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

Looks like this partially conflicts with #16277, so would you mind giving that one a review and then we can decide which one to adapt to suit the other?

@obrakmann obrakmann force-pushed the obrakmann:timelimit branch from 018923a to 32bf31b Mar 17, 2019

@obrakmann obrakmann force-pushed the obrakmann:timelimit branch 3 times, most recently from f4e8506 to 4ecbb00 Mar 28, 2019

@pchote
Copy link
Member

left a comment

Sorry for only getting around to this the day after your free time ended...

Some thoughts:

@obrakmann obrakmann force-pushed the obrakmann:timelimit branch 2 times, most recently from e116c1d to 5415a8e Apr 20, 2019

@obrakmann

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2019

Updated.

@reaperrr

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2019

OpenRA.Mods.Common/Traits/Player/TimeLimitManager.cs:L150: [SA1008] Invalid spacing around the opening parenthesis.

@obrakmann obrakmann force-pushed the obrakmann:timelimit branch 3 times, most recently from ec37af5 to c1d7870 Apr 20, 2019

@pchote
Copy link
Member

left a comment

This is looking really good :)

A few more requests, then I think this will be good to merge:

@obrakmann obrakmann force-pushed the obrakmann:timelimit branch from c1d7870 to 7e8177c Apr 21, 2019

@pchote
Copy link
Member

left a comment

Two final polish nits, then LGTM feature-wise.

We'll also need to disable or remove the time limit from the scripted maps / minigames where it doesn't make sense, but i'm okay with this being deferred to a followup PR if you don't have the time or interest to do it here.

I'll file a separate PR to tweak the option bins layout so that we can fit all the default options without scrolling (it is just possible).

OpenRA.Mods.Common/Traits/Player/TimeLimitManager.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/Player/TimeLimitManager.cs Outdated Show resolved Hide resolved
@obrakmann

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2019

Rebased and updated. Added a commit to disable the timelimit dropdown for Fort Lonestar and the MP missions.

@pchote

This comment has been minimized.

Copy link
Member

commented May 11, 2019

Aah, you pushed over my fixups!

Can you please refix the two comments that I just unresolved above (the LuaExceptions and displayTick)

@obrakmann obrakmann force-pushed the obrakmann:timelimit branch from 26a1959 to 2edf733 May 12, 2019

@obrakmann

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2019

Sorry, I forgot about that. I basically never fetch from my own repo, so I just updated my local branch. Re-added the fixes, hope it's ok now.

@pchote
Copy link
Member

left a comment

Found a crash while testing 😢

This otherwise is looking great, reconfirming my 👍 once this is fixed. Lets get this finally merged!

OpenRA.Mods.Common/Traits/Player/TimeLimitManager.cs Outdated Show resolved Hide resolved

@obrakmann obrakmann force-pushed the obrakmann:timelimit branch from 2edf733 to 0bfc5b1 May 12, 2019

@abcdefg30
Copy link
Member

left a comment

A few other minor nits (only really need to fix the last one, but the others are nice to have):

  • It appears that we make disabled options on Fort Lonestar invisible.
  • When the time limit in a mission runs out, the timer jumps back to the real game time. This is probably needed for non-game ending timers, but we might want to think about preventing that for missions where it does end the game (like top o the world).
  • Timers in skirmish end the game at 00:-01.

@abcdefg30 abcdefg30 force-pushed the obrakmann:timelimit branch from 0bfc5b1 to a80b46d May 16, 2019

@abcdefg30

This comment has been minimized.

Copy link
Member

commented May 16, 2019

Allied reinforcements have arrived from the south east: Rebased and updated.

Unfortunately, there is still one problem remaining: What if two players have the exact same amount of points? It currently will just let one player win.

Code changed.

@pchote

This comment has been minimized.

Copy link
Member

commented May 16, 2019

Unfortunately, there is still one problem remaining: What if two players have the exact same amount of points? It currently will just let one player win.

IMO this is fine, because its almost never going to happen.
If you disagree, the simplest tiebreak to implement would be current cash+resources.

@abcdefg30 abcdefg30 force-pushed the obrakmann:timelimit branch from a80b46d to 1b51451 May 16, 2019

@pchote
pchote approved these changes May 16, 2019

@abcdefg30 abcdefg30 merged commit 215af20 into OpenRA:bleed May 16, 2019

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

commented May 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.