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

51 expanding one dimension #54

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

JohannesHoffmann
Copy link

@JohannesHoffmann JohannesHoffmann commented Mar 10, 2023

Hi, here is my version on how it should work.
This PR tackles #51
It currently allows only the width dimension.

What have i done:

  • found a bug when oversized elements fits into bin but in rotated manner
  • add tests to check the bug
  • changed PACKING_LOGIC values to be strings instead of numbers to make the logic option more readable
  • Added new logic option fillWidth because this feature is mainly related to the findNode method
  • added logic in findNodefor fillWidth
  • added tests for logic fillWidth
  • addes some comments helping me and hopefully others to explain what some code is doing
  • updated Readme.md to explain logic option

hope everything is clear and seeing forward for some feedback :)

@JohannesHoffmann
Copy link
Author

Here is a visual result for my usecase and i am really happe with it:
Bildschirmfoto 2023-03-10 um 12 58 43
(lightblue boxes are rects, darker boxes are freeRects)

@soimy
Copy link
Owner

soimy commented Mar 10, 2023

The results look great!
But I prefer to let PACKING_LOGIC stick with enum rather than string. The switch of string will break many packages depending on this module, which will be a breaking change.

@JohannesHoffmann
Copy link
Author

oh yeah good point. will change it back to enum

@JohannesHoffmann
Copy link
Author

Just found out how to make it better druring setup in my project. Changed the code to keep the method updateBinSize() of maxrects-bin.ts untouched. Noticed that this.verticalExpand set to always false for the fill width method leads to the same result AND it stores the actual used width instead of always the maxWidth. I like this more then my previous approach. Both width and height are now working the same and the same as with other logics when smart is set to true.

@coveralls
Copy link

Coverage Status

Coverage: 91.299% (+0.4%) from 90.862% when pulling e88abfa on JohannesHoffmann:51-expanding-one-dimension into 11fd72c on soimy:master.

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