47

[PatrikLundell]

Additional research on army controllers, including some field naming/identification.

Edit: fix #403
Edit2: Fix DFHack/dfhack#1578

[PatrikLundell]

I'm not going to try to deal with the remaining two schema failures as those are things I haven't changed and I don't know enough to determine what the correct correction should be.

[BenLubar]

those were my mistake - you can merge or rebase to fix them

[PatrikLundell]

It seems the introduction of a bitfield-type (in history.xml) is a first, but it's legal according to the XSD and did compile (as well as display the expected data).
It's a slight guess that the bitfield type is a bitfield rather than an enum, as I haven't seen value 3, i.e. scout mercs employed by both sides at the same time, but usage of scouts is not the norm.

[PatrikLundell]

A couple of comments:

• gui/gm-editor didn't seem to like to display rhythm_pattern.bars [X].beat [0] as an enum value, possibly because of the "is_array" attribute.
• Given that rhythm_pattern string pointers are arrays (I tried to set them as static arrays with the largest count seen, and that caused gui/gm-editor to run out of memory, while an array allowed me to view the 3 elements of both arrays expected to be present and verify that their contents matched what the exported XML indicated they should have), are we sure the other art related structures use static arrays with uninitialized contents for unused elements, or could it be that all of them actually use the cumbersome parameter dependent array size? I think this is @BenLubar 's area of expertise.

[lethosor]

Might be good to move the version into a since attribute:

Suggested change

	<compound name='picked_growths' comment="also includes 'automatically picked' i.e. fallen fruit that becomes item_spatter">
	<compound name='picked_growths' since='v0.40.14' comment="also includes 'automatically picked' i.e. fallen fruit that becomes item_spatter">

[lethosor]

Does it match either of the coordinates on the right edge of the screen?

[PatrikLundell]

No, it didn't match any of those.

Edit: Correction it does indeed seem to match the lower value, which looks like the Elevation of the tile (not sure why I did see a different value there previously). I don't know how to translate from Elevation to whatever format is used elsewhere, though, but regardless, the comment should be updated.

[quietust]

Perhaps this allows for partial harvests of extremely dense growths, so you can pick the rest of it later.

[PatrikLundell]

Possibly, although I've seen only 1, possibly 2, and 1000 here. If your idea is correct we'd briefly see something in between 2 and 1000 for trees, but that would require a bit of luck (or care) with the timing.
I haven't seen anything indicating a gradual picking graphically, but the "graphics" might not show the full picture, and I haven't studied fruit picking for hours...

Edit: I've run a callback script that checks these fields every frame, and it hasn't registered a single case of a density value in the range 3 - 999, which ought to happen while dorfs are vigorously picking fruit (at least the durians and olives had a raw density value of 1000).

[quietust]

It's possible that it only happens when gathering items in Adventurer mode - I'd try it myself, but I don't have a script handy for finding the correct object_data entry for my current location.

[PatrikLundell]

  local embark_x = df.global.world.world_data.region_details [0].pos.x
  local embark_y = df.global.world.world_data.region_details [0].pos.y
  local embark_min_x, embark_max_x, embark_min_y, embark_max_y
  local object_id
  
  for i = 0, 15 do
    for k = 0, 15 do
      if df.global.world.world_data.region_details [0].biome [i] [k] >= 48 then
        if not embark_min_x then
          embark_min_x = i
        end
        
        embark_max_x = i
        
        if not embark_min_y then
          embark_min_y = k
        end
        
        embark_max_y = k

        object_id = i + embark_x * 16 + k * 16 * df.global.world.world_data.world_width + embark_y * 256 * df.global.world.world_data.world_width
        dfhack.println (df.world_object_data.find (object_id), object_id)        
      end
    end
  end

This snippet gets the object ids for an embark. It obviously would have to be adapted to adventurer mode, where I guess the region_details [0] part remains the same, but there'd have to be some other logic to find the current MLT. I'm rather unfamiliar with adventurer mode, though.

[quietust]

In Adventurer mode, region_details[4] is the one that contained >=48 biome values (possibly because it corresponded to the town I started in), but none of the calculated object_id values corresponded to existing world_object_data records.

I tried a more brute-force approach and simply searched through all records to find ones with non-empty picked_growths lists, and it looks like Adventurer mode just doesn't use them at all - no matter how many finger limes I picked from a nearby tree, all of those lists remained empty.

It's possible Toady added the ability to handle partial harvests but just never finished implementing it - perhaps in a later version, dwarves won't harvest the entire tile at once.

Did you only monitor those values while dwarves were harvesting, or did you also check while growths were falling off and landing on the ground as spatters?

[PatrikLundell]

Brute force is what I've used mostly, until trying a callback every frame, where the frame rate dropped to 28 even when accessing only the appropriate entries. It's odd, however, that different structures would be used for the same purpose in Adventurer mode and Fortress mode, as I assume the trees actually were picked visually (and not possible to pick indefinitely).

A fruit tree density of 1000 looks like some kind of max value rather than a sensible one, when shrubs tend to have a value of 1, so it might be that partial picking was waiting for the agriculture overhaul.

Have you used tools to look at what code Adventurer mode picking uses? It might be something different that uses different data structures.

[quietust]

(disregard the above force-push - I tried using the "Resolve Conflicts" button below and introduced a typo, so I reverted it)

[lethosor]

Ok, I think I've gotten the merge conflicts sorted out. (For some reason, git had the most trouble with your minor changes to historical_figure_info.relationships.intrigues - adding Corruptly_Punish_Actor and naming a couple unk fields - probably because it moved to historical_figure_relationships.intrigues. I ended up just copying the relevant section from master and applying those couple changes manually, instead of trying to sort through git's output, so I guess that's a strategy that could be helpful sometimes.)

