Skip to content

Conversation

@alexfc
Copy link

@alexfc alexfc commented Jul 9, 2021

When box item sides looks like 44, 4, 41 and Item::ROTATION_BEST_FIT expected 6 permutations, but rotations 4 44 41 and 44 4 41 with two different values can replace each other.

@shakethatweight-simon
Copy link

For a box with a square face you are causing both orientations to be calculated when they would both be packed the same way. Example 44x44x4

Also for a perfect cube, 44x44x44 there is only one orientation worth checking, but with your prefix it would cause all variations to be checked?

@alexfc
Copy link
Author

alexfc commented Jul 9, 2021

Yes, this not ideal solution, but now

<?php

namespace Tests\Unit;


use DVDoug\BoxPacker\ItemTooLargeException;
use DVDoug\BoxPacker\Packer;
use DVDoug\BoxPacker\Test\TestBox;
use DVDoug\BoxPacker\Test\TestItem;
use Tests\TestCase;

class OrientedItemFactoryTest extends TestCase
{
    public function testPackTwoItemsInBox(): void
    {
        $this->expectException(ItemTooLargeException::class);

        $box1 = new TestBox('custom box', 41, 40, 150, 10, 41, 40, 150, 10000);

        $item1 = new TestItem('Item 1', 41, 4, 44, 1000, TestItem::ROTATION_BEST_FIT); // <== this item will be added
        $item2 = new TestItem('Item 2', 44, 41, 4, 1000, TestItem::ROTATION_BEST_FIT); // <== but this throw exception

        $packer = new Packer();

        $packer->addBox($box1);
        $packer->addItem($item1);
        $packer->addItem($item2);

        $packer->pack();
    }
}

@dvdoug
Copy link
Owner

dvdoug commented Jul 9, 2021

Hi @alexfc

Thanks for this report, wrong-duplicate is obviously a theoretical possibility here but I never saw a a real-life example where this could duplicate wrongly. Surely 4 44 41 and 44 4 41 are items with identical dimensions though (one is just a 90deg rotation of the other)

@alexfc
Copy link
Author

alexfc commented Jul 10, 2021

Thanks.

@alexfc alexfc closed this Jul 10, 2021
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.

3 participants