Incidencia #44703

Optimize create_city()

Abrir Fecha: 2022-05-29 06:25 Última actualización: 2024-02-09 15:42

Informador:
Propietario:
Estado:
Open [Owner assigned]
Componente:
Hito:
Prioridad:
5 - Medium
Gravedad:
5 - Medium
Resolución:
Ninguno
Fichero:
3

Details

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.

Ticket History (3/24 Histories)

2022-05-29 06:25 Updated by: ihnatus
  • New Ticket "Optimize create_city()" created
2022-05-29 06:30 Updated by: ihnatus
  • File 3.0-optimize-create-city.patch (File ID: 9365) is attached
2022-05-29 08:12 Updated by: ihnatus
  • File 3.0-optimize-create-city.patch (File ID: 9365) is deleted
2022-05-29 08:22 Updated by: ihnatus
Comentario

Likely, 3.1 patch suites also for master.

2022-05-29 09:21 Updated by: cazfi
Comentario

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)

2022-05-30 02:33 Updated by: ihnatus
Comentario

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.

(Edited, 2022-05-30 02:36 Updated by: ihnatus)
2022-05-30 03:36 Updated by: cazfi
Comentario

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.

(Edited, 2022-05-30 03:37 Updated by: cazfi)
2022-05-30 07:49 Updated by: ihnatus
Comentario

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?

2022-06-11 21:52 Updated by: cazfi
2022-07-06 08:36 Updated by: cazfi
Comentario

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.

2022-08-05 08:58 Updated by: cazfi
2022-10-07 09:27 Updated by: cazfi
2022-12-06 21:49 Updated by: cazfi
2022-12-07 02:35 Updated by: cazfi
Comentario

So, this is currently blocker for #44312

2022-12-23 05:08 Updated by: cazfi
Comentario

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

2023-02-02 06:30 Updated by: cazfi
2023-04-04 03:07 Updated by: cazfi
2023-06-12 10:09 Updated by: cazfi
Comentario

Reply To ihnatus

One of the goals was removing the step by step growth procedure...

See also #43379

2023-06-25 19:41 Updated by: cazfi
2023-11-10 15:10 Updated by: cazfi
2024-02-09 15:42 Updated by: cazfi

Attachment File List

Editar

Please login to add comment to this ticket » Entrar