Possibly related HRM#849859 Lua callbacks are called during illegal game positions
Looked through a bit, there are loads of work... Possibly we should divide the task to keep the patches reviewable, e.g. one patch for checking just existence and next one for more sophisticated checks.
I have currently done about 50% of 3.0 patch (without testing). Some parts I leave to handle in the mentioned HRM ticket since those cycles can't be resolved for good with scripts actively meddling.
Uff, made a patch for 3.0, have not even compiled it yet. Next I'll need a terrible script to test removing all possible things in all possible cases, and to make the autogame live long enough for these cases, I'll need probably HRM#849859 compiled before. And then, the other branches...
There is a patch and a function that is intended to test it being tied to all the callbacks. I tied it to "unit_moved" only but that broke AI civil units, so no cities testing (but nothing has failed due to dangling pointers til the explorers crept around).
- search_homecity_for_caravan(): It might be better to set 'alive' rather than to return immediately - if the unit IS still alive, there might be more code added after that block in the future
- Should we think something else for those comments about a HRM ticket. There's a fair chance that the comments outlive the hrm ticket availability. Maybe that's one ticket that we should just move to osdn for having osdn ticket number for the comments?
- citytools.h: Is the indentation of the parameter lines of the transfer_city_units() wrong (and having tabs?) like it seems reading the patch? If you touch it anyway (for removing trailing space?), might as well fix that.
Overall this should make a great improvement to the situation. Hopefully there's not too big conflicts in porting it to later branches.
--
For future autogame (regression) testing it would be great if we had a ruleset with a lua script doing all kind of nasty things, or actively using lua script in general.
btw. The script does not apply against current S3_0. Seems like minor rebase needed.
Reply To cazfi
- Should we think something else for those comments about a HRM ticket. There's a fair chance that the comments outlive the hrm ticket availability. Maybe that's one ticket that we should just move to osdn for having osdn ticket number for the comments?
Well, your suggestions? I just need some pattern to mark the code that need further dealing with.
Reply To ihnatus
Reply To cazfi
- Should we think something else for those comments about a HRM ticket. There's a fair chance that the comments outlive the hrm ticket availability. Maybe that's one ticket that we should just move to osdn for having osdn ticket number for the comments?
Well, your suggestions? I just need some pattern to mark the code that need further dealing with.
All the options have their downsides, and this is not very important anyway -> after thinking a while, my suggestion would be that we live with it as it's in the current patch. (my original comment was a question, not a request for a change)
Reply To cazfi
All the options have their downsides, and this is not very important anyway -> after thinking a while, my suggestion would be that we live with it as it's in the current patch. (my original comment was a question, not a request for a change)
Ok, this is not changed. Other your notices are accepted.
You know that your process of first developing patch for the most stable branch and then going towards development master is backwards compared to the usual way? Not that it's any of my business, if that's the way you want to work, and with some fixes it does make sense (esp. if you can reproduce the problem in specific branch only). Only that we now have 3.0 patch waiting already, but we have no version to have in testing in unstable branches.
Reply To cazfi
You know that your process of first developing patch for the most stable branch and then going towards development master is backwards compared to the usual way? Not that it's any of my business, if that's the way you want to work, and with some fixes it does make sense (esp. if you can reproduce the problem in specific branch only). Only that we now have 3.0 patch waiting already, but we have no version to have in testing in unstable branches.
I did it this way because number of signals grows in further versions and I wanted to start with a simpler task. Testing all these places already needs a lot of loop jumping. Probably I'll first make the HRM patch for 3.0 to not bother bypassing still dangerous code parts and test them together, and then will gradually advance both to the newer versions.
Reply To ihnatus
I did it this way because number of signals grows in further versions and I wanted to start with a simpler task.
The one option to get something in quicker would be to limit the scope of this ticket to just those signals that are present in 3.0, i.e., to port just current patch to later branches without adding handling for more signals in them. Those new signals could be handled in a separate ticket.
FYI. For exposing the current patch to some test use, I'll apply it to my local S3_0-work branch, and will also apply it to autogame runner for the next S3_0 testing round (my autogame runners are currently busy, mainly with with S3_1-d3f / S3_1-alpha3 stuff, but maybe next week I can have another S3_0 run)
Reply To cazfi
FYI. For exposing the current patch to some test use, I'll apply it to my local S3_0-work branch, and will also apply it to autogame runner for the next S3_0 testing round (my autogame runners are currently busy, mainly with with S3_1-d3f / S3_1-alpha3 stuff, but maybe next week I can have another S3_0 run)
I have it in S3_0 autogame test, but currently not in my own S3_0-work tree as it does not apply with some other patches there (so another rebase will be needed in the future)
Reply To cazfi
I have it in S3_0 autogame test
All the autogames had quickly failed on srv_main.c:1297 call of sanity_check(): "assertion 'pplayers_allied(((punit)->owner), city_owner(pcity))' failed."
I think that's from sanitycheck.c line 211; unit_owner(punit) macro is defined as "(punit)->owner"
Reply To cazfi
I think that's from sanitycheck.c line 211; unit_owner(punit) macro is defined as "(punit)->owner"
Hm, some function in city conquest code exits before transferring the city, can't find the place but it's something stupid...
Reply To ihnatus
Reply To cazfi
I think that's from sanitycheck.c line 211; unit_owner(punit) macro is defined as "(punit)->owner"
Hm, some function in city conquest code exits before transferring the city, can't find the place but it's something stupid
Just reading what the patch changes in unit_conquer_city(), this seems wrong:
+ city_remains = player_city_by_number(pplayer, saved_id)
+ && transfer_city(pplayer, pcity, 0, TRUE, TRUE, TRUE,
+ !is_barbarian(pplayer));
First you check that city is *already* owned by the same player (pplayer) that you then transfer the city to.
Reply To cazfi
Just reading what the patch changes in unit_conquer_city(), this seems wrong: + city_remains = player_city_by_number(pplayer, saved_id)
+ && transfer_city(pplayer, pcity, 0, TRUE, TRUE, TRUE,
+ !is_barbarian(pplayer));
First you check that city is *already* owned by the same player (pplayer) that you then transfer the city to.
As I have predicted, it was stupid.
I've got alien ruleset autogame failure with the latest patch, and I think the patch is to blame. It's the "1 citizens not equal to size (2)" from check_city_size() failing. There's create_city() call with size 2 in the backtrace (before this patch create_city() was always creating size 1 cities)
Reply To cazfi
I've got alien ruleset autogame failure with the latest patch, and I think the patch is to blame. It's the "1 citizens not equal to size (2)" from check_city_size() failing. There's create_city() call with size 2 in the backtrace (before this patch create_city() was always creating size 1 cities)
Probably, this long planned optimization of create_city() must have a separate ticket (any way, it needs rebasing).
EDIT: created, tested for 3.0, #44703
That problem is related to the realization of #44216 but actually parts of it (except player complete removal that is now impossible by scripts) already present in 3.0 branch. We must check after any direct or indirect call of any signal that any pointer on a unit, a city (or a player) we have obtained before the callback is valid when we use it after, and if so, that this object is still in the scope it is supposed to be (player is alive, city belongs to the same owner etc.) Maybe we have to rewrite some iterators in some places.
Note: generic existence test for a city or a unit is comparing pointer at their saved index to their saved pointer. For players, this test is not 100% reliable since player indices may be reused but at least it guarantees that we get some valid player pointer, and the probability of creating new player object at the same memory address is low on most OSs.