The tutorial can stuck after building a farm when you finish the houses, before you start the farm.
This is fixed, by introducing an init function that counts the number of built houses at the start of this goal.
Details
Details
- Reviewers
bb Imarok - Commits
- rP19998: Fix tutorial stuck after building farm
- Trac Tickets
- #4696
Look at ticket desciption and this summary and check one cannot stuck anymore
Diff Detail
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
Concerning the fix of the houses problem in your patch, i don't support the way it is done: if the tutorial asks for 2 new houses, it should ensure that these 2 houses are effectively built, even if other houses were already built.
okay, agree on that but that could also be accomplished by setting this.houseCount == rangeman... and then this.houseReq = this.houseCount + 2
imo would should take best from both patches, thus using the IsDone functions from here and the init as proposed by Imarok. I guess that will lead use to the most robust and generic solution.
| binaries/data/mods/public/maps/scripts/Tutorial.js | ||
|---|---|---|
| 6 ↗ | (On Diff #3065) | this will only work for foundations, not for unit training resource gathering, tech research (and too many others that can be listed), so keeping track of all those is not really robust. |
| 39 ↗ | (On Diff #3065) | isn't that the same as goal.IsDone || () => false |
| binaries/data/mods/public/maps/tutorials/starting_economy_walkthrough.js | ||
| 128 ↗ | (On Diff #3065) | and what happens if now the second next goal is done? (not really robust) |
| 133 ↗ | (On Diff #3065) | these functions indeed seem a nice addition, nuke some duplication |
| 164 ↗ | (On Diff #3065) | when a house is destroyed and rebuilded during this step, the this.foundation wouldn't be updated, imo adding that here too is an hack, and this more proof this.foundation is not robust. |
| 174 ↗ | (On Diff #3065) | there should be 2 houses right? |
| 290 ↗ | (On Diff #3065) | this shows why a this.count used for everything isn't working. the description says there should be 3 on stone and 3 on metal. so we need two parameters for that. |
Comment Actions
that would not work as players seem to not follow the tutorial and live their own life: you can't count house foundations in this houseCount, so what if the player had already another house foundation and is also building it. When this extra foundation is built, you would think the goal is achieved while it is not and your builders are still working on the houses from the tutorial. The only robust way is to register the two foundations in question, and ensure they are built. That is what the this.foundations is doing. But maybe it should not be in Tutorial.js, but only created on the fly by the economic tutorial itself. And thus, if a future tutorial need to do something analoguous for trained units or whatever, it will also do it on the fly.
imo would should take best from both patches, thus using the IsDone functions from here and the init as proposed by Imarok. I guess that will lead use to the most robust and generic solution.
We can add also a init function, but currently there is no need for it.
| binaries/data/mods/public/maps/scripts/Tutorial.js | ||
|---|---|---|
| 6 ↗ | (On Diff #3065) | see answer in the general comment. |
| 39 ↗ | (On Diff #3065) | yes, i'll change it. |
| binaries/data/mods/public/maps/tutorials/starting_economy_walkthrough.js | ||
| 128 ↗ | (On Diff #3065) | The original goal of this tutorial structure was a linear tutorial, when each goal comes after the previous one, and without imbrication. But sometimes, the player would have to wait too much, so i've added some imbrication, but limited to one level and its the responsability of the tutorial writer to ensure it is the case. If future tutorials will need more levels, the file Tutorial.js would have to be improved (or a new one written for such imbricated tutorials), but that's out of the scope of that ticket. |
| 164 ↗ | (On Diff #3065) | No but if the player destroys it during the tutorial, he may expect to be stuck somewhere. I don't think it's useful to cover all such cases (although it's fine if somebody want to add them). |
| 174 ↗ | (On Diff #3065) | yes, and when built, the two foundations will no more exist, so the filter will return []. |
| 290 ↗ | (On Diff #3065) | Although the tutorial asks for 3 on stone and 3 on metal, the goal will be accepted if at least two bunches of workers sent to mine, and it could even be both on the same resource. The goal was to learn how to queue some actions (deposit resource and go mining) and the number or the resource does not matter. If the player does not follow what is required, it's up to him. Once again, covering all possible actions is not worth, but i agree that the this.count could be improved, but not on this ticket. |
Comment Actions
updated version with adding a Init function, removing of the global this.count and allowing techs to be already researched.
Comment Actions
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! Checking XML files...
http://jw:8080/job/phabricator/1855/ for more details.
Comment Actions
Executing section Default...
Executing section Source...
Executing section JS...
| | [NORMAL] ESLintBear (no-else-return):
| | Unnecessary 'else' after 'return'.
|----| | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/TechnologyManager.js
| |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/TechnologyManager.js
| 440| 440| {
| 441| 441| if (this.researchQueued[tech])
| 442| 442| return true;
| 443| |- else
| 444| |- return false;
| | 443|+ return false;
| 445| 444| };
| 446| 445|
| 447| 446| // Get all techs that are currently being researched
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 153| » » » switch·(type)
| | [NORMAL] ESLintBear (default-case):
| | Expected a default case.
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 168| » switch·(entity.check)
| | [NORMAL] ESLintBear (default-case):
| | Expected a default case.
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 66| » » » ||·(tech.top·&&·(this.IsTechnologyResearched(tech.top)·||·this.IsTechnologyResearched(tech.bottom))))
| | [NORMAL] JSHintBear:
| | Misleading line break before '||'; readers may interpret this as an expression boundary.
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 232| » » » for·(var·component·in·modifiedComponents)
| | [NORMAL] JSHintBear:
| | 'component' is already defined.
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 238| » » var·cmpTemplateManager·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_TemplateManager);
| | [NORMAL] JSHintBear:
| | 'cmpTemplateManager' is already defined.
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 239| » » var·template·=·cmpTemplateManager.GetCurrentTemplateName(msg.entity);
| | [NORMAL] JSHintBear:
| | 'template' is already defined.
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 244| » » » var·cmpIdentity·=·Engine.QueryInterface(msg.entity,·IID_Identity);
| | [NORMAL] JSHintBear:
| | 'cmpIdentity' is already defined.
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 247| » » » » var·classes·=·cmpIdentity.GetClassesList();
| | [NORMAL] JSHintBear:
| | 'classes' is already defined.
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 305| » » » var·template·=·this.GetTechnologyTemplate(i);
| | [NORMAL] JSHintBear:
| | 'template' is already defined.
binaries/data/mods/public/maps/tutorials/starting_economy_walkthrough.js
| 258| » » » ····························&&·(cmpTechnologyManager.IsTechnologyQueued("gather_farming_plows")·||
| | [NORMAL] JSHintBear:
| | Misleading line break before '&&'; readers may interpret this as an expression boundary.
Executing section XML GUI...
Executing section Python...
Executing section Perl...http://jw:8080/job/phabricator_lint/395/ for more details.
Comment Actions
This system seems much more flexible and covers all cases I can foresee without much duplication. Still some (mainly style) comments
| binaries/data/mods/public/maps/tutorials/starting_economy_walkthrough.js | ||
|---|---|---|
| 153 ↗ | (On Diff #3129) | why not a simple array? |
| 183 ↗ | (On Diff #3129) | this alone wouldn't fix the issue, since during the previous goal the houses could be finished during the last goal, and then never be deleted from this. |
| 187 ↗ | (On Diff #3129) | wondering if there can be some strange onEntityRenamed on these entities (i guess not in current code) |
| 229 ↗ | (On Diff #3129) | this.count => this.femaleCount imo |
| 311 ↗ | (On Diff #3129) | notice that there is not specified how many gatherers are needed, so one seems indeed good |
| 331–332 ↗ | (On Diff #3129) | (could be rewritten to something fancy like this.stone = this.stone || TriggerHelper.GetResourceType(msg.cmd.target).generic == "stone") but really no strong opinion |
| 333 ↗ | (On Diff #3129) | else if ? |
| binaries/data/mods/public/simulation/components/TechnologyManager.js | ||
| 94 ↗ | (On Diff #3129) | unneeded braces, same below |
| binaries/data/mods/public/simulation/components/Trigger.js | ||
| 232 ↗ | (On Diff #3129) | perhaps "foundation" is clearer than "from" ("from" could also be seen as owner) |
Comment Actions
There are nonetheless some points which could still be improved:
- We should add a TriggerHelper fonction to know what are the possible techs available from a structure (the farmstead here) and use them instead of assuming the 2 techs currently available (and could change in the future).
- And also have a new TriggerHelper function checking for QueuedTech + StartedTech + ResearchedTechs, to avoid the current dupplication.
But i'd rather have them in a following cleaning patch.
| binaries/data/mods/public/maps/tutorials/starting_economy_walkthrough.js | ||
|---|---|---|
| 153 ↗ | (On Diff #3129) | Set is much simpler: array would need to use indexOf and splice while add and delete are much more readable. |
| 183 ↗ | (On Diff #3129) | fixed now, it was supposed to be followed in all goals between, but i forgot to add it. |
| 187 ↗ | (On Diff #3129) | I don't think we will ever upgrade a foundation. That's the only case where we would need to listen to onEntityRenamed here. |
| 229 ↗ | (On Diff #3129) | ok |
| 331–332 ↗ | (On Diff #3129) | no real opinion also, so i leave it as it is |
| 333 ↗ | (On Diff #3129) | ok |
| binaries/data/mods/public/simulation/components/TechnologyManager.js | ||
| 94 ↗ | (On Diff #3129) | yep |
| binaries/data/mods/public/simulation/components/Trigger.js | ||
| 232 ↗ | (On Diff #3129) | ok |
Comment Actions
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! Checking XML files...
http://jw:8080/job/phabricator/1861/ for more details.
Comment Actions
Executing section Default...
Executing section Source...
Executing section JS...
binaries/data/mods/public/maps/tutorials/starting_economy_walkthrough.js
| 268| » » » ····························&&·(cmpTechnologyManager.IsTechnologyQueued("gather_farming_plows")·||
| | [NORMAL] JSHintBear:
| | Misleading line break before '&&'; readers may interpret this as an expression boundary.
| | [NORMAL] ESLintBear (no-else-return):
| | Unnecessary 'else' after 'return'.
|----| | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/TechnologyManager.js
| |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/TechnologyManager.js
| 440| 440| {
| 441| 441| if (this.researchQueued[tech])
| 442| 442| return true;
| 443| |- else
| 444| |- return false;
| | 443|+ return false;
| 445| 444| };
| 446| 445|
| 447| 446| // Get all techs that are currently being researched
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 153| » » » switch·(type)
| | [NORMAL] ESLintBear (default-case):
| | Expected a default case.
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 168| » switch·(entity.check)
| | [NORMAL] ESLintBear (default-case):
| | Expected a default case.
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 66| » » » ||·(tech.top·&&·(this.IsTechnologyResearched(tech.top)·||·this.IsTechnologyResearched(tech.bottom))))
| | [NORMAL] JSHintBear:
| | Misleading line break before '||'; readers may interpret this as an expression boundary.
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 232| » » » for·(var·component·in·modifiedComponents)
| | [NORMAL] JSHintBear:
| | 'component' is already defined.
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 238| » » var·cmpTemplateManager·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_TemplateManager);
| | [NORMAL] JSHintBear:
| | 'cmpTemplateManager' is already defined.
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 239| » » var·template·=·cmpTemplateManager.GetCurrentTemplateName(msg.entity);
| | [NORMAL] JSHintBear:
| | 'template' is already defined.
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 244| » » » var·cmpIdentity·=·Engine.QueryInterface(msg.entity,·IID_Identity);
| | [NORMAL] JSHintBear:
| | 'cmpIdentity' is already defined.
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 247| » » » » var·classes·=·cmpIdentity.GetClassesList();
| | [NORMAL] JSHintBear:
| | 'classes' is already defined.
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 305| » » » var·template·=·this.GetTechnologyTemplate(i);
| | [NORMAL] JSHintBear:
| | 'template' is already defined.
Executing section XML GUI...
Executing section Python...
Executing section Perl...http://jw:8080/job/phabricator_lint/400/ for more details.
Comment Actions
Was already thinking of the second one myself, indeed are nice additions for a later patch. A thing to add to this is storing the playerID (which is 1) in a global somewhere, but also for a later patch.
| binaries/data/mods/public/maps/scripts/Tutorial.js | ||
|---|---|---|
| 54 ↗ | (On Diff #3143) | unneeded newline |
| binaries/data/mods/public/maps/tutorials/starting_economy_walkthrough.js | ||
| 121–124 ↗ | (On Diff #3143) | shouldn't this for sanity goes into the previous goal, since this assignment has nothing to do with this goal? |
| 147 ↗ | (On Diff #3143) | a player can misunderstand this and building houses already in this step without pressing "ready", which isn't that obscure imo and he is not living his own live (which also isn't a crime imo), perhaps move the init to this step and adding the OnPlayerCommand. (or merge the two instructions) |
| 167–170 ↗ | (On Diff #3143) | This will give us a new problem, imagine a player building first one house and finishes it, then construct another one. the player has done what is said in the instructions, but won't continue to the next goal... |
| 193 ↗ | (On Diff #3143) | there doesn't seem a reason why a player wouldn't destroy his houses, and those are not removed, thus one gets rly stuck here... i guess we should store the amount of houses already build in the previous houseBuild goal and check here for that value +2. |
| 276 ↗ | (On Diff #3143) | trailling comma (there are more of them, but adding one seems meh) |
| 308 ↗ | (On Diff #3143) | the check for IsDone doesn't break anything, but why is in necessary to call it? I don't see a situation where it is useful (perhaps i overlook something though) |
| 311–315 ↗ | (On Diff #3143) | sorta the same here, how could a tech finish when there is no cheat and it didn't start nor already was started? |
| 341–344 ↗ | (On Diff #3143) | strictly one can order 1 unit to stone and then the same unit to metal and the goal is reached, however idc if a player cheats like that. |
Comment Actions
In fact, the helper for the techs is now added (was simple enough), as well as the playerID.
| binaries/data/mods/public/maps/scripts/Tutorial.js | ||
|---|---|---|
| 54 ↗ | (On Diff #3143) | fixed |
| binaries/data/mods/public/maps/tutorials/starting_economy_walkthrough.js | ||
| 121–124 ↗ | (On Diff #3143) | i've no strong opinions about it, so ok |
| 147 ↗ | (On Diff #3143) | The intent was to not have to scroll for seeing the text of the last goal. So merging is not possible (maybe later if we succeed to shrink a bit the text, there is another ticket for that). I've also fixed your next comment (destroyed houses) by rather listening to OnOwnershipChanged, and also moved it here. |
| 167–170 ↗ | (On Diff #3143) | fixed by adding a houseCount |
| 193 ↗ | (On Diff #3143) | ok fixed by listening to OnOwnershipChanged |
| 276 ↗ | (On Diff #3143) | ok |
| 308 ↗ | (On Diff #3143) | yes, was a leftover from previous iterations. |
| 341–344 ↗ | (On Diff #3143) | yes, the player can do that, but we don't care. |
Comment Actions
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! Checking XML files...
http://jw:8080/job/phabricator/1870/ for more details.
Comment Actions
Executing section Default...
Executing section Source...
Executing section JS...
binaries/data/mods/public/maps/scripts/TriggerHelper.js
| 45| » if·(owner·==·null)
| | [NORMAL] JSHintBear:
| | Use '===' to compare with 'null'.
binaries/data/mods/public/maps/scripts/TriggerHelper.js
| 206| » » if·(cmpTerritoryManager.GetOwner(pos.x,·pos.z)·==·0)
| | [NORMAL] JSHintBear:
| | Use '===' to compare with '0'.
binaries/data/mods/public/maps/scripts/TriggerHelper.js
| 222| » ································cmpTechnologyManager.IsTechnologyResearched(techName))
| | [NORMAL] JSHintBear:
| | Missing semicolon.
| | [NORMAL] ESLintBear (no-else-return):
| | Unnecessary 'else' after 'return'.
|----| | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/TechnologyManager.js
| |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/TechnologyManager.js
| 440| 440| {
| 441| 441| if (this.researchQueued[tech])
| 442| 442| return true;
| 443| |- else
| 444| |- return false;
| | 443|+ return false;
| 445| 444| };
| 446| 445|
| 447| 446| // Get all techs that are currently being researched
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 153| » » » switch·(type)
| | [NORMAL] ESLintBear (default-case):
| | Expected a default case.
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 168| » switch·(entity.check)
| | [NORMAL] ESLintBear (default-case):
| | Expected a default case.
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 66| » » » ||·(tech.top·&&·(this.IsTechnologyResearched(tech.top)·||·this.IsTechnologyResearched(tech.bottom))))
| | [NORMAL] JSHintBear:
| | Misleading line break before '||'; readers may interpret this as an expression boundary.
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 232| » » » for·(var·component·in·modifiedComponents)
| | [NORMAL] JSHintBear:
| | 'component' is already defined.
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 238| » » var·cmpTemplateManager·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_TemplateManager);
| | [NORMAL] JSHintBear:
| | 'cmpTemplateManager' is already defined.
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 239| » » var·template·=·cmpTemplateManager.GetCurrentTemplateName(msg.entity);
| | [NORMAL] JSHintBear:
| | 'template' is already defined.
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 244| » » » var·cmpIdentity·=·Engine.QueryInterface(msg.entity,·IID_Identity);
| | [NORMAL] JSHintBear:
| | 'cmpIdentity' is already defined.
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 247| » » » » var·classes·=·cmpIdentity.GetClassesList();
| | [NORMAL] JSHintBear:
| | 'classes' is already defined.
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 305| » » » var·template·=·this.GetTechnologyTemplate(i);
| | [NORMAL] JSHintBear:
| | 'template' is already defined.
Executing section XML GUI...
Executing section Python...
Executing section Perl...http://jw:8080/job/phabricator_lint/406/ for more details.
Comment Actions
We now have made 5 triggers with 37 codelines, which still does not solve all cases (a player can reach a goal with deleting houses), why do we make such a complexity and error prone system instead of this total of 11 lines of code and 3 triggers: ...
| binaries/data/mods/public/maps/tutorials/starting_economy_walkthrough.js | ||
|---|---|---|
| 149 ↗ | (On Diff #3155) | ... |
| 154 ↗ | (On Diff #3155) | ... if (TriggerHelper.EntityHasClass(+msg.entity, "House"))
++this.houseFoundationCount;} |
| 174 ↗ | (On Diff #3155) | ... |
| 178 ↗ | (On Diff #3155) | ... if (TriggerHelper.EntityHasClass(+msg.entity, "House") && ++this.houseFoundationCount > 1)
this.nextGoal();} |
| 221 ↗ | (On Diff #3155) | ... this.housesToBuild = this.houseCountBefore - rangeManager.query("house", playerID).length + 2;} |
| 222 ↗ | (On Diff #3155) | ... |
| 226 ↗ | (On Diff #3155) | .... if (TriggerHelper.EntityHasClass(+msg.entity, "House") && --this.housesToBuild < 1)
this.nextGoal();} |
Comment Actions
But go back to the aim of this step of the tutorial: it is to learn the player how to queue orders in order to build several structures in a go, and then to ensure that the two workers assigned to that task have finished their jobs. What you propose does not fit that purpose: having two houses built is not the aim, but just a consequence if the player follows the tutorial. With you proposition (which would still need a lot of additional lines to work properly), if the player make another house foundation somewhere else and build it before the two required, the tutorial will think the builders affected to the required construction have finished their job.
The complexity come only when we want to foresee the case where the player would destroy some of its foundations. I didn't provide it on the first version because i think the tutorial should stay simple, and not become a monkey-proof full game which would allow the player to do anything he wants: if the player does not follow the tutorial, he should expect to be stuck. But as you seem to be bothered by that, i've added it.
Comment Actions
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! Checking XML files...
http://jw:8080/job/phabricator/1871/ for more details.
Comment Actions
Executing section Default...
Executing section Source...
Executing section JS...
binaries/data/mods/public/maps/scripts/TriggerHelper.js
| 45| » if·(owner·==·null)
| | [NORMAL] JSHintBear:
| | Use '===' to compare with 'null'.
binaries/data/mods/public/maps/scripts/TriggerHelper.js
| 206| » » if·(cmpTerritoryManager.GetOwner(pos.x,·pos.z)·==·0)
| | [NORMAL] JSHintBear:
| | Use '===' to compare with '0'.
binaries/data/mods/public/maps/scripts/TriggerHelper.js
| 222| » ································cmpTechnologyManager.IsTechnologyResearched(techName))
| | [NORMAL] JSHintBear:
| | Missing semicolon.
| | [NORMAL] ESLintBear (no-else-return):
| | Unnecessary 'else' after 'return'.
|----| | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/TechnologyManager.js
| |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/TechnologyManager.js
| 440| 440| {
| 441| 441| if (this.researchQueued[tech])
| 442| 442| return true;
| 443| |- else
| 444| |- return false;
| | 443|+ return false;
| 445| 444| };
| 446| 445|
| 447| 446| // Get all techs that are currently being researched
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 153| » » » switch·(type)
| | [NORMAL] ESLintBear (default-case):
| | Expected a default case.
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 168| » switch·(entity.check)
| | [NORMAL] ESLintBear (default-case):
| | Expected a default case.
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 66| » » » ||·(tech.top·&&·(this.IsTechnologyResearched(tech.top)·||·this.IsTechnologyResearched(tech.bottom))))
| | [NORMAL] JSHintBear:
| | Misleading line break before '||'; readers may interpret this as an expression boundary.
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 232| » » » for·(var·component·in·modifiedComponents)
| | [NORMAL] JSHintBear:
| | 'component' is already defined.
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 238| » » var·cmpTemplateManager·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_TemplateManager);
| | [NORMAL] JSHintBear:
| | 'cmpTemplateManager' is already defined.
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 239| » » var·template·=·cmpTemplateManager.GetCurrentTemplateName(msg.entity);
| | [NORMAL] JSHintBear:
| | 'template' is already defined.
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 244| » » » var·cmpIdentity·=·Engine.QueryInterface(msg.entity,·IID_Identity);
| | [NORMAL] JSHintBear:
| | 'cmpIdentity' is already defined.
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 247| » » » » var·classes·=·cmpIdentity.GetClassesList();
| | [NORMAL] JSHintBear:
| | 'classes' is already defined.
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 305| » » » var·template·=·this.GetTechnologyTemplate(i);
| | [NORMAL] JSHintBear:
| | 'template' is already defined.
Executing section XML GUI...
Executing section Python...
Executing section Perl...http://jw:8080/job/phabricator_lint/407/ for more details.
Comment Actions
The aim of a tutorial is to give information and let the player do some basic actions so the player can learn a bit, not to enforce the player doing things in a fixed order or a fixed way. New players (the main audience of a tutorial) will probably have no knowledge of the game or maybe even none of the game type. Some of those players might (accidentally) perform actions which are not explicitly stated in the tutorial, however for that the player shouldn't get stuck when performing "normal" (okay they might get stuck when destroying the cc or similar, i am ok with that).
Such a system might include that players does not follow exactly what is written in the tutorial, but that isn't the case in the current either. F.e. the instructions state that the player should select his woman in 3 different ways and then order them somewhere. Yet just ordering them is enough to get through. Also if a player wants to live his own live, --so what--, it seems we rather need a lot of complexity to disallow it than to allow it, since if that is the case the player doesn't want to learn, so we shouldn't force him to do that.
IMO the instructions can give as much information about the game we want, and then have a simple defined goal that the player should execute (like build 2 houses or finish 2 houses). That way (using the rangeManager queries) the system would be simple.
Perhaps before we do any more work on this we should consult other team members/players for how a tutorial should work.
Comment Actions
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! Checking XML files...
http://jw:8080/job/phabricator/1872/ for more details.
Comment Actions
Executing section Default...
Executing section Source...
Executing section JS...
binaries/data/mods/public/maps/scripts/TriggerHelper.js
| 45| » if·(owner·==·null)
| | [NORMAL] JSHintBear:
| | Use '===' to compare with 'null'.
binaries/data/mods/public/maps/scripts/TriggerHelper.js
| 206| » » if·(cmpTerritoryManager.GetOwner(pos.x,·pos.z)·==·0)
| | [NORMAL] JSHintBear:
| | Use '===' to compare with '0'.
binaries/data/mods/public/maps/scripts/TriggerHelper.js
| 222| » ································cmpTechnologyManager.IsTechnologyResearched(techName))
| | [NORMAL] JSHintBear:
| | Missing semicolon.
| | [NORMAL] ESLintBear (no-else-return):
| | Unnecessary 'else' after 'return'.
|----| | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/TechnologyManager.js
| |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/TechnologyManager.js
| 440| 440| {
| 441| 441| if (this.researchQueued[tech])
| 442| 442| return true;
| 443| |- else
| 444| |- return false;
| | 443|+ return false;
| 445| 444| };
| 446| 445|
| 447| 446| // Get all techs that are currently being researched
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 153| » » » switch·(type)
| | [NORMAL] ESLintBear (default-case):
| | Expected a default case.
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 168| » switch·(entity.check)
| | [NORMAL] ESLintBear (default-case):
| | Expected a default case.
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 66| » » » ||·(tech.top·&&·(this.IsTechnologyResearched(tech.top)·||·this.IsTechnologyResearched(tech.bottom))))
| | [NORMAL] JSHintBear:
| | Misleading line break before '||'; readers may interpret this as an expression boundary.
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 232| » » » for·(var·component·in·modifiedComponents)
| | [NORMAL] JSHintBear:
| | 'component' is already defined.
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 238| » » var·cmpTemplateManager·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_TemplateManager);
| | [NORMAL] JSHintBear:
| | 'cmpTemplateManager' is already defined.
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 239| » » var·template·=·cmpTemplateManager.GetCurrentTemplateName(msg.entity);
| | [NORMAL] JSHintBear:
| | 'template' is already defined.
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 244| » » » var·cmpIdentity·=·Engine.QueryInterface(msg.entity,·IID_Identity);
| | [NORMAL] JSHintBear:
| | 'cmpIdentity' is already defined.
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 247| » » » » var·classes·=·cmpIdentity.GetClassesList();
| | [NORMAL] JSHintBear:
| | 'classes' is already defined.
binaries/data/mods/public/simulation/components/TechnologyManager.js
| 305| » » » var·template·=·this.GetTechnologyTemplate(i);
| | [NORMAL] JSHintBear:
| | 'template' is already defined.
Executing section XML GUI...
Executing section Python...
Executing section Perl...http://jw:8080/job/phabricator_lint/408/ for more details.
Comment Actions
Are you aware that we are discussing if the tutorial should switch goal when any two houses or only when the two required houses are built? I would not have expected to loose any time on such discussions, but feel free to consult anybody you want. In the meantime, if there are no more comments on the general structure (basically the Tutorial.js file), i'll commit that patch and you will be able to raise a concern. There are more important things to do as