Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix comment for job->item_subtype #435

Merged
merged 5 commits into from
Aug 24, 2021
Merged

Conversation

myk002
Copy link
Member

@myk002 myk002 commented Aug 22, 2021

While implementing the --hauler-type param for the prioritize script, I noticed that the job->item_subtype field, which is tagged as being a hauler_type enum, did not match up with the type of item being hauled. I fixed this enum based on observation of which enum values resulted in which types of items were hauled.

I was unabled to verify the "Bin" enum. I could not get any hauling jobs of this type to be generated in my fort, even when explicitly building bins and enabling bins for stockpiles.

myk002 added 2 commits August 22, 2021 14:40
while implementing the --hauler-type param for the prioritize script, I
noticed that the job->item_subtype field, which is tagged as being a
hauler_type enum, did not match up with the type of item being hauled. I
fixed this enum based on observation of which enum values resulted in
which types of items were hauled.

I was unabled to verify the "Bin" enum. I could not get any hauling jobs
of this type to be generated in my fort, even when explicitly building
bins and enabling bins for stockpiles.
@lethosor
Copy link
Member

lethosor commented Aug 22, 2021

This is an interesting one.

  • The only place (supposedly) where hauler_type is used as a value is here (edit: this is what you identified in your PR description above). This is used by labormanager here as a unit_labor, and your change would cause unit_labors 1-8 to line up with hauler_types 1-8. Bin (9 with your change) does not correspond to a HAUL_ unit_labor (where 9 = CLEAN).
  • hauler_type is also used as an index-enum in stockpile, but @ab9rf reports that this enum was inaccurate at the time labormanager was written, so indices were hardcoded here. Note that "bin" corresponded to 4 at the time.

My guess is that Toady might have changed hauler_type at some point to be consistent with unit_labor, but I don't know for sure, or when.

Copy link
Member

@ab9rf ab9rf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would break the indexing of world.stockpiles.num_jobs and world.stockpiles.num_haulers. The current enum is correct; I tested this extensively over the course of years developing labormanager, and just spot-checked it in a mature fort.

hauler_type is also used as an index-enum in stockpile, but @ab9rf reports that this enum was inaccurate at the time labormanager was written, so indices were hardcoded here. Note that "bin" corresponded to 4 at the time.

No, the existing enum is correct. I originally thought it was wrong, but it was more because I didn't understand how it was being used when I initially wrote that, and haven't gone back to update the code to reflect lessons learned since.

There is no direct correspondence between hauling type and labors. The labor for a hauling job is determined in a fairly complicated way: see labormanager job mapping source for more info.

df.stockpile.xml Outdated
@@ -3,13 +3,13 @@
<enum-item name='Any'/>
<enum-item name='Stone'/>
<enum-item name='Wood'/>
<enum-item name='Item'/>
<enum-item name='Bin'/>
<enum-item name='Body'/>
<enum-item name='Food'/>
Copy link
Member

@ab9rf ab9rf Aug 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong, at least with respect to world.stockpiles.num_haulers, and I must recommend against this PR without more evidence.

I just tested on a mature fort by clearing all hauling labors on everyone. This resulted in world.stockpiles.num_haulers going to all zero. Then, reenabling food hauling on everyone resulted in world.stockpiles.num_haulers[Food] going to 194 while the others remained zero.

If the enum were wrong, some other field would have gone up. Therefore, the existing enum is correct, at least for food, but this change would change the value of the Food enum.

I must therefore object.

@lethosor
Copy link
Member

Thinking over it a bit, I suspect the comment for job.item_subtype is wrong, and this field is actually a unit_labor when type == StoreItemInStockpile. I'm not sure yet what implications this would have for DFHack/scripts#327, though.

@myk002
Copy link
Member Author

myk002 commented Aug 23, 2021

ack. I'll fix the comment and update DFHack/scripts#327 to interpret the field as a unit_labor. Thanks!

@myk002 myk002 changed the title Fix enum values for hauler_type Fix comment for job->item_subtype Aug 23, 2021
@myk002
Copy link
Member Author

myk002 commented Aug 23, 2021

changes reverted; misleading comment made less misleading

@ab9rf
Copy link
Member

ab9rf commented Aug 23, 2021

My research suggests that we should rename Bin to something else. Specifically, StoreItemInHospital jobs increment world.stockpile.num_jobs[Bin], even though a "store in hospital" job never result in anything being actually stored in a bin.

        pjVar38 = job::create();
        piVar42 = local_440;
        pjVar38->job_type = StoreItemInHospital;
...
        world.stockpile.num_jobs.Any = world.stockpile.num_jobs.Any + 1;
        world.stockpile.num_jobs.Bin = world.stockpile.num_jobs.Bin + 1;

@lethosor lethosor merged commit f2cf720 into DFHack:master Aug 24, 2021
@myk002 myk002 deleted the myk_hauler_type branch August 24, 2021 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants