We-the-People-civ4col-mod / Mod

This is the repository where the mod resides.
90 stars 37 forks source link

EventTrigger: improve and optimize #436

Open Nightinggale opened 3 years ago

Nightinggale commented 3 years ago

Looking at CvPlayer::getEventTriggerWeight, I spotted a few places, which can be improved. As this function is called while looping all event triggers, performance should be considered, particularly if the coding difficulty and effort is low. Also it will produce cleaner code, which could help avoid bugs.

bools

Add the following bools to xml:

This will allow setting what kind of player the event can trigger on. Unlike the current system, having access to those simple bools will allow quick rejection for players of the wrong type (like natives not looking at European only events). If all 3 are false, then set all 3 to true, hence rely on the current code. As such this system can be added without changing the existing xml and we can then add those tags to events one by one as needed. It only boosts performance if the tags are added though. Bools needed for animals and church? Not sure.

isTriggerFired

isTriggerFired is based on a vector of indexes, meaning if event trigger 7 fires, it adds 7 to the end of the vector. When asking if a specific event trigger has been used, it will have to search the vector for the index in question. EnumMap<EventTriggerTypes, bool> will be a list of bools meaning checking a specific index will translate to reading a specific offset while ignoring the rest. Much faster. The vector approach was likely added in vanilla to save memory, but EnumMap packs the data much better than vanilla. It uses a fixed 40 bytes (or 4 if all are false) while the vector uses 12 bytes + 4 bytes for each triggered event. This means they use the same amount of memory when 7 events have triggered. If anything, the argument for saving memory is now to not use the vector.

raystuttgart commented 3 years ago

Currently the

"bEuropean" "bNative" "bKing"

is checked by checks in Python coding in the functions called by the XML tags PythonCanDo PythonCanDoCity

This has also always worked ... Unless the code was messed up but then you normally get Python Exceptions when testing already.

But the Events can be triggered and they can be closed properly during testing. (You can test it by WorldBuilder only - you do not even need Python Cheat Menu.)

Currently I have no explanation for the things reported: (Also never could reproduce - but I also may have not played long enough.)

@Raubwürger said: Sometimes Quests can not be ended ... (absolutely no explanation for that yet) @Nightinggale said: Asserts are trigger that King is "NO_PLAYER" (absolutely no explanation for that yet)


Summary: This need stime for more examination and analysis. Checked my Python code and it looks perfectly ok. (And as I said, the events can be properly trigger and closed by using Worlbuilder.)

Nightinggale commented 3 years ago

Currently the [enums] is checked by checks in Python coding in the functions called by the XML tags

Much slower than an early check in C++. Regardless of the need for a change of functionality, this proposal is a performance boost. That alone makes it worth considering regardless of current issues.

It may or may not end up as a solution to the current problems. If the current problems are fixed in python, then it would affect the urgency this proposal, but it won't make it useless.

raystuttgart commented 3 years ago

Hi Nightinggale,

Those general checks are used very often and thus should be made available in XML): (Also, similar checks already exist, e.g. "beHumanOnly", "otherPlayerIsNative", ...)

bEuropean
bNative
bKing

The problem with the rest of the checks simply is, that there are 100 possibilities of checks and in "and/or combinations" and also in a certain order. (And it will get too complicated to model that in XML tags instead of writing actual logic.)

I give you examples:

Such cases can easily be checked by calling Python methods in "PythonCanDo" or in "PythonCanDoCity". And to additionally help configuring balancing parameters there are the 4 GenericParameters to check for Gold, Goods, ...

It is a system for programmers I admit but most of the "PythonEvents" and "PythonQuests" were and probably always will only be possible, with the help of a programmer.

Even in the current system our "non-programmers" already get lost with all the XML tags. It is simply too much for them already. Without getting either very detailled examples or complete step-by-step guides hardly any Events could or would have been created by "non-programmers".

Making the system "non-programmer" friendly by adding more XML tags is an illusion that only we programmers might have, because we then program the XML tags and then understand them.

By the way:

  1. The bugs we had the last days would not have been caused if not basic mistakes would have been made. (Forgetting to actually implement the checks you call which would have been simple copy&paste almost from the more than 50 working examples already created.)

  2. Checking for the crazy amount of XML tags we already have - if they were set correctly or not - is even much more difficult than checking the actual Python Code beause there you at least get Python Exeptions.

Summary:

The 3 XML tags above are perfectly fine to me. And yes they would be useful.

But please let us not fall into the trap to believe that 20 more XML tag options are easier to use and maintain, than writing maybe 20 lines of code speficially for every Use Case you need.

We should consider very carefully which XML tags can actually be used generically. Python Events and Python Quest will never become a feature for pure XML modders anyways. (It is already too complex and offers too many different variations.)