Page MenuHomeWildfire Games

Add safeguards for all queryOwnerInterface calls
ClosedPublic

Authored by bb on Wed, Aug 26, 1:21 PM.

Details

Summary

While reviewing D2810, I noticed that many of the Interfaces queried with queryOwnerInterface don't have safeguards for nonexistance. Usually not a problem since they exists, however they need to be checked.

Test Plan

grep and see all have safeguards now
Agree with the proposed safeguards
Observe effectively nothing changes

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

bb created this revision.Wed, Aug 26, 1:21 PM
Owners added a subscriber: Restricted Owners Package.Wed, Aug 26, 1:21 PM
bb requested review of this revision.Wed, Aug 26, 1:29 PM

Seems good overall

binaries/data/mods/public/simulation/components/ProductionQueue.js
452 ↗(On Diff #13294)

I feel like this should be a straight warning/error. Seems buggy, particularly since you've already subtracted resources.

831 ↗(On Diff #13294)

Likewise -> makes no sense to have a tech template if the owner has no technology manager

894 ↗(On Diff #13294)

samesies

Freagarach added inline comments.
binaries/data/mods/public/simulation/components/Promotion.js
88 ↗(On Diff #13294)

Can't we use the template value when we have no player?

binaries/data/mods/public/simulation/helpers/Commands.js
302 ↗(On Diff #13294)

Perhaps update the warnings as well?

Angen added a subscriber: Angen.Thu, Aug 27, 2:14 PM
Angen added inline comments.
binaries/data/mods/public/simulation/components/Promotion.js
88 ↗(On Diff #13294)

no player means owner is invalid so entity is mostly dead

bb updated this revision to Diff 13330.Fri, Aug 28, 6:59 PM
bb marked 2 inline comments as done.
bb added inline comments.
binaries/data/mods/public/simulation/components/ProductionQueue.js
452 ↗(On Diff #13294)

A second look showed, we already check for cmpTechnologyManager in this.GetTechnologiesList, so strictly we don't need to check this. Don't really like querying the interface twice, but out of scope

binaries/data/mods/public/simulation/components/Promotion.js
88 ↗(On Diff #13294)

Afaik the template doesn't contain a player ID

binaries/data/mods/public/simulation/helpers/Commands.js
302 ↗(On Diff #13294)

not much to change here, since no cmpPlayerEntityLimits means no limits, hence we just pass by

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/3086/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/1434/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/2533/display/redirect

Freagarach added inline comments.Fri, Aug 28, 7:03 PM
binaries/data/mods/public/simulation/components/Promotion.js
88 ↗(On Diff #13294)

I meant using +template.Promotion.RequiredXp. But Angen may be right and it may not be needed.

bb updated this revision to Diff 13331.Fri, Aug 28, 7:56 PM
Freagarach added a comment.EditedFri, Sep 18, 2:48 PM

This is complete and correct (awaiting the answer).

binaries/data/mods/public/simulation/components/ProductionQueue.js
383 ↗(On Diff #13331)

Should one still be allowed to train a restricted entity when there is no IID_EntityLimits? To me that equals having a limit of 0 so we should return here.

Freagarach accepted this revision.Fri, Sep 18, 3:02 PM
This revision is now accepted and ready to land.Fri, Sep 18, 3:02 PM
This revision was automatically updated to reflect the committed changes.