beyond-all-reason / beyond-all-reason/Beyond-All-Reason

Tweakunits with array values should replace instead of merge

Open
#4,875 0 comments 0 reactions 0 assignees View on GitHub
Bug
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.