From bb55a6d9ce45543d008aa957de44c86079cf59d7 Mon Sep 17 00:00:00 2001 From: Ruben Kluft Date: Tue, 21 Jul 2026 11:11:12 +0200 Subject: [PATCH 1/2] Fix weight redistributor sometimes dropping items --- CHANGELOG.md | 4 +++- src/WeightRedistributor.php | 21 +++++++++++++++++--- tests/WeightRedistributorTest.php | 32 +++++++++++++++++++++++++++++++ 3 files changed, 53 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2c70320f..2c957ad1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,7 +1,9 @@ # Changelog ## [4.x - Unreleased] - 2026-xx-xx - +### Fixed +- In some rare cases `WeightRedistributor` would silently drop items + ## [4.2.0] - 2026-07-05 ### Added - A mechanism to allow forcing certain items to be packed together [RubenKluft] diff --git a/src/WeightRedistributor.php b/src/WeightRedistributor.php index d5f7c3c0..1e6cef8a 100644 --- a/src/WeightRedistributor.php +++ b/src/WeightRedistributor.php @@ -117,8 +117,9 @@ private function equaliseWeight(PackedBox &$boxA, PackedBox &$boxB, float $targe continue; // moving this item would harm more than help } - $newLighterBoxes = $this->doVolumeRepack(array_merge($underWeightBoxItems, [$overWeightItem]), $underWeightBox->box); - if ($newLighterBoxes->count() !== 1) { + $packableItems = array_merge($underWeightBoxItems, [$overWeightItem]); + $newLighterBoxes = $this->doVolumeRepack($packableItems, $underWeightBox->box); + if ($this->isEverythingPacked($newLighterBoxes, $packableItems) === false) { continue; // only want to move this item if it still fits in a single box } @@ -135,7 +136,7 @@ private function equaliseWeight(PackedBox &$boxA, PackedBox &$boxB, float $targe unset($overWeightBoxItems[$key]); $newHeavierBoxes = $this->doVolumeRepack($overWeightBoxItems, $overWeightBox->box); - if (count($newHeavierBoxes) !== 1) { + if ($this->isEverythingPacked($newHeavierBoxes, $overWeightBoxItems) === false) { assert(true, 'Could not pack n-1 items into box, even though n were previously in it'); continue; } @@ -153,6 +154,20 @@ private function equaliseWeight(PackedBox &$boxA, PackedBox &$boxB, float $targe return $anyIterationSuccessful; } + private function isEverythingPacked(PackedBoxList $result, array $expectedItems): bool + { + if ($result->count() !== 1) { + return false; + } + + $packedItems = []; + foreach ($result->top()->items as $packedItem) { + $packedItems[] = $packedItem->item; + } + + return count($packedItems) === count($expectedItems); + } + /** * Do a volume repack of a set of items. * @param iterable $items diff --git a/tests/WeightRedistributorTest.php b/tests/WeightRedistributorTest.php index 1f8438ae..6570a4f2 100644 --- a/tests/WeightRedistributorTest.php +++ b/tests/WeightRedistributorTest.php @@ -10,6 +10,7 @@ namespace DVDoug\BoxPacker; use DVDoug\BoxPacker\Test\ConstrainedPlacementNoStackingTestItem; +use DVDoug\BoxPacker\Test\LimitedSupplyTestBox; use DVDoug\BoxPacker\Test\TestBox; use DVDoug\BoxPacker\Test\TestItem; use PHPUnit\Framework\Attributes\CoversClass; @@ -67,4 +68,35 @@ public function testWeightDistributionWorks(): void self::assertEquals(0, $packedBoxes->getWeightVariance()); } + + /** + * Test to ensure no items are silently dropped during weight redistribution. + */ + public function testWeightRedistributionDoesNotSilentlyDropItems(): void + { + $packer = new Packer(); + $packer->addBox(new LimitedSupplyTestBox('Box', 29, 29, 29, 0, 29, 29, 29, 68, 2)); + + $packer->addItem(new TestItem('Item 0', 10, 10, 10, 2, Rotation::BestFit), 2); + $packer->addItem(new TestItem('Item 1', 10, 10, 10, 3, Rotation::BestFit)); + $packer->addItem(new TestItem('Item 2', 10, 10, 10, 4, Rotation::BestFit)); + $packer->addItem(new TestItem('Item 3', 10, 10, 10, 8, Rotation::BestFit), 5); + $packer->addItem(new TestItem('Item 4', 10, 10, 10, 18, Rotation::BestFit), 3); + + // packer initially packs 6 items into each box, + // the imbalance in weights will be attempted to be corrected by the WeightRedistributor + $packedBoxes = $packer->pack(); + self::assertCount(2, $packedBoxes); + + $packedItemCount = 0; + foreach ($packedBoxes as $packedBox) { + $packedItemCount += $packedBox->items->count(); + } + + self::assertSame( + 12, + $packedItemCount + $packer->getUnpackedItems()->count(), + 'No items should be lost during weight redistribution' + ); + } } From 8cdbd804d3b8712cf662618732609b9eabf4fb31 Mon Sep 17 00:00:00 2001 From: Doug Wright Date: Mon, 3 Aug 2026 16:43:11 +0100 Subject: [PATCH 2/2] Ensure weight redistributor repacks keep all items Alternate approach that avoids looping over packed items: require zero packer leftovers after doVolumeRepack so a single-box partial result is not treated as a successful move. --- src/WeightRedistributor.php | 28 +++++++++------------------- 1 file changed, 9 insertions(+), 19 deletions(-) diff --git a/src/WeightRedistributor.php b/src/WeightRedistributor.php index 1e6cef8a..5ad60e79 100644 --- a/src/WeightRedistributor.php +++ b/src/WeightRedistributor.php @@ -117,9 +117,8 @@ private function equaliseWeight(PackedBox &$boxA, PackedBox &$boxB, float $targe continue; // moving this item would harm more than help } - $packableItems = array_merge($underWeightBoxItems, [$overWeightItem]); - $newLighterBoxes = $this->doVolumeRepack($packableItems, $underWeightBox->box); - if ($this->isEverythingPacked($newLighterBoxes, $packableItems) === false) { + $newLighterBoxes = $this->doVolumeRepack(array_merge($underWeightBoxItems, [$overWeightItem]), $underWeightBox->box); + if ($newLighterBoxes->count() !== 1) { continue; // only want to move this item if it still fits in a single box } @@ -136,7 +135,7 @@ private function equaliseWeight(PackedBox &$boxA, PackedBox &$boxB, float $targe unset($overWeightBoxItems[$key]); $newHeavierBoxes = $this->doVolumeRepack($overWeightBoxItems, $overWeightBox->box); - if ($this->isEverythingPacked($newHeavierBoxes, $overWeightBoxItems) === false) { + if (count($newHeavierBoxes) !== 1) { assert(true, 'Could not pack n-1 items into box, even though n were previously in it'); continue; } @@ -154,20 +153,6 @@ private function equaliseWeight(PackedBox &$boxA, PackedBox &$boxB, float $targe return $anyIterationSuccessful; } - private function isEverythingPacked(PackedBoxList $result, array $expectedItems): bool - { - if ($result->count() !== 1) { - return false; - } - - $packedItems = []; - foreach ($result->top()->items as $packedItem) { - $packedItems[] = $packedItem->item; - } - - return count($packedItems) === count($expectedItems); - } - /** * Do a volume repack of a set of items. * @param iterable $items @@ -184,7 +169,12 @@ private function doVolumeRepack(iterable $items, Box $currentBox): PackedBoxList $packer->setBoxQuantity($currentBox, $this->boxQuantitiesAvailable[$currentBox] + 1); $packer->setItems($items); - return $packer->doBasicPacking(true); + $packedBoxes = $packer->doBasicPacking(true); + if ($packedBoxes->count() !== 1 || $packer->getUnpackedItems()->count() !== 0) { + return new PackedBoxList($this->packedBoxSorter); + } + + return $packedBoxes; } /**