induktio / thinker

Improved game engine features for SMACX.
https://discord.gg/XdFuwWzzku
GNU General Public License v2.0
75 stars 11 forks source link

mod_base_production has side effects #19

Closed tnevolin closed 3 years ago

tnevolin commented 3 years ago

Original function proposes production choice and nothing else .text:004F81A0 base_prod_choice proc near ; CODE XREF: base_reset+C1↑p

mod_base_production modifies original function. It also calls consider_hurry which may modify base state. Original function doesn't do that.

It would be nice if you stick to original function purpose. Then I can safely redirect to your function for production suggestion. Currently I have to modify your function to ensure it doesn't silently modify base state under the hood.

induktio commented 3 years ago

The problem is these things don't have any well-defined APIs to follow. The implementation of consider_hurry basically has to modify base state in some way to accomplish it's goal. This shouldn't affect player bases ever because the functions don't get called on them. Maybe someday if base_reset gets rewritten the thing could be restructured in a cleaner way but currently I don't see how this interferes with how Thinker works.

tnevolin commented 3 years ago

That does not affect human player at all. This affects computer players for which both you and I create AI routines. I do not absolutely mind rewriting your code as needed. However, one of my priorities is to keep it intact if possible to make sure users experience same AI behavior.

induktio commented 3 years ago

Are you proposing to add some kind of a way for multiple AI routines to suggest base production? What would be the benefit for players, e.g. multiple strategy types for AI factions? Not sure what's the purpose there. Note that I have not been specifically reviewing stuff in other versions unless that code gets merged here.

tnevolin commented 3 years ago

Let me explain it again as clear as I can.

So if you want me to credit you for AI modifications you may be interested to modify your code in a way that is easy to reuse without altering. That's all. Otherwise, just say "Thank you but no" and forget about it. 😁

induktio commented 3 years ago

Just pointing out there area some games with APIs for pluggable AI modules, for example OpenRA. But development resources are limited and getting to that level is still quite a long ways off. So it's mainly about prioritization.

tnevolin commented 3 years ago

Eh. I don't think you want to get to that level! Just decoupling base_prod_choice from hurrying is enough for now. I would suggest to put hurrying into actual base_production where it belongs. If not there than at faction_upkeep level and just cycle bases by yourself and hurry as needed.

In this regard your mod_base_production name is confusing as it mods the base_prod_choice and not the base_production.

induktio commented 3 years ago

Probably should do something long term that enables more pluggable AI modules for bases for example. You can also look up ROTP, they also have multiple AI support.

But looking at the WTP code, it seems to mix up vanilla AI stuff, something else and Thinker base production logic for the same faction which is not the way it was designed to be. Doing so will almost surely mess up some AI priorities because they will not stay consistent for the same faction. There's probably other stuff like that in WTP code, not sure. Now you might want to run different AIs side-by-side on different factions and then observe the results. Otherwise I don't see any point in supporting or recommending features that just go against the design logic for Thinker here. AI design is kind of brittle for these kind of games, if one part doesn't do what it's supposed to be, then the coordination can fail badly.

tnevolin commented 3 years ago

Does it hurt your feeling seeing that your production suggestions are not used 100% of the time? Let me know if so. I will think of something.

induktio commented 3 years ago

Well, I'm not going to recommend any stuff that mixes up vanilla AI base code there. :) Thinker has been doing that stuff since the first version. It was also the first major AI feature that was implemented in this mod. Anyway I'll reorganize the base code further but can't really promise the code won't have side effects on the game state because it has to in some places.

tnevolin commented 3 years ago

Thanks.