#3234 closed enhancement (fixed)
[PATCH] Different timeouts for wonder victory
Reported by: | elexis | Owned by: | elexis |
---|---|---|---|
Priority: | Should Have | Milestone: | Alpha 21 |
Component: | UI & Simulation | Keywords: | patch |
Cc: | Patch: |
Description
If you play the wonder mode, then the party who builds the wonder will have won 5 minutes after the wonder was built.
It is very hard to kill it in those 5 minutes, if the wonder building team rushed it (since it is possible to build it minute 11 with two players). You need at least 20-30 champions (rather 40-50) to kill it (minute 14) and that only works if the wonder building team didnt build three layers of walls around it.
The host should be able to select different timeouts in the gamesetup - like 5, 10, 15, 20 and 30 minutes.
Attachments (10)
Change History (41)
comment:1 by , 9 years ago
comment:2 by , 8 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
by , 8 years ago
Attachment: | wonderOptions.patch added |
---|
comment:3 by , 8 years ago
Keywords: | review patch added |
---|---|
Milestone: | Backlog → Alpha 20 |
Summary: | Different timeouts for wonder victory → [PATCH] Different timeouts for wonder victory |
I implemented different timeouts (given in wonder_times.json [1, 5, 10, 15, 20, 25, 30])
Quite a lot of changes in different files. I hope it is not too messy.
Anyway, my 2 cents 1) I don't like gamesteup.js file. It's too big and not clean. 2) The options dialog could look better. However, I won't produce bad code. I suggest keeping the GUI code more dynamical to avoid the black line in the dialog when wonder isn't selected and to keep the code small. 3) I added two new functions to EndGameManager.js to make the wonder timeout accessible. That makes the js file less generic. It's might be a better solution to use a global wonder object that stores the timeout property
comment:4 by , 8 years ago
Component: | Core engine → UI & Simulation |
---|---|
Keywords: | review removed |
Thanks for working on this (especially since gamesetup is messy). I've attached a readable version of the patch that fixes the following issues I saw, except finishing resizeMoreOptionsWindow
.
Please follow the wiki:Coding_Conventions.
Whitespace:
- Almost every line has wrong whitespace (don't have trailing whitespace, use tabs and check the indentation)
- Like 3 of 4 changed lines only change whitespace without changing the code, which makes this patch unreadable unless applying the patch and then creating a patch that ignores whitespace changes. There are some options for diff tools to do that. In case your editor introduced them, teach it a lesson.
gamesetup.js:
- The host may not change the duration on scenario maps (something I wasn't aware about when writing the ticket)
if (victoryTitle == "Wonder")
-> don't check for the value of a translated string- Missing semicolon at
initWonderDurations
. You might want to check using jshint for syntax errors.
resizeMoreOptionsWindow
- The GUI objects need to be updated for both singleplayer and ratedgames. That function must become generic now, otherwise we need like 4 if's and like 10 hardcoded GUI sizes.
- Calling this function must be done in
updateGUIObjects()
, otherwise it's not updated for the clients (since the dropdown is only updated for the host) - The hiding of the element should not be done in this function, it should only resize.
- A simplification: If you have the pattern
if (x) y="foo" else y="bar"
, just use the https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Conditional_Operator. Since it's true|false in this case, you can just say.hidden = g_VictoryConditions.Title[victoryIdx] == "wonder"
. - Add a comment explaining where the magic numbers come from, like that one for the multiplayer-options
updateGUIObjects
g_GameAttributes.settings.WonderDurations
should beg_GameAttributes.settings.WonderDuration
, since it only saves the selected value
setMapDescription
- Build the victory title string in one action (hence not moving around that
g_VictoryConditions.Default
check) " (" + translate("Wonder victory time") + ": "
doesn't properly support internationalization. Translators should be able to translate the formatting too. How about this thing:victoryTitle = sprintf( translatePluralWithContext( "victory condition", "Wonder (%(min)s minute)", "Wonder (%(min)s minutes)", g_GameAttributes.settings.WonderDuration ), { "min": g_GameAttributes.settings.WonderDuration } );
- missing semicolons
gamesetup.xml
- Rename "wonder" / "wonderText" to "wonderDuration" / "wonderDurationText" (so it's more specific and other wonder options might be added later hypothetically)
- Rename "wonderEntry" to "optionWonderDuration", so it's in line with the other objectnames of that level
- Tooltip: "Set the survival time after a wonder has build" => How about: "Number of minutes that the player has to keep the wonder in order to win"? (Since surviving itself is not sufficient to win)
settings.js:
wonder survival times
=>wonder victory times
- I'd suggest using "wonder victory" instead of "wonder" as context
wonder_times.json:
- trailing whitespace
Wonder.js
- Don't comment out, remove
- Now we have an unused number in the template. I asked the author sanderd17 on irc and the intended usage for that number in the template is to make the timeout for different civs a balancing decision too. So just changing
TimeTillVictory
toDurationMultiplier
in the template is a nice solution to both use the tempalte and gamesetup setting. - Maybe we can get scythewirler to change that value for ptolemians, since they don't have any resource costs for houses and economic buildings, hence a big advantage in wondergames.
Stopped looking now, in particular couldn't I test the patch or had the time to check all the XML number changes. I think about making all of that generic. If I don't find the time to do that, you can fix that resizeMoreOptionsWindow to work in both cases, check the XML numbers again and test the actual patch (ideally in multiplayer-mode by using two windows of 0ad, in case that works on your OS).
by , 8 years ago
Attachment: | wonderOptions_v2.patch added |
---|
Fixed most things noted above besides resizeMoreOptionsWindow
. Didn't test.
comment:5 by , 8 years ago
Hey! I found three mistakes
- nonNegativeDecimal does not exists and kill the entire game!
"ERROR: CXeromyces: Parse error: in_memory_buffer:9: Error type 'nonNegativeDecimal' is not exported by type library 'http://www.w3.org/2001/XMLSchema-datatypes'"
- There is a logic mistake in gamesetup.js
Engine.GetGUIObjectByName("optionWonderDuration").hidden =
g_GameAttributes.settings.GameType && g_GameAttributes.settings.GameType == "wonder";
=> != "wonder"
- WonderDuration not WonderDurations in Setup.js
line 52: if (settings.WonderDuration)
- if you do thinks like "Maybe we can get scythewirler to change that value for ptolemians, since they don't have any resource costs for houses and economic buildings, hence a big advantage in wondergames."
-- then we should display it somewhere
Test: Only single game yet
resizeMoreOptionsWindow: not done yet
by , 8 years ago
Attachment: | wonderOptions_V2.1.patch added |
---|
according to comment 5 + resizeMoreOptionsWindow + passed multiplayer test
comment:6 by , 8 years ago
Keywords: | review added |
---|---|
Milestone: | Alpha 20 → Alpha 21 |
comment:7 by , 8 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
Too bad, because elexis reviewed the patch already two month ago (talked about it in IRC) and the only problem is that scythetwirler opinion about the general behaviour is needed.
comment:8 by , 8 years ago
a few comments: line 334 minuts -> minutes in the Wonder schema, some comment indicating what DurationMultiplier is used for
comment:9 by , 8 years ago
Milestone: | Alpha 21 → Alpha 20 |
---|
comment:10 by , 8 years ago
Guess we should use decimal
instead of an integer type, as the multiplier should be something like 0.8
or 1.2
.
I added you to the credits already, but do you want to be mentioned there with your real name too?
comment:11 by , 8 years ago
Some remarks by sanderd17:
-
<ref name='nonNegativeDecimal'/>"
instead of"<data type='nonNegativeInteger'/>
- Don't set it to 0 on init but keep it undefined, to make it obvious that we rely on the setter to be called (otherwise we could also set a timeout of 5 minutes by default)
- the
json
file is not in the most recent patch, but one patch before - 5 hunks of the patch above are conflicting, shouldn't stop us though
comment:12 by , 8 years ago
I have updated the patch again so it applies cleanly to the current SVN.
But IMO the gui looks really ugly, it shows a hole when the wonder victory isn't selected.
I also still doubt whether it's a good idea to store the time as minutes. So far, we only use seconds and milliseconds to store time, and it's already confusing enough. Using another unit will only make it more confusing.
by , 8 years ago
Attachment: | wonderOptions_V2.2.patch added |
---|
comment:13 by , 8 years ago
Didn't I implement automatically resize!? Too long ago and can't remember exactly.
comment:14 by , 8 years ago
Keywords: | review removed |
---|---|
Milestone: | Alpha 20 → Alpha 21 |
So this is too much subject to discussion to be included now apparently.
Thanks for working on it, please address all the remarks above and we'll look into committing it in A21. :)
comment:15 by , 8 years ago
The issue with this patch is the missing automatic-resizing.
- The wonder-option should appear right after the victory condition
- In
session.xml
every option below the item needs a name (like "optionRatings") - If the game mode is not victory-condition, all elements below that dropdown should move one line up
- The new height of the dialog should be derived from the number of options visible
Should be possible with like 30 lines of code.
by , 8 years ago
Attachment: | wonderOptions_V2.3.patch added |
---|
comment:16 by , 8 years ago
Patch update:
- Fixes typo, adds comment to schema mentioned by mimo
- Uses a float type in the schema
- Includes
wonder_times.json
- Saves time in milliseconds as sugested by sanderd17
- Starts with 5 minute timeout if
SetWonderDuration
wasn't called (should never happen (make it undefined to indicate that?))
remaining TODO:
- Automatic resizing (comment:15)
- Still don't know if svott wants his realname in the credits.
comment:19 by , 8 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
Finally had some time for 0AD
I tried to use as few code as possible. Don't know if you accept a Boolean summation [let reduceOptions = optionWonderDurationHiddenState + !g_IsNetworked + !Engine.HasXmppClient()]
by , 8 years ago
Attachment: | wonderOptions_V2.4.patch added |
---|
compared to 2.3: only relevant changes in gamesetup.js line 307-339
comment:20 by , 8 years ago
arrrrg.. I know you don't like tabs in empty lines. I will remove them if further changes are required.
comment:21 by , 8 years ago
Keywords: | review added |
---|---|
Resolution: | fixed |
Status: | closed → reopened |
Didn't think you wanted to do that...
comment:22 by , 8 years ago
Keywords: | simple removed |
---|
Hi svott, thanks for the update, this patch is really needed! :-)
I checked only the new parts of the patch and found few style optimizations should be done:
- The comments above
let reduceOptions = optionWonderDurationHiddenState + !g_IsNetworked + !Engine.HasXmppClient()
are somewhat outdated (since the new option is taken into account). Remove them and just add one line mentioning that we reduce the size of the dialog by up to three options (wonderduration, cheats and rated game).
if(g_IsController)
->if (g_IsController)
resizeOptionButton
andresizeOptionDialog
used only once -> nuke / inline- missing semicolons after statements all over the place
- Don't hardcode that many values like
218, 246
, but compute them usingoptionButtonHeight
, so only 1-3 values will have to be changed when new options are added (instead of 12)optionButtonHeight
sounds like it should be a global const, so that you don't need to compute nor pass they2
argument of that function, but compute it inplace. You also don't need to passhiddenWonderReduction
but can immediately subtrat if from they1
argument. - The computation
optionButtonHeight * reduceOptions
is done twice, could be done only once by multiplying in the line above that. - The variable names could be improved, for example
optionWonderDurationHiddenState
->wonderDurationHidden
- maybe
hiddenWonderReduction
->offset
?
comment:23 by , 8 years ago
Thx for the fast review :) I reworked all your hint -- didn't find the if(
- g_optionButtonHeight: Size 30 is kind of wrong because 28 + 2 for the gap would be right, but 30 reduces the code a bit
- I don't know if you allow nested functions. For me, its makes the code clearer and strengthen encapsulation
by , 8 years ago
Attachment: | wonderOptions_V3.patch added |
---|
Rewrote resizeMoreOptionsWindow
to (almost) not use magic-numbers.
comment:24 by , 8 years ago
Changelog:
Enhancements:
- Rewrote
resizeMoreOptionsWindow
to (almost) not use magic-numbers and work with arbitrary options - Added some more choices to the json
Fixed minor mistakes:
- You forgot again to svn add the
wonder_times.json
! this.wonderDuration = 5;
must be in milliseconds, not minutes- that
moreOptions
number change in the XML seems wrong
Stylechanges:
- Did I mention missing semicolons? newline before {
g_optionButtonHeight
->g_OptionButtonHeight
wonderDurationHidden
->optionWonderDuration.hidden
(more consistent with the rest of the code)- not fond of defining a function inside a function (esp. if it uses parent function vars)
TODO:
- the wonder setting should be shown right below the victory condition
- therefore add names to all options and add those to the function
Otherwise, the end is near.
comment:26 by , 8 years ago
I am testing it right now I just realised that there is a warn left in simulation/helpers/Setup.js at line 52
warn(cmpEndGameManager.GetWonderDuration());
by , 8 years ago
Attachment: | wonderOptions_V3.2.patch added |
---|
comment:28 by , 8 years ago
Keywords: | review removed |
---|
Finally, thanks for all the work and persistance! :)
Two things I had changed:
optionCeasefireText
->optionCeasefire
(occured in two other places as well)- added two lines to
selectMap
to ensure setting the default duration when a scenario map sets wondermode but no timeout
Though in general I like options IMO we should be careful with host options for victory/defeat conditions (game modes) because with to many options the players joining the game have to look through all that options and might miss an important one for choosing their strategy.
Adjusting the time needed for a wonder to stand until victory is granted should be adjusted to be fun ofc. *Thumbs up*