Page MenuHomeWildfire Games

Don't initialise null values in cmpGarrisonHolder.
Needs ReviewPublic

Authored by Freagarach on Jan 25 2021, 7:53 AM.

Details

Reviewers
None
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Trac Tickets
#5979
Summary

The allowGarrisoning is (for now) only used by the plane, so it is rather unnecessary to create an empty map on init of each GarrisonHolder and serialise that throughout a game.
As for the timer, initialising it to undefined is also not necessary. this.timer is now created when needed and deleted when not needed anymore.
One could argue the same for this.entities but that is such an integral part of a GarrisonHolder that I did not change it.

Test Plan

Verify garrisoning still functions, also in the cheat plane.
Verify also that garrisoned healing still occurs.

Event Timeline

Freagarach created this revision.Jan 25 2021, 7:53 AM
Freagarach added inline comments.Jan 25 2021, 7:55 AM
binaries/data/mods/public/simulation/components/GarrisonHolder.js
112

Notice that if one would split this function into AllowGarrisoning and DisallowGarrisoning one could delete the map when all callers allow garrisoning. Not sure whether that is wanted or not, but certainly out of scope.

Build has FAILED

builderr-debug-macos.txt
ld: warning: text-based stub file /System/Library/Frameworks//CoreAudio.framework/CoreAudio.tbd and library file /System/Library/Frameworks//CoreAudio.framework/CoreAudio are out of sync. Falling back to library file for linking.
ld: warning: text-based stub file /System/Library/Frameworks//AudioToolbox.framework/AudioToolbox.tbd and library file /System/Library/Frameworks//AudioToolbox.framework/AudioToolbox are out of sync. Falling back to library file for linking.
ld: warning: text-based stub file /System/Library/Frameworks//ForceFeedback.framework/ForceFeedback.tbd and library file /System/Library/Frameworks//ForceFeedback.framework/ForceFeedback are out of sync. Falling back to library file for linking.
ld: warning: text-based stub file /System/Library/Frameworks//CoreVideo.framework/CoreVideo.tbd and library file /System/Library/Frameworks//CoreVideo.framework/CoreVideo are out of sync. Falling back to library file for linking.
ld: warning: text-based stu

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/3048/display/redirect
See console output for more information: https://jenkins.wildfiregames.com/job/macos-differential/3048/display/redirectconsole

Freagarach requested review of this revision.Jan 25 2021, 7:57 AM
Freagarach updated this revision to Diff 15677.Jan 25 2021, 8:18 AM

Logic error.

Build is green

builderr-debug-macos.txt
ld: warning: text-based stub file /System/Library/Frameworks//CoreAudio.framework/CoreAudio.tbd and library file /System/Library/Frameworks//CoreAudio.framework/CoreAudio are out of sync. Falling back to library file for linking.
ld: warning: text-based stub file /System/Library/Frameworks//AudioToolbox.framework/AudioToolbox.tbd and library file /System/Library/Frameworks//AudioToolbox.framework/AudioToolbox are out of sync. Falling back to library file for linking.
ld: warning: text-based stub file /System/Library/Frameworks//ForceFeedback.framework/ForceFeedback.tbd and library file /System/Library/Frameworks//ForceFeedback.framework/ForceFeedback are out of sync. Falling back to library file for linking.
ld: warning: text-based stub file /System/Library/Frameworks//CoreVideo.framework/CoreVideo.tbd and library file /System/Library/Frameworks//CoreVideo.framework/CoreVideo are out of sync. Falling back to library file for linking.
ld: warning: text-based stu

See https://jenkins.wildfiregames.com/job/macos-differential/3049/display/redirect for more details.

Stan added a subscriber: Stan.Jan 25 2021, 8:23 AM

Think you forgot tests :)

binaries/data/mods/public/simulation/components/GarrisonHolder.js
115

While at it, I find the variable name a bit misleading.

I think there is a reason it's a map over an array (performance?), but it could be a WeakMap() (it has some advantages IIRC see the documentation on MDN.

Wouldn't it be better to initialize it once in the ctor iif the relevant parameters are there instead of adding a check for all the other cases at each call?

Stan added inline comments.Jan 25 2021, 8:24 AM
binaries/data/mods/public/simulation/components/GarrisonHolder.js
122

I think ternary might be safer here.

Angen added a subscriber: Angen.Jan 27 2021, 8:21 PM

Did not test it, but looks ok by reading the code.

Freagarach added inline comments.Jan 27 2021, 8:34 PM
binaries/data/mods/public/simulation/components/GarrisonHolder.js
115

I'd rather not touch the internals here.

It could be yeah, but there is no such thing as relevant parameters.

122

How do you mean safer?