To be safe, I'm going to double-check the diff between this branch and master before I merge, so it might not be merged until tomorrow. The good news is that no structures changed sizes, so the merge probably didn't break anything! (A couple names of less-commonly-used structures were changed, presumably intentionally: army_controller_villain_visiting -> army_controller_villainous_visit, army_controller_invasion -> army_controller_invasion_order.) check-structures-sanity also passed on world in a small world, so that's another good sign that nothing broke.

Overall, this is some great research. Again, sorry that it took so long to get to. Mostly, I think I looked at this a few times, thought "that's a big PR, I'll look at it later when I have more time", and got wrapped up in other tasks. It definitely would have been easier to deal with if I had merged it sooner, and I'll try to be on top of that next time.

It might help to know for sure if you're ready for a PR to be merged. I think your "0.4x" PRs in general have been a steady stream of in-progress work that could be merged at any time - am I correct in assuming that I can merge an open PR that you created whenever I want? I'd hate to merge something and have you push an additional change minutes later (it wouldn't be too hard to just open a new PR for it, but it could be noisy). A suggestion: if you want to indicate that you're still working on a PR, you could set it to "draft" mode, and I'll assume it's ready to be merged otherwise. GitHub has a help page here about draft PRs. (I should probably add something similar to the contributing guide...) - edit: how does the pull requests section of https://docs.dfhack.org/en/latest/docs/Contributing.html look?

I left a couple comments related to things I noticed while fixing conflicts. Some might not be relevant anymore, but hopefully some are useful.

[lethosor]

unk_4 to unk_9 are actually a vector (identities), according to 81e269f#diff-35ab45624fa78d4a8c071f3c0fee5d76R482
I am curious about the comments, though - usually, all three pointers making up a vector will point to valid memory or be set to 0. In my largest save, I couldn't find any cases where these vectors were populated or where any pointers were non-0, so I can't say for sure.

[PatrikLundell]

The 47 branch was used for a steady stream of small unrelated changes, intended to be possible to merge at any time (the only "outstanding" cases are the ones where a conflict is detected when the updates are pushed, and they're handled as quickly as possible after detection. What I'd hoped for, and what happened early on, was that it would be merged with reasonably short intervals so there wouldn't be such a mountain building up (getting increasingly burdensome as parallel changes clash with it). I'm certainly aware of the "this is too much effort to tackle now, I need a bigger chunk of time to do that" issue.
It's certainly possible that you'd make a merge and a new change trickles in minutes later, but the new change should be (mostly) unrelated to the previous ones, so it's not a big deal if it ends up waiting until the next merge opportunity.

So far I've kept my drafts local, so I haven't had any need to flag them (I've seen others using that, though, and it's useful when you want/need comments).

It's good that the fields listed have been identified as a vector, which means my comments are no longer relevant. The comments state what I saw (as pointers look like changing data), but I don't remember things in such details as to be able to comment on the comments beyond that.

[lethosor]

Got it. I'll do my best to keep up with them faster.
I forgot to go into more detail earlier: devel/visualize-structures (and the memview plugin, if you build with dev plugins enabled) marks valid pointers, so that could be helpful to tell whether random-looking data is actually a pointer (since vectors consist of three consecutive pointers). The vectors plugin will do some additional checks to find valid vectors. None of these will deal with empty vectors with 3 null pointers, though.

[lethosor]

I actually just ran into some of these fields populated in a pocket-sized world. Turns out they're not vectors after all, so you were right. (They're not even valid pointers.) I'll spin up a 32-bit instance to investigate whether they're intptr_ts or int32_ts.

There's a more complete list of memory-research tools here, in case it's useful: https://docs.dfhack.org/en/latest/docs/Memory-research.html

[PatrikLundell]

Thanks for digging deeper into this.

[lethosor]

Turns out this is a unit_thought_type:

df-structures/df.history.xml

Lines 1525 to 1526 in ecd6bcc

	<struct-type type-name='history_event_circumstance_info'>
	<enum name='type' type-name='unit_thought_type' base-type='int32_t'/>

257 is not currently known, so that could be interesting to investigate.

[lethosor]

This turned out to be a duplicate of the existing vague_relationship_type (although it only went up to 16 items at the time of your PR)

[lethosor]

init-value=0 is technically default for int fields, so these aren't strictly necessary.

I double-checked - here's the generated constructor for intrigue_corruption from my last build on develop (which lacks the init-value attrs; the relevant fields are the last two, although they have different names):

  df::intrigue_corruption::intrigue_corruption()
    :  crime(ENUM_FIRST_ITEM(crime_type)), corruptor_id(-1), target_id(-1), target_relationship(ENUM_FIRST_ITEM(vague_relationship_type)), target_relationship_entity_id(-1), lurer_id(-1), manipulation_type(Threat), anon_1(0), anon_2(0), manipulated_facet(ENUM_FIRST_ITEM(personality_facet_type)), anon_3(0), facet_roll(0), manipulated_value(ENUM_FIRST_ITEM(value_type)), anon_4(0), value_roll(0), manipulated_emotion(Trust), anon_5(0), emotion_roll(0), position_entity_id(-1), position_assignment_id(-1), offered_id(-1), offered_relationship(ENUM_FIRST_ITEM(vague_relationship_type)), corruptor_ally_roll(0), target_ally_roll(0)
  {
  }

[lethosor]

Your guess appears to have been correct, since this is also vague_relation_type (same enum) according to BenLubar's research