Skip to content

Opinionated Cleanup#65

Draft
simplyWiri wants to merge 6 commits intoMehni:masterfrom
simplyWiri:cleanup
Draft

Opinionated Cleanup#65
simplyWiri wants to merge 6 commits intoMehni:masterfrom
simplyWiri:cleanup

Conversation

@simplyWiri
Copy link
Contributor

@simplyWiri simplyWiri commented Aug 26, 2025

This is a series of changes ontop of my base changes - which are a bit more opinionated and try to make the code simpler & easier to reason about (for newcomers).

//maintain cell reservations on the trip back
//TODO: do that when we carry things
//I guess that means TODO: implement carrying the rest of the items in this job instead of falling back on HaulToStorageJob
yield return TargetB.HasThing ? Toils_Goto.GotoThing(TargetIndex.B, PathEndMode.ClosestTouch)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I completely deleted this code, because it didn't make much sense to me. TargetB is not where anything is guaranteed to be dropped off at - the Unload job driver owns that responsibility - maintaining reservations seems moot, if that was a desired feature - wouldn't you do this in the Unload JobDriver?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this was load-bearing, I've noticed a few edge cases where pawns are over-hauling when there is not enough space.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mm. I think this is a more general issue:

When we use a storage building, we store a (StoreTarget, CellAllocation) pair. Where CellAllocation is presumed to be for a particular item. This means we store a int capacity. This capacity is only correct for a particular item, not any item.

This means, for example - if we haul leather, to a cell with 225 capacity (3x free cells) - and we only have, i.e. 10 leather. We would then see 215 free slots.

If we then went to haul a bunch of single-slot items, i.e. apparel. We could attempt to haul ~215 individual apparel items into this slot - when we should only be able to store 2. Because fundamentally we are storing things by cell.

@simplyWiri
Copy link
Contributor Author

simplyWiri commented Aug 27, 2025

I feel like JobOnThing can be refactored into something which looks like this:

public Job JobOnThing(Pawn pawn, Thing thing, bool forced = false)
{
    if (InvalidHaulTarget(pawn, thing, forced))
    {
        return null; // Return a default job in this case
    }

    var job = ...;
    var haulables = new List<Thing>(ThingsPotentiallyNeedingHauling()).SortedBy(pred: DistanceTo(thing.Cell));
    Dict<StorageLocation, StoredThings> storageLocations; // Map of existing targets for hauling things to

    // Checks encumberance & pawn inventory weight - truncates the `stackCount` of `thing` if it won't fit in
    // the pawns inventory.
    while ((thing = CouldHaul(pawn, thing) != null)
    {
        var haulDestination = FindHaulDestination(pawn, thing, storageLocations);
        if (haulDestination is not null)
        {
            // Does a few things:
            //  1. Adds to `job.TargetQueueA, job.TargetQueueB, job.targetCount`
            //  2. Tracks the added encumberance to the pawn
            //  3. Adds tracking information to `storageLocations`
            // if the destination can't handle all of `thing` - the un-hauled remainder is set - otherwise it is null
            thing = HaulToDestination(pawn, thing, haulDestination, storageLocations);
        }

        if (thing is null) {
            thing = PopNextClosest(pawn, haulables);
        }
    }

    return job;
}

Some notes:

  • Minor improvement could be re-sorting haulables for each consecutive thing
  • Maybe better structured data for pawn encumberance to avoid some Stat calls for mass. Because its mostly memoized I'm not sure its a huge concern - but its a point of improvemnet
  • Types are vague in this sketch, some things should be a ThingCount vs a Thing.

However - a few reservations:

  • I don't understand bits like:
				if (HaulToHopperJob(thing, targetCell, map))
				{
					return HaulAIUtility.HaulToStorageJob(pawn, thing, forced);
				}

given this is inconsistently applied when we first look for a thing to haul - but not the consecutive things we pick up (is this mixed requirements for the first hauled item, vs the next ones we gather?).
InconsistentHauling

@simplyWiri
Copy link
Contributor Author

Fake news on the performance - it also broke things :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant