Page MenuHomeWildfire Games

Fix objectives dialog value of the Disable Treasures setting and remove double negative
ClosedPublic

Authored by elexis on May 20 2017, 4:40 PM.

Details

Summary

Since there is a typo in the objectives dialog code, that page always displays "Disable Treasure: Disabled".
While at it, replace the counterintuitive phrasing with a more accurate statement.

Further Treasure setting values could be added which incidentally would make this option more positive,
like Treasure: initial (500 food, 500 wood) or Treasure: reorccuring (500 metal all 5 minutes) (where these resource numbers could be set manually, just like the starting resources could be set manually by the controller).
Survival of the fittest should also place treasure only if the disabling of treasures is disabled (...).

Test Plan

Start a game with disable treasure enabled and see that the dialog shows the wrong value unless applying this patch.

Event Timeline

elexis created this revision.May 20 2017, 4:40 PM
Vulcan added a subscriber: Vulcan.May 20 2017, 5:26 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/1266/ for more details.

Phormio commandeered this revision.May 22 2017, 12:41 PM
Phormio added a reviewer: elexis.

I don't like the wording of this patch:

  • the 'Objectives' dialog now calls the option 'Treasure', while everywhere else it is called 'Disable Treasures'
  • 'if the map places it' sounds odd
Phormio updated this revision to Diff 2118.May 22 2017, 12:52 PM
Phormio retitled this revision from Fix objectives dialog value of the Disable Treasures setting and remove double negative to Rename 'Disable Treasures' to 'Remove treasures' (which removes a double negative), fix its displayed value in the 'Objectives' dialog.
Phormio edited the summary of this revision. (Show Details)
  • renamed option 'Disable Treasures' to 'Remove Treasures'
  • 'Disabled' and 'Enabled' are lowercase for all other match settings, adjusted accordingly
  • elexis can now be happy that someone reviews his patch :)
Owners added subscribers: Restricted Owners Package, Restricted Owners Package.May 22 2017, 12:52 PM
scythetwirler requested changes to this revision.May 22 2017, 11:00 PM
scythetwirler added a subscriber: scythetwirler.

Why are there rating parsing changes in the context of this patch?

IMO "Enable Treasures: False" and "Enable Treasures: True" or "Treasures: Enabled" and "Treasures: Disabled" is more natural.

This revision now requires changes to proceed.May 22 2017, 11:00 PM
elexis edited edge metadata.

IMO "Enable Treasures: False" and "Enable Treasures: True" or "Treasures: Enabled" and "Treasures: Disabled" is more natural.

Should we display Treasues: Enabled even if there are no treasures placed? Seems misleading. My idea was "Treasures: Maybe" and "Treasures: If the map places it", but not happy about any proposal

elexis added a subscriber: elexis.

oh how did that happen

In D523#21868, @elexis wrote:

IMO "Enable Treasures: False" and "Enable Treasures: True" or "Treasures: Enabled" and "Treasures: Disabled" is more natural.

Should we display Treasues: Enabled even if there are no treasures placed? Seems misleading. My idea was "Treasures: Maybe" and "Treasures: If the map places it", but not happy about any proposal

Even better than three values (mind that we'd need a new colour for the button in pre-game settings) would be not to display the option at all unless there are treasures on the map, neither in the pre-game match setup dialog nor the in-game objectives dialogue.

Phormio added a comment.EditedMay 23 2017, 12:00 AM

Why are there rating parsing changes in the context of this patch?

IMO "Enable Treasures: False" and "Enable Treasures: True" or "Treasures: Enabled" and "Treasures: Disabled" is more natural.

Rating parsing changes were copied from the initial version of this diff, which was done by elexis. Afaik he has committed them in another place now.

elexis commandeered this revision.May 24 2017, 4:52 PM
elexis edited reviewers, added: Phormio; removed: elexis.
elexis edited edge metadata.

(requested feedback, not a commandeer)

elexis retitled this revision from Rename 'Disable Treasures' to 'Remove treasures' (which removes a double negative), fix its displayed value in the 'Objectives' dialog to Fix objectives dialog value of the Disable Treasures setting and remove double negative.May 24 2017, 4:53 PM
elexis edited the summary of this revision. (Show Details)

Phormio's proposal:

Another word for 'disable' would remove the double negative, the option has thus been renamed across the entire game.
'Remove treasures' is the best candidate. It doesn't imply that treasures could still be on the map (as 'disable' or 'gather' might), 'delete' is another action in the game, 'hide' or 'conceal' also imply that treasures could still be salvaged.
'Disabled' and 'Enabled' are lowercase for all other match settings, this was adjusted accordingly

I disagree with that "Remove treasures: disabled" is still a double negative. It is technically correct, but IMO it would be more intuitive to only speak in positives, i.e. saying when something is placed.

This has been in discussion since the introduction of the option:

2016-04-30-QuakeNet-#0ad-dev.log:16:04 < sanderd17> * Disable treasures: no > looks awkward to read IMO

This revision was automatically updated to reflect the committed changes.

I don't like the "if the map places it" sentence.

Possible treasures: Yes and Possible treasures: No as proposal.

Don't like that either. Notice that most objective dialog options are explained with a sentence, so adding a sentence fits.

Build has FAILED

Link to build: http://jw:8080/job/phabricator/1344/
See console output for more information: http://jw:8080/job/phabricator/1344/console