Page MenuHomeWildfire Games

Move out world promoted, packed or upgraded before destroying.
ClosedPublic

Authored by fatherbushido on Jun 2 2017, 11:01 AM.

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
Summary

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.

Test Plan

-

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

fatherbushido created this revision.Jun 2 2017, 11:01 AM
Vulcan added a subscriber: Vulcan.Jun 2 2017, 11:48 AM

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.

fatherbushido added inline comments.Jun 2 2017, 12:23 PM
binaries/data/mods/public/simulation/templates/template_unit_infantry_ranged_slinger.xml
36 ↗(On Diff #2374)

(unrelated)

elexis edited edge metadata.Jun 2 2017, 2:02 PM

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)

elexis added a comment.Jun 2 2017, 2:15 PM

Eh, OnEntityRenamed that is, as we don't want to move units with the death type remain out of world.

fatherbushido added inline comments.Jun 4 2017, 10:20 AM
binaries/data/mods/public/simulation/helpers/Transform.js
100 ↗(On Diff #2374)

Yes, those check things needs to be checked :p

In D590#24271, @elexis wrote:

Eh, OnEntityRenamed that is, as we don't want to move units with the death type remain out of world.

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.

fatherbushido planned changes to this revision.Jun 5 2017, 8:10 AM

Remove a newline

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.

bb added a subscriber: bb.Aug 30 2017, 8:04 PM

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.

fatherbushido abandoned this revision.Aug 30 2017, 11:24 PM
In D590#33387, @bb wrote:

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.

I would not explore that kind of things...

fatherbushido reclaimed this revision.Oct 1 2017, 8:45 AM
fatherbushido added a reviewer: Restricted Owners Package.

In those cases, we destroy the entity and we just want it not intefering with the 'world'. (Like we do in other places).

wraitii requested changes to this revision.EditedOct 24 2017, 12:17 PM
wraitii added a subscriber: wraitii.

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.

This revision now requires changes to proceed.Oct 24 2017, 12:17 PM
fatherbushido added a comment.EditedOct 24 2017, 12:23 PM
In D590#38184, @wraitii wrote:

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!

wraitii accepted this revision.Oct 24 2017, 1:54 PM

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.

This revision is now accepted and ready to land.Oct 24 2017, 1:54 PM
This revision was automatically updated to reflect the committed changes.