UnitAI breakup

[sanderd17]

UnitAI is a bit of a monster to edit. It is so by concept. It will always be a monster if it has to do everything unit related.

So I've thought of splitting it up. My proposal doesn't change the code at all, but it changes where the code it put. IMO it's more logical to put the Unit logic for resource gathering in the ResourceGatherer component, unit logic for Garrisoning maybe in the GarrisonHolder component ...

That way, if you want to change something to a certain topic (like change the attack, resource gathering, garrisoning ...) you should only be in one file, where everything is together.

UnitAI itself should only take care of the very general behaviours (like moving). As in the other files, the UnitAI prototype and the FsmSpec are extended, this means the UnitAI has to run part of its code before all the rest, and the registration of the FsmSpec after the rest.

See my proposal where the changes are in for the Resource gathering. (note that the diff is a bit messed up. %UnitAI is a new file, and it's diffed with UnitAI, while UnitAI in the diff is marked as deleted)

[Philip Taylor]

Mixing the code for multiple components in a single file sounds kind of ugly to me (it breaks the modularity of components, where each file has a clear scope (i.e. to implement a single component) and there's no ambiguity over where a particular function should go). Splitting the code for one component into a random subset of a large number of files sounds annoying (since if you want to make a global change to UnitAI, or get a global overview of what it's doing, you'll have to open every single component (even the irrelevant ones) in your text editor).

Maybe just create a subdirectory for all of UnitAI? so you'd have simulation/components/UnitAI/Resource.js, UnitAI/Formation.js, UnitAI/Core.js, etc. Then all the UnitAI code would be in one coherent place, but would still be better organised than a single huge file.

[historic_bruno]

I agree with Philip, not only is it ugly, but so far there's not much unit-specific about ResourceGatherer. A structure or non-moving entity could be a resource gatherer, but if you start introducing UnitAI there, maybe we would need another component like BuildingResourceGatherer that duplicates most of the logic. Other similar components that come to mind: Attack, Armour, Heal, Pack (see #1919). Sure, you could mix UnitAI logic with them, but would you really want to?

The yuckiest part of UnitAI seems to be the 3000-line FSM JS object, if you take that away, there's only 2800 lines left, mostly reasonable functions and some comments/whitespace. I don't know if it's easier to comprehend and modify logic across many small files or one large file, without a decent IDE to manage them.

[sanderd17]

The logic is not mixed in a technical way. The UnitAI prototype is still a giant object, and the UnitFsmSpec too. But it's just split out per theme into different files.

That said, I do like the option of making new files instead of extending existing ones.

My main concern is that, now, if you want to change something to a component related to UnitAI (like Attack) you need to open at least two files (like Attack.js and UnitAI.js), but the most annoying is that the Attack code isn't grouped together in UnitAI. So you have to scroll over thousands of lines to see what a method called in the FsmSpec does. And scroll thousands of lines back to see how it's used. I don't know of any IDEs supporting this sort of code structure very well. Most tend to support multiple smaller files a lot better (where you can open them next to each other, and read the code easier).

Having related methods close to their FsmSpec usage would help a big deal IMO. The General UnitAI code can be in a separate, small file. Where you can again open it next to your other files.

I now see two ways of having this. Either

components/UnitAI/Core.js
components/UnitAI/ResourceGather.js
components/UnitAI/Attack.js
...

or

components/UnitAI.js
components/UnitAIResourceGather.js
components/UnitAIAttack.js
...

The latter has the advantage that it's loaded in a logical order (i.e. the core with the initialisation before everything else). But is would make the components directory a lot bigger (I don't think that's a problem though). For the former, you need other naming conventions to get the loading right, or put exceptions in the loading order.

In any case, this looks way better than my first proposal or the existing UnitAI.

[wraitii]

After the staff meeting on August 3 it has been agreed that splitting UnitAI is probably too difficult for a not-so-obvious advantage in readability. It has also been agreed that UnitAI should probably be cleaned up somehow.

[Freagarach]

Refs. r25206, r25207, r25208, r25235. (Also #6081 and some more commits on UnitAI.)

[Freagarach]

As mentioned above, the actions are split up, cleaning some logic from UnitAI. Also the file itself has had some cleaning passes (more to be done for sure).

However, breaking up the FSM into separate files was found to be not so advantageous, hence closing as "won't fix". Feel free to argue otherwise.