Page MenuHomeWildfire Games

Send a phase up notification to your teammates
ClosedPublic

Authored by Polakrity on Apr 18 2017, 12:39 AM.

Details

Summary

As indicated in trac #3512, only Petra send a notification.

  • Add a new option for chat notification.
  • Send a notification when the phase is started and is reached.
Test Plan

Test plan: Test all POV (observer, user, allies, ennemy) if the notification is sent.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

Polakrity created this revision.Apr 18 2017, 12:39 AM
Polakrity updated this revision to Diff 1318.Apr 18 2017, 12:44 AM

Fix some if

Lionkanzen added a reviewer: elexis.
Lionkanzen added a subscriber: Lionkanzen.
Polakrity updated this revision to Diff 1323.Apr 18 2017, 3:09 AM
  • Separate the state and name of phase.
  • Using .startsWith("phase_") for check the tech name.
  • Collect phase name from the phase template.
Polakrity updated this revision to Diff 1326.Apr 18 2017, 11:08 AM
  • Fix POV where the notifications weren't displayed.
  • Add autoResearch property for hide the phase_village.

Thanks to elexis for the reviews and the help

why not showing earlier when starts to phase due to make planing for things afterwards... inteam

mimo added a subscriber: mimo.Apr 18 2017, 8:20 PM

I would be nice to at least run a SP game with the proposed patch before uploading it.

binaries/data/mods/public/simulation/ai/petra/headquarters.js
415

how can it work if you remove that function? see line 30 of researchManager.js
You must either keep this function with an empty body , or remove that line from researchManager.
I'd keep an empty function in case of future need (as an illustration of how we can do such thing in the ai).

422

same comment with line 49 of researchManager.js

2216

this line as well as line 2218 must be kept otherwise the currentPhase is not updated.

Polakrity updated this revision to Diff 1350.Apr 19 2017, 8:33 AM
Polakrity marked 3 inline comments as done.

Fix some errors with the ia. (can cause problem with the serialize function)

Polakrity updated this revision to Diff 1354.Apr 19 2017, 10:46 AM

Change the boolean type by a dropdown in options with 3 news values.
This change allows display or not the notification when the research is started.

elexis requested changes to this revision.Apr 19 2017, 12:16 PM

None of the people I have asked (Polakrity, mimo, fatherbushido, wraitii, bb, Hannibal Barca and ffffffff) have a strong opinion.

