beyond-all-reason / beyond-all-reason/Beyond-All-Reason
Tweakunits with array values should replace instead of merge
- Dominant language
- Lua
- Stars
- 4.2k
- Forks
- 670
- Avg merge
- 1d 8h
- Merged PRs (30d)
- 314
Description
### Description
Find the following tweakunits:
```
{ armck = { buildoptions = ["armpw"] } }
```
The intended behavior should be to replace the buildoptions instead of merging with the current buildoptions, I can't give the expected output but suffice to say the original buildoptions exist and something "unexpected" happens (buildoption is appended to existing array or changed just in-place, e.g. index 1)
Find [`table.mergeInPlace`](https://github.com/beyond-all-reason/Beyond-All-Reason/blob/master/common/tablefunctions.lua#L59-L61) section on deep merging tables.
Notice it has some logic that looks like checking if the value is a dictionary or array table, but the logic seems faulty since arrays are also dictionary tables (just with numbers as keys), notice there might be mechanisms by which tables don't have type == "table" but I'm not aware of.
This bug should either be resolved by:
- Using a merge strategy and algorithm suitable for tweakunits expectations: array values with simple contained objects (not tables) should overwrite instead. e.g. `merge({a: [1,2]}, {a: [3]}) -> {a: [3]}`
- Consider if there's a bug or not on `table.mergeInPlace`, requires an investigation on its use case for the current logic and how it affects BAR on the places its used
### Remarks
- Investigate if `table.mergeInPlace` is like this due to merging of arrays with table values, e.g.: `merge({a: [{b: 1}]}, {a: [{c: 2}]}) -> {a: [{b:1, c:2}]}` is desired. If this is indeed desired then make your own merge strategy or find a suitable one to be used for tweakunits.
- Investigate whether the current behavior is desirable in any way for other array specifiable attributes in unitdefs
### See also
- [Tweakunits merge section](https://github.com/beyond-all-reason/Beyond-All-Reason/blob/master/gamedata/unitdefs_post.lua#L270)
### Expected Behaviour
Tweakunits `{ armck = { buildoptions = ["armpw"] } }` does replace all armcks buildoptions, only armpw is buildable by armck
### Actual Behaviour
Tweakunits `{ armck = { buildoptions = ["armpw"] } }` does not replace all armcks buildoptions
### Reproduction steps
Tweakunits `{ armck = { buildoptions = ["armpw"] } }` does replace all armcks buildoptions, only armpw is buildable by armck
### Other
_No response_
Contributor guide
Research direction
Start with table.mergeInPlace in common/tablefunctions.lua around lines 59-61, then inspect its use in the tweakunits merge section of gamedata/unitdefs_post.lua around line 270. Determine whether array values should replace existing buildoptions while preserving any required table-valued array behavior. Done means the armck tweakunit leaves only armpw buildable without breaking other merge use cases.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- lua
- Domain
- game-dev
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 38/100