Senya Shirt #3762
Senya Shirt #3762
Conversation
|
Someone is attempting to deploy a commit to the freesewing Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Ignored Deployments
|
There was a problem hiding this comment.
|
FYI, to get the Senya design to work in the localhost lab development environment to test it, I had to:
@@ -237,2 +237,9 @@
},
+ "senya": {
+ "description": "A FreeSewing pattern for a shirt",
+ "code": ["timorl"],
+ "design": "timorl",
+ "difficulty": 3,
+ "tags": ["tops"]
+ },
"shin": {
@@ -4,0 +5 @@ import { waistband } from './waistband.mjs'
+import { draftRingSector } from './shared.mjs'
@@ -13 +14 @@ const Sandy = new Design({
-export { skirt, waistband, Sandy }
+export { skirt, waistband, draftRingSector, Sandy }Sandy currently does not export |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3762 +/- ##
========================================
Coverage 97.14% 97.15%
========================================
Files 15 15
Lines 4455 4466 +11
Branches 534 538 +4
========================================
+ Hits 4328 4339 +11
Misses 123 123
Partials 4 4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Thanks a lot for the help! I now made the PR actually look like a PR, mostly.
Yup, otherwise I wouldn't have been able to sew it. :D But the standard way of making it work creates a lot of changes I don't understand (
Imo it makes sense for such a function to be externally accessible, it's very generic. Ideally it would be in some library both Sandy and Senya use, but I don't know where that would be and I think Sandy exporting it is fine as well, right? I removed one TODO I forgot about, and added an option because of that. The remaining problems I don't know what to do about:
|
(I'll file the bug for the dimensions bug.) FYI, until the bug is fixed, a workaround for Senya might be to use an @@ -81,6 +81,7 @@ function senyaBack({
// Remove dimensions that are front only
macro('rmd', { ids: ['frontOnly'] })
macro('vd', {
+ id: 'backDimension',
from: points.cbWaist,
to: points.cbNeck,
x: points.cbWaist.x - sa - 15,Another workaround, exploiting how the bug works, would be add the first dimension twice in Front, so the same dimension is contained in |
Thanks BenJamesBen!
Edit: aw damn, adding a new measurement breaks the tests, so apparently requires more work. <_<" I'll look into that, although I would be grateful for pointers. |
|
Where exactly are you wanting people to measure for this? Like a rib measurement? |
|
I was going for "narrowest part of your chest", so yeah. Sorry if I'm messing up some nomenclature, I am a complete amateur. ^^" |
|
An alternate way of doing things, instead of having midriff measurements, might be to simply use the Then, have an option to adjust the "midriff" line down from the HPS-to-bust bustline or up from the HPS-to-waist waistline. Or, have the option be a percent 0 to 80, with 0 being the waistline and 100% being the bustline (and 80% or whatever representing the underbustline). Unfortunately there is no standard HPS-to-underbust measurement. And then, have an option to adjust the waist/underbust circumference to represent the calculated midriff circumference. Otherwise, you will need to document and explain what you mean by the midriff measurement and how it is different than the underbust or waist. I am still unclear about how to find my midriff to take a measurement. What if the narrowest part of my chest is my underbust? What if the narrowest part of my chest is my waist? What happens if someone intends to construct Senya to wear with a tightlaced corset? Would their midriff be located below their navel? |
designs/senya/src/base.mjs
Outdated
| if (complete) { | ||
| points.title = new Point(points.armholePitch.x / 2, points.armholePitch.y) | ||
| points.logo = points.title.shift(-90, 100) | ||
| snippets.logo = new Snippet('logo', points.logo) |
There was a problem hiding this comment.
This Snippet is causing a floating logo to appear. (Ignore the "JSON". It was inadvertently captured in the screenshot.)
There was a problem hiding this comment.
Yuup, removed the whole if, I don't think there was anything of value there.
designs/senya/src/base.mjs
Outdated
| chestEase: { pct: 12, min: 5, max: 25, menu: 'fit' }, | ||
| backNeckCutout: { pct: 8, min: 4, max: 12, menu: 'fit' }, | ||
| // Advanced | ||
| acrossBackFactor: { pct: 98, min: 93, max: 100, menu: 'advanced' }, |
There was a problem hiding this comment.
Changing the acrossBackFactor option value doesn't seem to have any effect on the generated pattern?
Armhole Pitch is calculated but doesn't seem to be used?
There was a problem hiding this comment.
Uh, I even had a comment about this being probably not useful... Anyway, removed this and all the useless things that depended on it.
designs/senya/src/back.mjs
Outdated
| points.sleeveCp1, | ||
| points.sleeveCp2, | ||
| Path | ||
| ).hide() |
There was a problem hiding this comment.
Re-calculating paths.bottomAndSide and paths.sleevecap is superfluous because they were already calculated in the inherited Front part. The paths are the same, so they don't need to be re-calculated. The code and imports can be removed.
The only paths that changed and need re-calculating are the shoulder/neck, the full seam, and sa.
There was a problem hiding this comment.
Right, I think this was at least partially an artifact of me not understanding how this whole thing works, removed the unnecessary computations.
designs/senya/src/back.mjs
Outdated
| from: points.cfNeck, | ||
| to: points.cfWaist, | ||
| grainline: true, | ||
| }) |
There was a problem hiding this comment.
This macro shouldn't needed. A cutonfold macro was already inherited from the Front part.
(There seems to be a bug in the Cutting Layout where the behaviors are different and incorrect both with and without the macro. I'll need to investigate the issue.)
Edit: added: leave it in for now.
With:
Without:
designs/sandy/src/index.mjs
Outdated
| @@ -10,4 +11,4 @@ const Sandy = new Design({ | |||
| }) | |||
|
|
|||
| // Named exports | |||
| export { skirt, waistband, Sandy } | |||
| export { skirt, waistband, draftRingSector, Sandy } | |||
There was a problem hiding this comment.
If we export this method, does it need to get documented? Do we need to involve or inform the designer? Is this the best way to do this? Or, is there a different way we should be accessing the function rather than exporting from Sandy? Copying the code to Senya? Doing something like import { draftRingSector } from '../../sandy/src/shared.mjs' in bottom.mjs?
(I don't know the answers to these questions. I'm hoping that more knowledgable people will provide input.)
There was a problem hiding this comment.
// I don't know the answers either, so below just guesses based on my experience as a software dev; will defer to anyone with a stronger opinion.
- It preferably should be documented, but I have no idea where. The function itself is relatively well commented, maybe that is enough?
- Probably not, the license lets us do much more with the code if we want to and making one function exported is absolutely tiny. As for politeness, if they are still around they'll see it anyway, and if they aren't they are extremely unlikely to care.
- No, having it in a separate "library" (or whatever the equivalent here is) would be best, but I don't have enough understanding of neither javascript nor this codebase to be able to do that properly. ^^"
- Copying code sounds much worse than importing it, especially since this is very generic code and it does exactly the desired thing.
- I think I tried this form of import and it didn't work (somethingsomethingoutofmodule?), but see my comment above about my lack of JS knowledge. :P
designs/senya/src/base.mjs
Outdated
|
|
||
| export const base = { | ||
| name: 'senya.base', | ||
| hide: true, |
There was a problem hiding this comment.
There is a new way/API to hide parts. It was implemented just a month ago or so. Please see: https://freesewing.dev/reference/api/part/config/hide
I'm guessing that you'll probably use hide: {self: true}, and hide: 'HIDE_TREE', most often, but i haven't checked all of Senya parts to know if that is so.
This comment applies to other parts.
There was a problem hiding this comment.
I did the first one for base, the second for front and bottom, nothing for back.
|
I think it would be the best idea to stick to waist and chest measurements
for something like this, since some people don't have (and as of yet, can't
add unless they say they have breasts) an underbust measurement in the
system. I'm also unclear on why you need specifically the smallest part of
the chest, but in that case waist would probably be better than chest. Then
there are people like me whose waists are larger than the smallest parts of
our chest, but I'm not sure how you could accommodate for that either way
…On Sat, Apr 8, 2023, 6:53 PM BenJamesBen ***@***.***> wrote:
An alternate way of doing things, instead of having midriff measurements,
might be to simply use the underbust or waist measurement (whichever one
makes more sense... I don't have a good grasp on this subject).
Then, have an option to adjust the "midriff" line down from the
HPS-to-bust bustline or up from the HPS-to-waist waistline. Or, have the
option be a percent 0 to 80, with 0 being the waistline and 100% being the
bustline (and 80% or whatever representing the underbustline).
Unfortunately there is no standard HPS-to-underbust measurement.
And then, have an option to adjust the waist/underbust circumference to
represent the calculated midriff circumference.
Otherwise, you will need to document and explain what you mean by the
midriff measurement and how it is different than the underbust or waist. I
am still unclear about how to find my midriff to take a measurement. What
if the narrowest part of my chest is my underbust? What if the narrowest
part of my chest is my waist? What happens if someone intends to construct
Senya to wear with a tightwaist corset? Would their midriff be located
below their navel?
—
Reply to this email directly, view it on GitHub
<#3762 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/APLWJYXJEEJHJ55WY2B6L7LXAIJCTANCNFSM6AAAAAAWUDP7WU>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
I think I was vaguely going for something like the Empire waistline, maybe a tad lower. But the discussions above made me lean towards something mostly like what @BenJamesBen describes. I would suggest a solution like:
I would be happy to implement 2 & 3, no idea how to do 1 (but I suspect I should be able to figure it out). I will only get to this after we reach some consensus as to how this should work. @raphaelsiz I'm also among the people whose waist is larger than the narrowest part of their chest, this was even the exact motivation for this separate measurement. :P |
|
I don't like the solution with special-casing 100% – I thought in case of Sandy the "seamless circle" option was present exactly for this usecase. Anyway, because of that I instead limited the percentage here to 90%, which should make this not happen with (almost?) any settings. I'll write some docs, if this is not merged by when I'm done I'll add them to this PR, otherwise open a separate one. Thanks for the pointer as to where to look for them! Perhaps I'll also try documenting this somewhere in the guide for creating designs. |
|
Yay, I'm back! I added some rudimentary documentation, it's not great but it's something. I have no idea how to test it though (even by looking at it <_<"). Unfortunately this no longer works somehow – when I run Side problem: the button there has "Senya.t" and "Senya.d" instead of the title and description. I thought I put these in the correct place, but apparently not, so where should they be? |
|
Eh, and now it no longer even compiles... I am completely lost I'm afraid. :/ |
Apologies for both the long wait, and for the volatility. As mentioned before, v2 was frozen sometime last year and I've been busy with the v3 release. Today, not only is v3 out, I have also implemented my preferred solution I mentioned above, which is to put the ring sector functionality into its own plugin. So now I'd like to revive this effort and land it in our repo and on our website.
I'm happy to do it, and realistically that will be the faster option. |
|
Lemme try myself for now, I'll let you know if I hit any more walls. I'm writing this before even properly trying, so it might turn out it will be fast, but maybe I'll manage. |
|
Phew, seems to be working although a lot has changed so I might have done some weird things... Also, I'm not sure if the tag set is correct. |
designs/senya/src/back.mjs
Outdated
| .setClass('fabric') | ||
|
|
||
| // Complete pattern? | ||
| if (complete) { |
There was a problem hiding this comment.
You no longer have to wrap this in a complete check.
All annotations check themselves whether complete is set yes or no, and if it is not, they don't output anything.
By all annotations I mean everything that comes from plugin-annotations, so title, scalebox, dimensions, snippets, ...
There was a problem hiding this comment.
I completely removed these ifs, as I think I don't actually need it for anything beyond the things you mention here. If this is correct, then I'm very glad this is the case. :3
designs/senya/src/back.mjs
Outdated
| points.scaleboxAnchor = points.scalebox = points.title.shift(90, 100) | ||
| macro('scalebox', { at: points.scalebox }) | ||
|
|
||
| if (sa) { |
There was a problem hiding this comment.
Independent of hte remark about not needing the if (complete) { } loop above, you should not make SA dependent on complete.
If a user wants a non-complete pattern with SA, we should give it to them.
There was a problem hiding this comment.
Should be irrelevant with the complete check removed, but noted for the future.
designs/senya/src/back.mjs
Outdated
| } | ||
|
|
||
| // Paperless? | ||
| if (paperless) { |
There was a problem hiding this comment.
There is no need to wrap dimensions in a paperless check, dimensions will check themselves if paperless is active, and if not they will not output anything.
There was a problem hiding this comment.
Removed everywhere then.
designs/senya/src/back.mjs
Outdated
| // Paperless? | ||
| if (paperless) { | ||
| // Remove dimensions that are front only | ||
| macro('rmd', { ids: ['frontOnly'] }) |
There was a problem hiding this comment.
The rmd macro no longer exists, use this instead:
macro('rmvd', 'frontOnly')
There was a problem hiding this comment.
Changed as suggested.
designs/senya/src/back.mjs
Outdated
| x: points.cbWaistline.x - sa - 15, | ||
| }) | ||
| } | ||
| log.info('back done!') |
There was a problem hiding this comment.
Please remove this log output
designs/senya/src/front.mjs
Outdated
| x: points.cfWaistline.x - sa - 15, | ||
| }) | ||
| } | ||
| log.info('front done!') |
There was a problem hiding this comment.
Remove this log output please
| import { front } from './front.mjs' | ||
| import { back } from './back.mjs' | ||
| import { bottom } from './bottom.mjs' | ||
|
|
There was a problem hiding this comment.
You'll need to import and re-export the translations as i18n
There was a problem hiding this comment.
Done, although I think I must've missed something, because the translations don't show up in the lab. :/
| title: "Senya shirt" | ||
| --- | ||
|
|
||
| <PatternDocs pattern='senya' /> |
There was a problem hiding this comment.
This should be:
<DesignDocs design="senya" />
| @@ -0,0 +1,35 @@ | |||
| --- | |||
| title: "Sandy circle skirt: Sewing Instructions" | |||
There was a problem hiding this comment.
You forgot to update this title :)
There was a problem hiding this comment.
Whoops, removed any evidence of my misdeeds.
| title: "Senya shirt: Required Measurements" | ||
| --- | ||
|
|
||
| <PatternMeasurements pattern='senya' /> |
There was a problem hiding this comment.
This should be:
<DesignMeasurements design="senya" />
|
|
||
| macro('sprinkle', { | ||
| snippet: 'notch', | ||
| on: ['in1Rotated', 'gridAnchor'], |
There was a problem hiding this comment.
Is in1Rotated where you intended the notch to be?
Edit: added: I think that there were some changes to Sandy that changed/renamed some of the points. I think that points.in2 is the new name for the point you want the notch on.
|
|
||
| // Paperless? | ||
| if (paperless) { | ||
| macro('vd', { |
There was a problem hiding this comment.
I think there might be a bug with Sandy's dimensions for non-50% circles.
See: #5859
| @@ -16,6 +16,7 @@ | |||
| *.env | |||
| *.sh | |||
| *.json | |||
| *.local | |||
There was a problem hiding this comment.
Was this file accidentally changed and added to this PR? Or, is it something that is needed because sites/sde/env.local was added to the repo?
| points.neck = points.hps.shiftFractionTowards(points.shoulder, options.necklineWidth) | ||
| points.neckCp2 = points.neck | ||
| .shiftTowards(points.shoulder, points.neck.dy(points.cfNeck) * (0.2 + options.necklineBend)) | ||
| .rotate(-90, points.neck) |
There was a problem hiding this comment.
There is something about the way points.neckCp2 is calculated that causes the neckline curve seam allowance to be misshapen at times.
I made this design and it turned out ok, so I want to share it. Unfortunately, I completely don't get how to do anything else than the code (sewing instructions? adding pictures? making it show up somewhere? the last part kinda worked, but produced a huge diff and then I did weird things with it, now I have no idea how to have it work again) or in general make a proper PR, so this is probably useless without some help, sorry. <_<" I'm very willing to put in more work to get this polished, but I would need assistance with that.
Oh, and this was originally done on some slightly older version of
develop, I'm not sure it in any way (even after fixing the stuff mentioned above) works on the current one, sorry. >.<There are some photos of the finished product here.
As a fun aside, with the help of a friend I used Inkscape and a, uh, "manual plotter"(?) to transfer the design onto fabric to avoid having to print a lot. Some details here, more on request if anyone's interested.
Edit: oh, I almost forgot – I am technically using the
waistmeasurement family incorrectly, instead of measuring the waist I measured the thinnest part of the chest. There was no measurement for that (afaik?), so I cheated.