Skip to content
This repository was archived by the owner on Apr 2, 2025. It is now read-only.

Conversation

BenJamesBen
Copy link
Contributor

@BenJamesBen BenJamesBen commented May 20, 2023

This PR adds a strictMax property to the bin pack() call to take advantage of packing improvements to be implemented in freesewing/bin-pack-with-constraints#1 .

With that PR and this one, there will be improvements in parts packing in cutting layouts, as described in #3909.

This PR can be merged before or after eriese/bin-pack-with-constraints#1. Currently, the strictMax property is ignored by bin-pack-with-constraints, so merging this PR before that one will result in no change in behavior.

@boring-cyborg boring-cyborg bot added the 📦 core Core library label May 20, 2023
@vercel
Copy link

vercel bot commented May 20, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
freesewing-lab ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 20, 2023 4:10am
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
v2-freesewing-dev ⬜️ Ignored (Inspect) May 20, 2023 4:10am

@vercel
Copy link

vercel bot commented May 20, 2023

@example123 is attempting to deploy a commit to the freesewing Team on Vercel.

A member of the Team first needs to authorize it.

@codecov
Copy link

codecov bot commented May 20, 2023

Codecov Report

Merging #4092 (03e29b8) into develop (4b21956) will decrease coverage by 0.02%.
The diff coverage is 80.00%.

@@             Coverage Diff             @@
##           develop    #4092      +/-   ##
===========================================
- Coverage    98.43%   98.41%   -0.02%     
===========================================
  Files           16       16              
  Lines         4731     4735       +4     
  Branches       588      589       +1     
===========================================
+ Hits          4657     4660       +3     
- Misses          69       70       +1     
  Partials         5        5              
Flag Coverage Δ
core 98.41% <80.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/core/src/pattern/pattern-renderer.mjs 99.47% <80.00%> (-0.53%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@joostdecock
Copy link
Member

This is all feeling a bit ad-hoc and brings back memories to the time before the monorepo where changes in various repositories had to carefully click together to make things work. I really don't want to go back to that.

So @eriese forked the bin-pack package for their fabric cutting layout. Do we want this? Not sure. I'm sort of letting things evolve as things are in alpha and I appreciate that we're still figuring out what works best.

But if we are serious about maintaining our own fork of bin-pack it should be added as a package in our monorepo.
On the other hand, core doesn't care because the cutting layout thing is not a core feature. So it probably makes more sense to see how we can plug a different layout engine into core so that people have options for the layout. Perhaps we want a different layout engine for the cutting layout, and so on.

I feel that's something we need to figure out because my concern is that people are somewhat cavalier about the API boundary between core and our frontend.

@eriese
Copy link
Contributor

eriese commented May 23, 2023

So @eriese forked the bin-pack package for their fabric cutting layout. Do we want this? Not sure. I'm sort of letting things evolve as things are in alpha and I appreciate that we're still figuring out what works best.

My feeling is that we were using an external library for bin-packing and as our needs evolved we changed to one that better suited our needs. That particular library is not actively maintained, so I forked it instead of submitting a PR that would never get merged.

But if we are serious about maintaining our own fork of bin-pack it should be added as a package in our monorepo.

I don't think we are serious about maintaining our own fork of bin-pack, and I definitely don't think we need to release a new version of bin-pack every time we release new freesewing versions. This is a package that should change almost never, but Ben has submitted a pull request to that package because he wanted another behavior. This isn't the first time, nor will it be the last, that one of us submits a pull request to an external library and then says "I made a change in our code that has no effect now but will make sense if/when my external PR is accepted"

On the other hand, core doesn't care because the cutting layout thing is not a core feature. So it probably makes more sense to see how we can plug a different layout engine into core so that people have options for the layout. Perhaps we want a different layout engine for the cutting layout, and so on.

I do imagine that some people with their own implementations would appreciate being able to plug in their own packing library. Until we decide to do that, though, what Ben is suggesting here is that we allow full use of all the features available in the packing library that we are currently using. If we were to implement a plugin instead, it would presumably be identical code and it would want these changes Ben proposes.

I feel that's something we need to figure out because my concern is that people are somewhat cavalier about the API boundary between core and our frontend.

It has benefits for the cutting layout, and that's the only current use case we can think of, but it's available for use in all circumstance, so I don't agree that it's blurring the boundary between front end and core. I think it still honors the idea that the core should not make decisions based on conditions that will only exist in our own front end.

The only thing I would change here is that now that there are two packing-specific settings to look for, it might make sense to change to a packing setting, and whatever is in there gets passed to the packer.

@joostdecock
Copy link
Member

Superseded by #5133

@BenJamesBen BenJamesBen deleted the cutting-layout-packing branch January 24, 2024 15:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
📦 core Core library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants