Skip to content

find_best_packing(): optimize required order storage to 2 * std::size(subjects) - #20

Open
lyorig wants to merge 11 commits into
TeamHypersomnia:masterfrom
lyorig:2n_optimization
Open

find_best_packing(): optimize required order storage to 2 * std::size(subjects)#20
lyorig wants to merge 11 commits into
TeamHypersomnia:masterfrom
lyorig:2n_optimization

Conversation

@lyorig

@lyorig lyorig commented Nov 3, 2025

Copy link
Copy Markdown
Contributor

Implements #16.

As it stands, this PR is a rough draft cobbled together in my spare time that compiles and seems to work. There are most likely some things to improve (especially variable names), but hopefully it'll serve as a good starting point.

@lyorig

lyorig commented Nov 3, 2025

Copy link
Copy Markdown
Contributor Author

CI seems to have caught something weird. I'll try to look into it soon™.

@lyorig
lyorig marked this pull request as draft November 3, 2025 19:06
@lyorig

lyorig commented Nov 3, 2025

Copy link
Copy Markdown
Contributor Author

Oh yeah, I got some stuff completely backwards. Will try to fix if time allows for it.

@lyorig

lyorig commented Nov 13, 2025

Copy link
Copy Markdown
Contributor Author

Checks are currently failing due to a single misplaced rectangle, which is interesting. I'm still pretty short on time, so I won't get into debugging it right away—but it may very well be caused by some trivial UB or other coding error on my part (although ASan + UBSan stayed silent).

@lyorig

lyorig commented Jun 14, 2026

Copy link
Copy Markdown
Contributor Author

@geneotech, I've decided to rear my ugly head again, and hopefully finish up this PR after God knows how long. The output mismatch seems to be caused by individual sorts being influenced by the result of the previous one; to fix this, I instead chose to allocate 3 * std::size(subjects) pointers, and copying the initial state to the "staging area" after every sort.

So, tests are currently passing, but I wonder whether the previous iteration (which did not pass tests) was still correct in the algorithmic sense of the word, just producing different (but still valid) results. You'll probably have to help me out here with your expertise.

@lyorig
lyorig marked this pull request as ready for review June 14, 2026 21:33
@geneotech

geneotech commented Jun 21, 2026

Copy link
Copy Markdown
Member

Hey, since we confirmed the tests are green after introducing the third backup array to sort from, I believe it's okay if we just have 2 buffers to not overcomplicate this, and just change the test results. The sort is likely just non-deterministic when there's e.g. two rects with the same area.

lyorig added 2 commits June 22, 2026 15:50
This reverts commit 2713cb9.
Also changes the expected output so that tests pass.
@lyorig

lyorig commented Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

Understood, I've reverted the changes and changed the test output accordingly.

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