We do not have such auras yet in vanilla game, but this should be fixed for mods as this is misleading.
Details
Details
- Reviewers
fatherbushido - Group Reviewers
Restricted Owners Package (Owns No Changed Paths) - Commits
- rP20299: Ranged auras not enabled (because of a tech requirement for example) should not…
The patch adds a tech requirement on town phase for female auras (only for testing, will be removed on commit). Check that without the rest of the patch, the aura range is always displayed even when in village phase, while with the full patch the visualization is fixed.
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
Comment Actions
Build is green
Updating workspaces... Build (release)... Build (debug)... Running release tests... Running cxxtest tests (307 tests).ERROR: JavaScript error: simulation/components/Auras.js line 274 ReferenceError: IID_RangeVisualization is not defined Auras.prototype.Clean@simulation/components/Auras.js:274:7 Auras.prototype.Init@simulation/components/Auras.js:21:2 global.ConstructComponent@simulation/components/tests/setup.js:105:2 testAuras@simulation/components/tests/test_Auras.js:91:17 @simulation/components/tests/test_Auras.js:115:1 In TestComponentScripts::test_scripts: /mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:44: Error: Test failed: L"Running script simulation/components/tests/test_Auras.js" /mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:44: Error: Assertion failed: scriptInterface.LoadScript(pathname, content) ................................................................................................................................................................................................................................................................................................................. Failed 1 and Skipped 0 of 307 tests Success rate: 99% Running debug tests... Running cxxtest tests (307 tests).ERROR: JavaScript error: simulation/components/Auras.js line 274 ReferenceError: IID_RangeVisualization is not defined Auras.prototype.Clean@simulation/components/Auras.js:274:7 Auras.prototype.Init@simulation/components/Auras.js:21:2 global.ConstructComponent@simulation/components/tests/setup.js:105:2 testAuras@simulation/components/tests/test_Auras.js:91:17 @simulation/components/tests/test_Auras.js:115:1 In TestComponentScripts::test_scripts: /mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:44: Error: Test failed: L"Running script simulation/components/tests/test_Auras.js" /mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:44: Error: Assertion failed: scriptInterface.LoadScript(pathname, content) ................................................................................................................................................................................................................................................................................................................. Failed 1 and Skipped 0 of 307 tests Success rate: 99% Checking XML files...
http://jenkins-master:8080/job/phabricator/2122/ for more details.
Comment Actions
Executing section Default... Executing section Source... Executing section JS... binaries/data/mods/public/simulation/components/Auras.js | 208| » » this.IsRangeAura(name) | | [NORMAL] JSHintBear: | | Missing semicolon. Executing section XML GUI...
http://jenkins-master:8080/job/phabricator_lint/593/ for more details.
Comment Actions
Patch works as expected (and is indeed needed)
I didn't look at the code (at least I didn't see something you could have miss). If you need a more detailed review, I can look at it.
You need to update (maintain) the Auras test
(adding Engine.LoadComponentScript("interfaces/RangeVisualization.js"); should be enough)
| binaries/data/mods/public/simulation/components/RangeVisualization.js | ||
|---|---|---|
| 110 ↗ | (On Diff #3887) | (UpdateRangeAura, UpdateVisualAuraRanges, RegenerateRangeVisualizations, naming is a bit confusing, was wondering wheather that could be refactored (rename or getting away without adding a new function) to be more clear, dunno) |
Comment Actions
Thanks for the comments.
I indeed forgot the tests.
Concerning refactoring, i've no good idea so for the time being, i've removed the new function and just call both needed ones from Auras.js. Maybe we could add an argument to UpdateVisualXXXRanges which when true would regenerate the visualization. But refactoring can be another patch when we have a good proposition.
There were nonetheless some missing parts in the previous patch:
OnOwnershipChanged: the new owner may not have the required tech, or have a tech which changes the ranges of heal. So all UpdateVisualXXXRanges are now called.
OnDeserialized ??? i don't understand why it was needed as Deserialize call Init which already update heal ranges. So i've removed it, and everything seems to work. But if anybody knows why it was needed? i may have missed something.
Comment Actions
Build is green
Updating workspaces...
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDAnimation]’:
FCollada/FCDocument/FCDLibrary.cpp:149:30: required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
const T* cptr = ((const FCDLibrary<T>*)l1)->GetEntity(0);
^
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDAnimationClip]’:
FCollada/FCDocument/FCDLibrary.cpp:150:34: required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDCamera]’:
FCollada/FCDocument/FCDLibrary.cpp:151:27: required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDController]’:
FCollada/FCDocument/FCDLibrary.cpp:152:31: required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDEffect]’:
FCollada/FCDocument/FCDLibrary.cpp:153:27: required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDEmitter]’:
FCollada/FCDocument/FCDLibrary.cpp:154:28: required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDForceField]’:
FCollada/FCDocument/FCDLibrary.cpp:155:31: required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDGeometry]’:
FCollada/FCDocument/FCDLibrary.cpp:156:29: required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDImage]’:
FCollada/FCDocument/FCDLibrary.cpp:157:26: required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDLight]’:
FCollada/FCDocument/FCDLibrary.cpp:158:26: required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDMaterial]’:
FCollada/FCDocument/FCDLibrary.cpp:159:29: required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDSceneNode]’:
FCollada/FCDocument/FCDLibrary.cpp:160:30: required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDPhysicsModel]’:
FCollada/FCDocument/FCDLibrary.cpp:161:33: required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDPhysicsMaterial]’:
FCollada/FCDocument/FCDLibrary.cpp:162:36: required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ shttp://jenkins-master:8080/job/phabricator/2123/ for more details.
Comment Actions
Executing section Default... Executing section Source... Executing section JS... binaries/data/mods/public/simulation/components/Auras.js | 208| » » this.IsRangeAura(name) | | [NORMAL] JSHintBear: | | Missing semicolon. Executing section XML GUI...
http://jenkins-master:8080/job/phabricator_lint/594/ for more details.
Comment Actions
Build is green
Updating workspaces... Build (release)... Build (debug)... Running release tests... Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK! Checking XML files...
http://jenkins-master:8080/job/phabricator/2124/ for more details.
Comment Actions
Executing section Default... Executing section Source... Executing section JS... Executing section XML GUI...
http://jenkins-master:8080/job/phabricator_lint/595/ for more details.
Comment Actions
Run a blame when not finding a reason from reading the code why a line is there:
svn blame RangeVisualization.js | grep Deser
rP19861 RangeVisualization.prototype.OnDeserialized = function(msg)
Code looks better IMO
Comment Actions
Would have been worth a comment in the code (I'll add it).
Then also the Auras range have to be updated onDeserialized. And there is no need to update them on Init (onOwnershipChanged et onDeserialized should be enough).
Comment Actions
updated patch: restore the initialization onDeserialized but including Auras, and remove useless initialization in Init.
Comment Actions
Build is green
Updating workspaces... Build (release)... Build (debug)... Running release tests... Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK! Checking XML files...
http://jenkins-master:8080/job/phabricator/2125/ for more details.
Comment Actions
Executing section Default... Executing section Source... Executing section JS... Executing section XML GUI...
http://jenkins-master:8080/job/phabricator_lint/596/ for more details.