exportlegends: Historical event type: `masterpiece_lost` `histfig` incorrect

ralpha commented 4 years ago

While testing a world for DF Storyteller I found this event:

<historical_event>
        <id>113917</id>
        <type>masterpiece_lost</type>
        <creation_event>108560</creation_event>
        <histfig>1953722220</histfig>
        <site>395</site>
        <method>melt</method>
</historical_event>

The histfig in this case has ID 1953722220 the problem is the world only has 11733 historical figure. So this historical figure does not exist. I don't know how this happened. It might have something to do with <type>masterpiece_lost</type> as most histfig for this type are set to 0. You can find the export here: https://gitlab.com/df_storyteller/df-storyteller/uploads/e09bde70c0e78847dde25e8753b5d744/legends-region4_4th_embark-00162-08-26.zip I don't have the world save, but here it the related issue https://gitlab.com/df_storyteller/df-storyteller/-/issues/86

lethosor commented 4 years ago

That looks like uninitialized memory to me. Thanks for the report! Any idea if creation_event is correct?

ralpha commented 4 years ago

I tested 2 creation_event tags and both are referring to masterpiece_created_item type events. So I think they are correct.

0xa08ab242 commented 4 years ago

FYI - I'm not sure if it helps, but here is the world save region4_4th_embark.zip

lethosor commented 4 years ago

The save definitely does help, thanks! The first world I tried locally had no events of this type.

The corresponding creation event in this case (108560) does exist, and corresponds to the same site, but its item_id is garbage. I don't know how to track it down to figure out if there's something weird about the item.

[DFHack]# lua ~world.history.events[108560]
<history_event_masterpiece_created_itemst: 0x7fffb36df2e0>
year                     = 157
seconds                  = 396275
flags                    = <BitArray<>: 0x7fffb36df2f0>
id                       = 108560
maker                    = 11527
maker_entity             = 1582
site                     = 395
item_type                = 16
item_subtype             = 0
mat_type                 = 57
mat_index                = -1
item_id                  = 11338150

[DFHack]# lua ~world.history.events[113917]
<history_event_masterpiece_lostst: 0x7fffb3778570>
year                     = 158
seconds                  = 334418
flags                    = <BitArray<>: 0x7fffb3778580>
id                       = 113917
creation_event           = 108560
histfig                  = 1953722220
site                     = 395
method                   = 0

I'm tempted to say that this is something wrong with this specific world, possibly minor corruption of some kind, because I haven't seen any other abnormal IDs. However, 0 being so common makes me think that histfig might not be an ID after all, let alone a histfig ID, because those almost always use -1 to indicate N/A (and 0 is a valid histfig ID!). For reference, here are all of the masterpiece_lost events in this world:

id = 71949   histfig = 0   <historical_figure: 0x7fff9d70f190>
id = 71950   histfig = 0   <historical_figure: 0x7fff9d70f190>
id = 71951   histfig = 0   <historical_figure: 0x7fff9d70f190>
id = 71952   histfig = 0   <historical_figure: 0x7fff9d70f190>
id = 71953   histfig = 0   <historical_figure: 0x7fff9d70f190>
id = 71954   histfig = 0   <historical_figure: 0x7fff9d70f190>
id = 71955   histfig = 4633    <historical_figure: 0x7fff9f617d90>
id = 100399  histfig = 0   <historical_figure: 0x7fff9d70f190>
id = 113916  histfig = 0   <historical_figure: 0x7fff9d70f190>
id = 113917  histfig = 1953722220  nil
id = 121166  histfig = 0   <historical_figure: 0x7fff9d70f190>

quietust commented 4 years ago

I've just checked against a disassembly, and history_event_masterpiece_lostst.histfig is definitely supposed to be a historical figure ID - the code for describing the event in Legends mode uses it as such, it checks for -1 to choose how to format the message ("X destroyed Y due to Z" versus "Y was destroyed due to Z"), and it's even initialized for events describing art defacement by miners, masons, and engravers.

However, the field never actually gets initialized to -1 when it's unused, so this is definitely a bug in DF itself - in fact, it's already been logged in the DF bug tracker as #0009727 (where I've just added a comment with these findings).

lethosor commented 4 years ago

There are a couple things we could do on the DFHack side, then:

Remove <histfig> entirely (might break legends tools)
Document that <histfig> is likely incorrect, but leave it
Correct <histfig> to -1 if it's not a valid histfig ID (this won't handle cases where it happens to be a valid histfig ID)
- Optional: assume 0 is also invalid and change it to -1
- More complicated: check whether the histfig was alive at the time of the event

@Kromtec @robertjanetzko do you have a preference?

robertjanetzko commented 4 years ago

removing would be ok for the legendsviewer. otherwise -1, as this is also the default value I use. (i will have a closer look next week, when I'm back from vacation)

Kromtec commented 4 years ago

Removing it would not break anything for LegendsViewer. -1 for invalid values would be also ok and I think there is no need for the complicated alive check.

lethosor commented 4 years ago

Thanks for letting me know! I think we should probably just remove it, then (maybe comment it out to future-proof it a bit)

DFHack / dfhack

exportlegends: Historical event type: `masterpiece_lost` `histfig` incorrect #1629