Refactor generic effect utility functions
Description
In /lua/effectutilities.lua there are several utility functions that make it slightly easier to work with effects. The functions in question are:
CreateEffectsCreateEffectsWithOffsetCreateEffectsWithRandomOffsetCreateBoneEffectsCreateBoneEffectsOffsetCreateBoneTableEffectsCreateBoneTableRangedScaleEffectsCreateRandomEffectsScaleEmittersParam
There are several issues with these functions:
- They allocate, populate and return a new table on each call
- The table is populated using table.insert
- All the global functions are not upvalued
In particular the first one is an issue for two reasons:
- The result (the allocated table) is almost never used, and therefore thrown away again
- The functions use the old garbage management method using tables, while they should be using trashbags as done by https://github.com/FAForever/fa/pull/3743
But there's an issue: these functions have existed since the start of the game. Therefore countless mods rely on these functions to return a table by default. A quick scan through my personal mods folder show that this is the case: the Blackops and Total Mayhem mods use these functions.
Course of action
We can not adjust the input and output of the functions as that may impair with mod compatibility. Therefore this issue has two sides: one to improve the performance and deprecate the original functions and one to introduce new functions that have a different input / output and replacing all the instances on the base repository.
Part 1: Creating the new functions
This part consists of five steps:
- We copy all the functions and append
Optito the end of the function name. This indicates that this is the optimized version of the function. The functions should reside in/lua/EffectUtilitiesGeneric.lua, and imported into/lua/effectutilities.lua. - We introduce an additional parameter to all of these new functions: the bag that stores the result. This bag can be nil if we're not interested in storing the result - as the general case is.
- We scope any global tables / functions as upvalues for these new functions.
- We document the new functions with the same style as all the other recently-revamped functions have in the effect utilities files.
- We replace any instance in the repository to the new, optimized instances.
Note that the bag paramater must be compatible with being a table or a trashbag. Therefore we must use table.insert (but then TableInsert as an upvalue) to add populate the bag. We can not keep a separate counter, as we'll do with the deprecating old functions.
Part 2: Deprecating the old functions
This part consists of ... steps:
- We introduce deprecation warnings to the old functions, similar to the deprecation warning added to
CreateCybranBuildBeams. - We keep a separate counter to populate the tables to prevent the call to
table.insert, as shown in the benchmark that populates a table - We make sure that all global values are scoped as an upvalue
By all means the function should have the exact same behavior. Therefore they should always allocate a new table and they should always return that table, populated with effects.
As you're likely to do part 1 first, make sure to base the pull request of part 2 on a commit that doesn't include part 1. This makes it easier to test whether you indeed did not change the original behavior.
Test plan
This issue should be resolved with two pull requests:
- The first pull request tackles part 1
- The second pull request tackles part 2
Make sure that you base your branch from (a commit of) deploy/fafdevelop for each branch. The order that you complete this issue is not relevant as you should base your branch from a version that doesn't contain the changes of the other pull request.
Before you start it is important to have an idea where these functions are used. You can do this using the Find in Files (CTRL + SHIFT + F) operation of Visual Studio Code. Not all of these functions are used in the base game - if that is the case you can look for a mod in your mods folder. If it isn't used there either, then you'll have to make sure that the original behavior is still intact after changing the function.
Please report what units you used to test these functions - that makes it easier for the reviewer 😃 .
If you have any questions, please ask them in this issue or on Discord in the #game-repository channel. If you do not have write access to that channel you can give yourself the Tester role in the #role-selection channel.
If you claim this issue then please claim it in the Github UI or state in a reply to this issue that you claimed it.
Learning goals
Get more comfortable working with the repository and gain more experience on refactoring across multiple files.