As noticed in #4595, we can see the old actor in some cases.
In many other places (I didn't check all places), that's a common (good) practice to do that.
Details
- Reviewers
wraitii - Group Reviewers
Restricted Owners Package (Owns No Changed Paths) - Commits
- rP20393: Move out of world promoted, packed and upgraded entity as they are not…
- Trac Tickets
- #4595
-
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
Build has FAILED
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (306 tests).ERROR: JavaScript error: simulation/helpers/Transform.js line 100 TypeError: cmpPosition is null ChangeEntityTemplate@simulation/helpers/Transform.js:100:2 Pack.prototype.PackProgress@simulation/components/Pack.js:135:18 @simulation/components/tests/test_Pack.js:154: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_Pack.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 306 tests Success rate: 99%
Link to build: http://jw:8080/job/phabricator/1442/
See console output for more information: http://jw:8080/job/phabricator/1442/console
Executing section Default... Executing section Source... Executing section JS... binaries/data/mods/public/simulation/components/Promotion.js | 154| » » while·(true) | | [NORMAL] ESLintBear (no-constant-condition): | | Unexpected constant condition. binaries/data/mods/public/simulation/components/Promotion.js | 35| » if·(cmpCurrentUnitHealth.GetHitpoints()·==·0) | | [NORMAL] JSHintBear: | | Use '===' to compare with '0'. binaries/data/mods/public/simulation/components/Promotion.js | 79| » var·pos·=·cmpCurrentUnitAI.GetHeldPosition(); | | [NORMAL] JSHintBear: | | 'pos' is already defined. Executing section XML GUI... Executing section Python... Executing section Perl...
http://jw:8080/job/phabricator_lint/103/ for more details.
binaries/data/mods/public/simulation/templates/template_unit_infantry_ranged_slinger.xml | ||
---|---|---|
36 ↗ | (On Diff #2374) | (unrelated) |
Perhaps it could also be solved by calling MoveOutOfWorld from CCmpPosition.cpp OnOwnershipChange to -1?
binaries/data/mods/public/simulation/components/Promotion.js | ||
---|---|---|
126 ↗ | (On Diff #2374) | (No if-statement needed unless the other callers get one too.) |
binaries/data/mods/public/simulation/helpers/Transform.js | ||
100 ↗ | (On Diff #2374) | That what Vulcan said. (In this file we have a check for the definition of cmpPosition, so this call should get a check too) |
Eh, OnEntityRenamed that is, as we don't want to move units with the death type remain out of world.
binaries/data/mods/public/simulation/helpers/Transform.js | ||
---|---|---|
100 ↗ | (On Diff #2374) | Yes, those check things needs to be checked :p |
It seems indeed that in all cases (at least most of the one I checked), the sending of that message should have a MoveOutOfWorld with it. Despite, I don't "feel" good to have to listen to that message and to do that in the Position component.
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/1776/ for more details.
Executing section Default... Executing section Source... Executing section JS... binaries/data/mods/public/simulation/components/Promotion.js | 155| » » while·(true) | | [NORMAL] ESLintBear (no-constant-condition): | | Unexpected constant condition. binaries/data/mods/public/simulation/components/Promotion.js | 35| » if·(cmpCurrentUnitHealth.GetHitpoints()·==·0) | | [NORMAL] JSHintBear: | | Use '===' to compare with '0'. binaries/data/mods/public/simulation/components/Promotion.js | 79| » var·pos·=·cmpCurrentUnitAI.GetHeldPosition(); | | [NORMAL] JSHintBear: | | 'pos' is already defined. Executing section XML GUI... Executing section Python... Executing section Perl...
http://jw:8080/job/phabricator_lint/337/ 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! Checking XML files...
http://jw:8080/job/phabricator/1777/ for more details.
Executing section Default... Executing section Source... Executing section JS... binaries/data/mods/public/simulation/components/Promotion.js | 155| » » while·(true) | | [NORMAL] ESLintBear (no-constant-condition): | | Unexpected constant condition. binaries/data/mods/public/simulation/components/Promotion.js | 35| » if·(cmpCurrentUnitHealth.GetHitpoints()·==·0) | | [NORMAL] JSHintBear: | | Use '===' to compare with '0'. binaries/data/mods/public/simulation/components/Promotion.js | 79| » var·pos·=·cmpCurrentUnitAI.GetHeldPosition(); | | [NORMAL] JSHintBear: | | 'pos' is already defined. Executing section XML GUI... Executing section Python... Executing section Perl...
http://jw:8080/job/phabricator_lint/338/ for more details.
Fix previous not tested update. Adress one lint complain. Ignore the === and the while(true) complains.
Executing section Default... Executing section Source... Executing section JS... binaries/data/mods/public/simulation/components/Promotion.js | 155| » » while·(true) | | [NORMAL] ESLintBear (no-constant-condition): | | Unexpected constant condition. binaries/data/mods/public/simulation/components/Promotion.js | 35| » if·(cmpCurrentUnitHealth.GetHitpoints()·==·0) | | [NORMAL] JSHintBear: | | Use '===' to compare with '0'. Executing section XML GUI... Executing section Python... Executing section Perl...
http://jw:8080/job/phabricator_lint/343/ 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! Checking XML files...
http://jw:8080/job/phabricator/1782/ for more details.
Why don't we make a function Engine.DestroyEntityNow(entity_id_t ent) which destroys the entity immediately (by calling DestroyEntity and FlushDestroyedEntities)? Yes it would destroy all entities in queue but why would that be a problem? If that is a problem we can also move the logic for destroying 1 ent to a new function and use that in FlushDestroyedEntities.
In those cases, we destroy the entity and we just want it not intefering with the 'world'. (Like we do in other places).
I think the cleanest solution would be to send a "prepare for Destroy" message in ComponentManager::DestroyComponentSoon() and have CCmpPosition subscribe to that and move itself out of the world there.
Destroying "now" might lead to inefficiency and weird hiccups (GC-like) and I'd rather avoid that.
@wraitii thanks for your input!
Isn't it a bit "use a sledgehammer to crack a nut"? (adding that kind of (complex or not) stuff in the engine as we just move out of world in other places - preview, formation, foundation...). Perhaps in some cases (I can't imagine) we don't want to do that.
(No conviction, just a thought)
Destroying "now" might lead to inefficiency and weird hiccups (GC-like) and I'd rather avoid that.
same!
Adding a new message that wouldn't even be broadcast doesn't seem like a huge increase in complexity to me but maybe I'm wrong. I guess the question is "are there other components that might benefit from this behaviour?" How many other behaviour currently implemented in "Destroy" would be better implemented in "PrepareToDestroy"?
If it's basically just this case, I'd say fair enough to just fixing the code in relevant places, otherwise it might be worth abstracting. Then again, it might be worth fixing this first and investigating after. So I'm reverting my judgment to an OK with this caveat.