Feature #1677: allow terrain to have > 1 animal - Freeciv - Freeciv Ticket System

Actions

Copy link

Feature #1677

closed

allow terrain to have > 1 animal

Added by Dean Brown 3 months ago. Updated 10 days ago.

Status:

Closed

Priority:

Normal

Assignee:

Marko Lindqvist

Category:

General

Target version:

3.3.0-d3f

Start date:

09/08/2025

Due date:

% Done:

Estimated time:

Description

In terrain.ruleset, animal="..." currently only supports 1 animal. I would like to use a comma separated list.

Files

Download all files

1677.patch (48.9 KB) 1677.patch		Dean Brown, 11/17/2025 03:02 AM
1677Main.patch (48.1 KB) 1677Main.patch		Dean Brown, 11/17/2025 03:02 AM

Related issues 2 (2 open — 0 closed)

Actions

Copy link

Updated by Dean Brown 2 months ago

File 1677.patch added
File 1677Main.patch added

Causes a change to network packets, so expecting target 3.3.0.

I wanted to change the "animal = ..." to "animals = ..." in the terrain.ruleset files, but that breaks freeciv-ruleup. In file
server/ruleset/ruleload.c, routine load_ruleset_terrain() would have to try to lookup both "%s.animal" and "%s.animals" to make freeciv-ruleup work.

I can update the wiki page [[http://www.freeciv.org/wiki/How_to_update_a_ruleset_from_3.2_to_3.3]] when this is done.

Actions

Copy link

Updated by Marko Lindqvist 2 months ago

Category changed from Rulesets to General
Target version set to 3.4.0-d3f

- Since this is not going to S3_2 or older, use nullptr instead of NULL
- As you said: "animal" needs to be changed to "animals"
- Style: Empty line between variable declarations and first line of code within the block
- Style: Some broken indentation
- It would be better to store num_animals in the terrain structure (just a few integers more memory taken) than to burn CPU to figure out the count every time it's needed
- In the client side terrain.animals must be initially initialized to nullptr, and in the server side it wouldn't hurt (so can be done in common code). Otherwise we may have non-nullptr animals, when in fact there's no animals
- 'FALSE' and 'TRUE', not 'false' and 'true' (Yeah, that's something we probably should change in coming versions, but I'd rather concentrate NULL ~~> nullptr migration for now, instead of starting another migration already)~~
Style: "if (! ok) {" -> "if (!ok) {" (no space after '!')

Actions

Copy link

Updated by Dean Brown 2 months ago

File deleted (~~1677.patch~~)

Actions

Copy link

Updated by Dean Brown 2 months ago

File deleted (~~1677Main.patch~~)

Actions

Copy link

Updated by Dean Brown 2 months ago

File 1677.patch added
File 1677Main.patch added

Thanks for the feedback, here's updated better patches. I figured out how to use rscompat to avoid breaking freeciv-ruleup.

I originally made the terrain->animals to be null-terminated like the terrain->resources, but I agree it's better to also have
terrain->num_animals. It might also be better if terrain->resources were to be re-done similarly, but that's out of scope for this issue.

Actions

Copy link

Updated by Marko Lindqvist about 2 months ago

Target version changed from 3.4.0-d3f to 3.3.0-d3f

Actions

Copy link

Updated by Marko Lindqvist about 2 months ago

Blocks Tasks #1631: S3_3 datafileformat freeze (d3f) added

Actions

Copy link

Updated by Marko Lindqvist about 1 month ago

I reviewed S3_3 patch (one with the rscompat part). Presumably most comments apply to main patch too

- Style: Empty line between variable declarations and first line of code within the block (still missing in some places)
- In case of "None" rscompat animal, the animals is set to nullptr without freeing the memory that it previously pointed to
- There is no need to support special value "None" for animals, as empty array gives the same result. (Old 'animal' needed it as 'animal' was a mandatory field to be always present)
- In rulesets, addition of 's' push rest of the line one character off. Remove one space to compensate.
- Several "} terrain_animals_iterate_end" with extra space (two spaces) between '}' and 'terrain_animals_iterate_end'

Actions

Copy link

Updated by Dean Brown about 1 month ago

File deleted (~~1677Main.patch~~)

Actions

Copy link

#10

Updated by Dean Brown about 1 month ago

File deleted (~~1677.patch~~)

Actions

Copy link

#11

Updated by Dean Brown about 1 month ago

File 1677.patch added
File 1677Main.patch added

Yes both patches needed fixing.

Going forward, animals = "None" will generate a ruleset load error. Which is OK - that's how "Resources = ..." works.

Actions

Copy link

#12

Updated by Marko Lindqvist 26 days ago

Status changed from New to In Review
Assignee set to Marko Lindqvist

Actions

Copy link

#13

Updated by Marko Lindqvist 22 days ago

Starting tutorial scenario, I get
1: Trying to put 539309362 into 16 bits; it will result 13618 at receiving side.

Debugging that with the help of FIELD_RANGE_ASSERT of dataio_raw.c, I found out that it comes from the packets_gen.c line
e |= DIO_PUT(sint16, &dout, &field_addr, real_packet->animals[i]);

Actions

Copy link

#14

Updated by Marko Lindqvist 22 days ago

I guess the issue is in send_ruleset_terrain():

terrain_animals_iterate(pterrain, panimal) {
      packet.animals[packet.num_animals] = utype_number(panimal);
    } terrain_animals_iterate_end;

On every round of the iteration it places the animal to the same index (packet.num_animals), actually outside the whole legal range (0 ... num_animals-1). All the actually used indices are left with garbage values.

Actions

Copy link

#15