A notification should occur as soon as possible (so therefore the start notification is intuitively the one we want),
But since the technology can be cancelled, the CC destroyed, deleted or captured, the starting notification would be a false positive (i.e. we can't derive completion form the start) to only show the started notification.
The current phase durations are short, so for vanilla 0AD it is not needed, so seems noisy to have both.

Either show only the completed one or add the option to allow players chosing between none, finished and both,.

This revision now requires changes to proceed.Apr 19 2017, 12:16 PM

Ah didn't get the update
Tested with the AI and cheating (with cheats only the completed message is shown).

binaries/data/mods/public/gui/options/options.json
285

Show a chat notification if you or an ally have started or completed a new phase, and phases of all players in observer mode

286

(Both -> "Started and Completed" won't work since the string is too long)

binaries/data/mods/public/gui/options/options.xml
81 ↗(On Diff #1354)

yep, that's the first dropdown seen in that panel anyway

binaries/data/mods/public/gui/session/messages.js
964

notifPhase -> notifyPhase

965

unneeded parenthesis, && binds stronger than ||, just like * and +, see JS operator precedence

976

Really nice that it displays Civ-specific phase name (Generic Name)

binaries/data/mods/public/simulation/ai/petra/headquarters.js
415

Thanks for the feedback mimo! (I didn't look at the petra code at all before), that helps deciding on what to do with the remains.
They might indeed be useful for future coding or even debugging.

binaries/data/mods/public/simulation/components/Player.js
735

An alternative would be TechnologyManager.prototype.ResearchTechnology. If you change the other, changing this one is probably better too, in particular since it doesn't do anything with the player component itself.

741

whitespace, let

binaries/data/mods/public/simulation/helpers/Commands.js
338

Capital I in accordance with the rest of the file and most of the code and since it's a sentence

339

An alternative place to include instead of Commands.js would be ProductionQueue.
This would for example make a difference if some other code would queue that tech instead of sending a command.

Actually it was said a long time ago that Commands.js should contain as few code as possible, everything should be moved to the components themselves. so probably better to move it there.

mimo added a comment.Apr 19 2017, 7:45 PM

Fix some errors with the ia. (can cause problem with the serialize function)

What kind of problem do you mean? if it is because it breaks compatibility with previous saved games, that can be expected and not a worry.

Polakrity updated this revision to Diff 1370.Apr 19 2017, 11:42 PM
Polakrity edited edge metadata.

@mimo: reverting not fixing. it was a bad choice of word. ;)

  • moving notifications in TechnologyManager and ProductionQueue.
  • notification when a phase is stopped.
  • GetStartedResearch & GetTechsStarted (duplicate functions) in TechnologyManager.
  • number -> word for option parameter (none, completed, all)
elexis requested changes to this revision.Apr 21 2017, 2:24 PM
elexis added inline comments.
binaries/data/config/default.cfg
348

whitespace.

Should mention the difference betwen 0, 1 and 2 brielfy

binaries/data/mods/public/gui/options/options.js
203 ↗(On Diff #1370)

Those hardcodings are bad, people should be able to use options.json without changing this file at all.

If you want to use nicer dropdown options, the options.json diff should become something like [ {"value": "completed": "label": "Completed" }, ...].
But this is a dropdown option rewrite which should be in a separate diff!

The material manager setting should become a slider (see some patch somewhere on phabricator and trac).

So use indices for now.

binaries/data/mods/public/gui/session/messages.js
973

cancelled -> canceled, as far as I can see, since we use american english, not british english

This revision now requires changes to proceed.Apr 21 2017, 2:24 PM
Polakrity updated this revision to Diff 1400.Apr 21 2017, 3:28 PM
Polakrity edited edge metadata.
Polakrity marked 6 inline comments as done.

updated in accordance to @elexis review

Polakrity updated this revision to Diff 1401.Apr 21 2017, 3:40 PM
Polakrity marked an inline comment as done.
Vulcan added a subscriber: Vulcan.Apr 21 2017, 4:40 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/839/ for more details.

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/840/ for more details.

elexis requested changes to this revision.Apr 21 2017, 6:21 PM
  • Canceled -> Aborted as approved in irc
  • ProductionQueue cancel missed a check in case someone stopped the production of a unit
  • cancel one should be in the technology manager, so that canceling the research without going through the production queue also works
  • autoresearched techs can't be stopped
  • Removing the duplicate function in a separate patch would be nicer
This revision now requires changes to proceed.Apr 21 2017, 6:21 PM
Polakrity updated this revision to Diff 1419.Apr 22 2017, 4:14 AM
Polakrity edited edge metadata.
Polakrity updated this revision to Diff 1420.Apr 22 2017, 5:29 AM

remove an useless condition

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/853/ for more details.

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/854/ for more details.

Polakrity updated this revision to Diff 1423.Apr 22 2017, 4:37 PM
  • move the started notification in TechnologyManager.
  • In ProductionQueue if we are starting a technology and the queue is empty, define productionStarted as true. (avoids double calls of StartedResearch and obviously to display twice started notification)

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/857/ for more details.

Polakrity updated this revision to Diff 1428.Apr 22 2017, 8:19 PM

add a new boolean argument notification, thanks @elexis

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/862/ for more details.

elexis accepted this revision.Apr 22 2017, 11:29 PM

The boolean argument seems to be the best option. This way we can control in the caller when we want to have a notification and when not, without repeating the guiinterface push notification call and without changing the code flaw oddly.
Taking those active strings, not the passive forms, as they appeared more sound.
The "stopped" change isn't needed anymore either.

Thanks for the patch, I have waited many months for that!

This revision is now accepted and ready to land.Apr 22 2017, 11:29 PM

Also notice we might want to extend this to arbitrary technologies in the future, in which case we have to rename the phaseName and phaseState part,remove the startsWith("phase") check and then let the user config decide which things will be displayed, potentially changing the strings.
bichtiades also wanted market build notifications badly in that ticket.
At some point it might make sense to make that user customizable if we can query buildings and techs (querying would also be useful to assing hotkeys to buildings).

elexis added inline comments.Apr 23 2017, 1:50 AM
binaries/data/mods/public/simulation/components/ProductionQueue.js
360 ↗(On Diff #1428)

This started change isn't needed anymore and if we were to take it, the productionStarted comment would have to be updated too.
The boolean should be false to avoid the duplicate message instead

binaries/data/mods/public/simulation/components/TechnologyManager.js
417 ↗(On Diff #1428)

There was a bug here.

  • The this.researchedTechs[tech] check must be a relic from the time when we didn't have the notification boolean check
  • A this.researchStarted[tech] is needed to ensure that we don't get a notification if we remove a queued research that isn't currently researching (i.e. when training a unit currently, adding the phase and then removing the phase while the unit is still training).

The delete calls need to be moved below the notification.

This revision was automatically updated to reflect the committed changes.