Likely, 3.1 patch suites also for master.
Let's concentrate on master/S3_1 patch first. Whether we want to backport it to release branch remains to be seen.
- You still need to emit proper "city_size_change" and (deprecated) "city_growth" signals to not break scripts that track the size via them
- handle_edit_city_create() is about handling untrusted input from the client - do not assert() input validity, but check it even in release build
- "Set from UINT8 packet field, no need to check the top" sounds a bit like something to make a FC_STATIC_ASSERT() about, if possible
- "Max range is %d" - that's not a "range" but just one end of one - upper limit.
- The unit type "city_size" sanity check cannot go in this form to a d3f branch, as it prohibits zero "city_size" even when it does not break nothing (unit would not be able to build a city anyway). I wouldn't break S3_1 semi-freeze for this, but add the sanity check to master only, with rscompat code that updates value 0 from a 3.1 ruleset to 1.
- As for the "city_growth" signal that you need to emit once for each increment step, and that's already been deprecated for some time, I think it's the time to start thinking how to turn deprecated lua constructs to completely unsupported ones -> will open a ticket about that, but that's not going to get resolved for this need (esp. in stable branches)
Reply To cazfi
Let's concentrate on master/S3_1 patch first. Whether we want to backport it to release branch remains to be seen. - You still need to emit proper "city_size_change" and (deprecated) "city_growth" signals to not break scripts that track the size via them
One of the goals was removing the step by step growth procedure... But yes, we should leave it as it is for scripts in d3f'd versions, just probably remove the limitation (maybe in another ticket). I propose though removing the steps in master, one has "city_built" to record the size. For building a city with unit city_size > 1, "city_size_change" is always called once and limiting effects are not checked.
- handle_edit_city_create() is about handling untrusted input from the client - do not assert() input validity, but check it even in release build
Yes.
- "Set from UINT8 packet field, no need to check the top" sounds a bit like something to make a FC_STATIC_ASSERT() about, if possible
Well, I'll see...
- "Max range is %d" - that's not a "range" but just one end of one - upper limit.
Ok, just copied paradrop code...
- The unit type "city_size" sanity check cannot go in this form to a d3f branch, as it prohibits zero "city_size" even when it does not break nothing (unit would not be able to build a city anyway). I wouldn't break S3_1 semi-freeze for this, but add the sanity check to master only, with rscompat code that updates value 0 from a 3.1 ruleset to 1.
It breaks nothing but helptext with current code, so let's just correct it to 1 on load.
So we probably should retarget most of this ticket to S3_2 d3f and open some tickets for the noticed problems in earlier versions.
Reply To ihnatus
Reply To cazfi
- You still need to emit proper "city_size_change" and (deprecated) "city_growth" signals to not break scripts that track the size via them
One of the goals was removing the step by step growth procedure...
I think we can still have most of the benefits of that. As "city_growth" is already deprecated, and the state of the city during signal emission has never been well defined, I think it's enough to have minimal loop to emit those signals (probably just add +1 to city size and +1 to default specialists, and emit the signal on each round) - no need to do all the heavy stuff of setting city in a good state for each temporary size.
Reply To cazfi
- "Set from UINT8 packet field, no need to check the top" sounds a bit like something to make a FC_STATIC_ASSERT() about, if possible
Could not find a way, UINT8 packet fields still have type int, put a usual check.
Here is a patch for master, I hope I understood how rscompat works. Do you think we should still emit "city_size_change" signals on city creation in 3.2?
Reply To ihnatus
Here is a patch for master, I hope I understood how rscompat works.
You should be able to do this update already when loading the units, instead of postponing it to rscompat_postprocess(). Then you could have strict check in sanity_check_ruleset_data() that the ruleset is sane at that point - no need to allow special cases for a ruleset being updated.
E.g. Add rscompat_unit_adjust_3_2() function to rscompat.ch, and call that for each unit loaded (after all data is read) in load_ruleset_units()
Do you think we should still emit "city_size_change" signals on city creation in 3.2?
Don't see way around it at the moment.
So, this is currently blocker for #44312
Reply To cazfi
- As for the "city_growth" signal that you need to emit once for each increment step, and that's already been deprecated for some time, I think it's the time to start thinking how to turn deprecated lua constructs to completely unsupported ones -> will open a ticket about that, but that's not going to get resolved for this need (esp. in stable branches)
-> #46332
Add size parameter to create_city()function. As a side effect, allow creation of cities bigger than maximal size an empty city may grow to (a notice is left). As another related fix, check city_size sanity for all ruleset units.