fa icon indicating copy to clipboard operation
fa copied to clipboard

Refactor generic effect utility functions

Open Garanas opened this issue 3 years ago • 0 comments

Description

In /lua/effectutilities.lua there are several utility functions that make it slightly easier to work with effects. The functions in question are:

  • CreateEffects
  • CreateEffectsWithOffset
  • CreateEffectsWithRandomOffset
  • CreateBoneEffects
  • CreateBoneEffectsOffset
  • CreateBoneTableEffects
  • CreateBoneTableRangedScaleEffects
  • CreateRandomEffects
  • ScaleEmittersParam

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 Opti to 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.

Garanas avatar Apr 03 '22 09:04 Garanas