Page MenuHomeWildfire Games

Don't regenerate status bars for destroyed entities.
AbandonedPublic

Authored by fatherbushido on Feb 14 2017, 3:31 PM.

Details

Reviewers
None
Trac Tickets
#4480
Summary

The rev rP19092 triggered a bug. When a global aura target is destroyed, Auras component try to remove its bonus and request StatusBar to regenerate. That part is itself allowed by other things.

Test Plan

(In game "Test":

  • take macedonians, build demetrius hero and a boltshooter.
  • withtout the patch:
    • destroy the Boltshooter: get a warning/error
    • unpacked the Boltshooter: you'll get a segfault)

There are different way to prevent that. Basically transforming the error message in StatusBars in an early return should be sufficient (Imo it's too much hidding things).
Input are welcome.
The change in GiveMembersWithValidClass needs to be consider carefully.

If ever that would be commited, it should be after D264.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 523
Build 834: Vulcan BuildJenkins
Build 833: arc lint + arc unit

Event Timeline

fatherbushido created this revision.Feb 14 2017, 3:31 PM
Vulcan added a subscriber: Vulcan.Feb 14 2017, 4:15 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/358/ for more details.

fatherbushido edited the summary of this revision. (Show Details)Feb 14 2017, 10:38 PM
fatherbushido added a reviewer: Restricted Owners Package.

(Patch is incomplete as point out by leper, some other stuff should be adressed)

fatherbushido planned changes to this revision.Feb 19 2017, 5:55 PM

Should this be committed anyways?

In D143#9980, @wraitii wrote:

Should this be committed anyways?

That is 'enough' to come back to the previous state but while at it, we can do the good job.
As leper pointed out somewhere, also the Auras itself do things it shouldn't.
There is also something in Pack that I want to put in the table. At last, in StatusBar I don't know if we should let him did bad things (even if not noticeable), checking and throwing error, or just checking.

fatherbushido retitled this revision from Adress concern in rP19092 to Don't regenerate status bars for destroyed entities..Apr 2 2017, 10:31 AM
fatherbushido edited the summary of this revision. (Show Details)
fatherbushido edited the test plan for this revision. (Show Details)
fatherbushido edited the test plan for this revision. (Show Details)

Update

fatherbushido edited the test plan for this revision. (Show Details)Apr 2 2017, 10:36 AM
wraitii added inline comments.Apr 2 2017, 10:52 AM
binaries/data/mods/public/simulation/components/AuraManager.js
250

Might be worth commenting why this is useful and/or necessary.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/680/ for more details.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/681/ for more details.

fatherbushido edited the summary of this revision. (Show Details)Apr 2 2017, 2:42 PM
fatherbushido edited the test plan for this revision. (Show Details)
fatherbushido added inline comments.Apr 3 2017, 2:49 PM
binaries/data/mods/public/simulation/components/AuraManager.js
250

Something like "Don't try to apply or remove bonus for units which are destroyed."
That has to be discussed though.
It is redundant with the added filter in Auras.js.
So it's a security or an optimization.
(If ever we want to remove the bonus from the modification cache, perhaps it should be done here.)

binaries/data/mods/public/simulation/components/Auras.js
247 ↗(On Diff #1073)

units -> entities

sanderd17 edited edge metadata.Apr 4 2017, 6:43 PM

IMO, it should be solved more in the Statusbars and rendering code instead of the Auras. As long as the StatusBars component exists, it should be able to accept all re-rendering requests.

For the regular error message, it should be enough to check the owner != -1 arount line 194 of the statusbars.

But then it all results in a segfault which shouldn't happen (and that's a bug in the C++ part, as no JS code should be able to cause a segfault).

fatherbushido added a comment.EditedApr 14 2017, 11:38 AM

IMO, it should be solved more in the Statusbars and rendering code instead of the Auras. As long as the StatusBars component exists, it should be able to accept all re-rendering requests.

For the regular error message, it should be enough to check the owner != -1 arount line 194 of the statusbars.

But then it all results in a segfault which shouldn't happen (and that's a bug in the C++ part, as no JS code should be able to cause a segfault).

We can indeed just prevent the status bar to regenerate (changing the error in the current diff by an early return)
Or just fix the Pack component and add an early return in the capture bar part.
Or the current diff approach.

Just need some consensus about the best way of doing stuff.

wraitii resigned from this revision.Apr 15 2017, 9:08 AM

I will split that in two parts.

leper edited edge metadata.Apr 23 2017, 12:22 AM

I will split that in two parts.

As D357 is merged, can we have an update of this?

Update after D357.

fatherbushido added a comment.EditedApr 23 2017, 7:36 AM

@leper: Thanks for the remainder. I'll do that today

EDIT: done

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

I think it's now mainly an aura patch. It will be need to rewrite some clean test of Auras component functions.

fatherbushido abandoned this revision.May 4 2017, 8:10 AM
fatherbushido removed reviewers: Restricted Owners Package, leper, sanderd17, wraitii.

Well following rP19350 rP19423 rP19424 rP19429 rP19451, nothing really remains here.
The Auras function itself should be checked (tested) individually in the way they handle dead units. I would leave that for another diff.