- User Since
- Dec 21 2016, 3:52 PM (161 w, 2 d)
I wouldn't mind switching to nanis theme under the condition that it uses a color style that fits to the existing theme, i.e. using gold for enabled controls and gray for disabled ones, no? and secondly the rectangular shape looks more brutal than the current slider sprite. so if that square rectangle shape would be changed, I might agree with that diff. One advantage of nanis proposal might be that the area of the slider selector is larger, so it may be easier to use / move it (or not, I didnt look at the size property in the xml).
Also dropdowns require hardcoding the values whereas a slider only has 3 datapoints (min, max, step)
The relic and wonder time go from 0 to 60 minutes, ceasefire from 0 to 45; why not 60 as well, for consistency? 60 minutes ceasefire used to be an option.
The more narrow the range, the easier it is to pick values.
I have seen maybe 300 matches with Victory durations and they were usually 50minutes, since for shorter times it was too hard to attack the enemy, as it is easy to turtle up.
Ceasefire has the main use case at < 5 minutes on nomad.
(See also irc discussion with Vladislav linked)
First proof of concept with stepWidth
20:14 < temple> stepWidth sounds easier
nani posted more review comments in a PM conversation, most importantly motivating me to create the second iteration of the patch, also he tested the patch.
Thu, Jan 23
Perhaps it's easier if you'd state the percentages how you'd like to distribute the empty space?
(Finding good numbers would take some time, and to me its currently not broken.)
Simplify CSlider code following the IGUIButtonBehavior inheritance by deleting m_IsPressed which is now redundant with m_Pressed from the inherited class.
This patch fixes the "copy twice" bug described in #3145!
And the crash over there can't be reproduced by me on arch, nor by go2die on mint 19.3 Tricia.
Wed, Jan 22
I like the example in the description though I think it could a bigger top padding.
I could move the value label 1px further below, but if there is a dropdown coming below the dropdown, it will start to collapse with that. There is simply too few space, which is why the setting height was increased from 32px to 36px already:
- Discussed with Vladislav on http://irclogs.wildfiregames.com/2020-01/2020-01-19-QuakeNet-%230ad-dev.log on the problem that not all values are chosen with the same frequency by users
- The UI theme was discussed with nani in a PM session today. nani proposed that the vertical center axis of the label on the left should be on the same vertical center as the average of the slider + label on the right side:
Slider Label: ----------- Number
which IMO looks worse.
Rebase, add value label, use it for RelicCount too
Tue, Jan 21
Mon, Jan 20
Perhaps the logo can be moved something like 25-100px depending on resolution, perhaps a percent number to the bottom, and then some margin to the buttons?
Test plan: Changes were tested on the balance mod.
(I guess it takes a bit of time, but it wouldn't hurt to document the test sample (i.e. approximate number of matches, list of participating players who may be considered either capable of judging or capable of playing representatively for competitive players. If its easier, one could also just upload replays and perhaps a link to the mod. The purpose of the list of test participant would allow (1) the reviewers to determine in how far the patch was tested and (2) later consumers of the patches (a24 players and devs) to find identify which players were capable of judging, or what might have gone wrong or right in case it was a fail or success.)
Yes. (I don't care about this one line changed here, just in general. Discussion != patch)
(or posting that you accept it so that he can close the revision proposal since it was committed in rP23395, to avoid ambiguity with an empty review)
Please do not forget to post which parts were reviewed, which parts were tested when posting an acceptance.
What do you mean exactly?
This is the patch I uploaded at https://code.wildfiregames.com/rP23387#40853 so it would be correct to credit that.
The patch results in
ERROR: GUI: Script path gui/pregame is not a directory
while it actually is a directory, just not one recognized by the code.
Removes the hack to Pop and Push the dialog in order to update it.
Update the gameAttributes instantly when changing an AI dropdown, so that the selection isnt lost when another client changes any game setting, preparing for #3806.
As mentioned in D2581, it is necessary to run the AIConfig page in the same script context as the GameSetupPage if one wants exclusively that page to be in charge of the AI attributes in g_GameAttributes.settings.PlayerData, since the page can't run code if its closed, but it can if its only hidden.
Not doing it that way would mean there would be a class in the AIConfig page manifesting the AI setting logic and another class in the Gamesetup page coding the same logic.
Second reason to perform this change is as mentioned the existing coupling between the Gamesetup page and subpages, which is a pattern, and thus refactoring this page to become a subpage means the pattern becomes an example of how to implement Gamesetup subpages, not a counterexample, with the new subpages being empowered and encouraged to reuse the existing gamesetup controlers (gameSettingsControl, playerAssignmentsControl, netMessages, mapCache, mapFilters, ...).
Rebase following rP23413 and remove more hardcodings.
Sat, Jan 18
(The patch was not tested, see IRC)
Discussed with Vladislav on http://irclogs.wildfiregames.com/2020-01/2020-01-18-QuakeNet-%230ad-dev.log and with nani in a PM, concluding preference for running it in the same page.
It is necessary to run it in the same page if there is a requirement to have the setting handler exclusively in one class, and in only one page, see summary.
Fri, Jan 17
Disagree both with reducing the amount of structures necessary to be built (the phases are usually eco-rushed through quite quickly) and with forcing the player to build certain structures.
Thu, Jan 16
It would be better to write new function SetBuilders and pass whole array at once.
Perhaps one can pass the Map or Map keys instead of creating an array as well.
Just have to be careful then, since the object reference might be taken over in case of assigning it directly, instead of getting a copy.
(I suppose that shouldn't break serialization, since object references are serialized as well, but checking would be better than assuming.
Also two entities having the same object reference in their state means that they cant modify it as if they owned it exclusively, as every modification to the object would change the state of both entities. In this case the old entity is destructed, so that might not be a problem.)
(Note to self: forgot those)
Wed, Jan 15
- Kush bonus:
So the only thing I wasnt sure about is the Kushite elephant bonus. It was discussed in the lobby about 2-3 weeks ago with ValihrAnt and on 2020-01-05.
It came out the purpose for nerving the elephant bonus was because some other non-uploaded patch (either borg or Valihrs balancing mod) changed the cost of elephants.
Since we don't have strong indication that this bonus is currently overpowered, we'll leave it out for now and if someone comes along and changes elephant stats or finds other aspects really warranting the bonus reduction for Kushites, we can add it then (once it's warranted).
Tue, Jan 14
From blackbox testing, search is acceptable to me, I'd prefer exact as a default too (I dont expect to ever use fuzzy search, but nani implemented it for his mapbrowser and other features too, so perhaps there are users for that).
Should optimize the solution for the user, not for the developer, and it looks more appealing if it is vertically centered, which is relevant for all resolutions greater than that one?
Otherwise one should do an evaluation which of the two (top alignment vs center alignment) looks more appealing. (Or perhaps even some golden ratio stuff?)
And then pick the one that looks best for all relevant resolutions.
Why did I post the JS?
or this pattern (has the advantage of not hardcoding references to neighbors):
top = 50%-halfHeight bottom = 50% + halfHeight `
where 2 * halfHeight = buttons * buttonHeight
Is there no way to force victory conditions?
Why not allow the host to vary the map more by playing with other or additional victory conditions?
Currently when selecting a skirmish/random map that comes with VictoryConditions, that choice is overwritten.
So the user still has to actively decide against the preference of the map.
(When playing in conquest, the player gaining the middle still has a big strategical advantage, and its the primary passage connecting the players, so the victory condition doesnt change the gameplay that much in case Conquest is enabled)
(It is quite easy to change the VictoryCondition format of maps to differentiate between forcing and overwriting a setting, the change could be local to VictoryConditions.js, except for the fact that Atlas would also need to be changed and perhaps the Gamesetup.cpp Autostart code)
Do you have a replay for that match? Or do you know which map it was? I cant reproduce with or without the persist file for skirmish maps, random maps and the unknown map, nor tutorial.