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

Bugs in edgy rect placement in updateBinSize() #55

Open
JohannesHoffmann opened this issue Mar 13, 2023 · 1 comment
Open

Bugs in edgy rect placement in updateBinSize() #55

JohannesHoffmann opened this issue Mar 13, 2023 · 1 comment

Comments

@JohannesHoffmann
Copy link

JohannesHoffmann commented Mar 13, 2023

Hi,

i found two edge cases where the library is not placing the rects as intended.

Unrotated rect will not fit, rotated will fit but it moves to a new bin.

This is a test for maxrects-bin.ts describing the edge case:

test("edge case: only rotated version fits and should be set", () => {
        const edgeCaseBin = new MaxRectsBin(256, 1024, 0, {allowRotation: true, pot: false});
        edgeCaseBin.add(260, 80);
        edgeCaseBin.add(260, 80);
        edgeCaseBin.add(260, 80);
        edgeCaseBin.add(260, 80);
        expect(edgeCaseBin.rects).toHaveLength(4);
});

The reason for this bug is here:

if (rotWidth * rotHeight < tmpWidth * tmpHeight) {
tmpWidth = rotWidth;
tmpHeight = rotHeight;
}
.
The comparison checks only the areas and not if a rect fits or not. The edge case is that the unrotated rect will not fit but has the smaller area then the rotated version which will fit.

With Padding, four rects only placeable rotated. First three rotate, fourth not. This is should be impossible.

This is a test for maxrects-bin.ts describing the edge case:

test("edge case: multiple rects with slightly bigger size then maxWidth should be placed rotated", () => {
        const edgeCaseBin = new MaxRectsBin(256, 1024, padding, {allowRotation: true, pot: false, square: false, smart: true});
        edgeCaseBin.add(260, 80);
        edgeCaseBin.add(260, 80);
        edgeCaseBin.add(260, 80);
        edgeCaseBin.add(260, 80);

        expect(edgeCaseBin.rects).toHaveLength(4);
        expect(edgeCaseBin.rects[3].rot).toBeTruthy();
        expect(edgeCaseBin.rects[3].width).toBe(80);
    });

This bug occures only on the given options. The fourth rect is not placed rotated when width is bigger then maxWidth + padding

Here i am not sure why this happens. I guess that the reason is in here:

if (tmpWidth > this.maxWidth + this.padding || tmpHeight > this.maxHeight + this.padding) {
return false;
}

The tmpHeight and tmpWidth values are without padding compared to the maxWidth and maxHeight plus padding.

I will provide a PR with the fixes.

JohannesHoffmann added a commit to JohannesHoffmann/maxrects-packer that referenced this issue Mar 13, 2023
@soimy
Copy link
Owner

soimy commented Mar 23, 2023

Sorry for the delay. I may run some tests on the PR this weekend, hopefully ;-)

usmanpakistan added a commit to usmanpakistan/maxrects-packer that referenced this issue Apr 26, 2024
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

No branches or pull requests

2 participants