Fixed Bulge & Closed properties on hatch #113
Conversation
…nts to controlPoints in SPLINE and from points to vertices in POLYLINE for consistency.
There was a problem hiding this comment.
Thank you for continuing to improve this repo @ieskudero!
The rename of points -> vertices and points -> controlPoints is technically a breaking change, do you agree? I don't have a problem with breaking changes, but it is good to know and to do it intentionally so we can properly follow semver.
| if (isPolyline) loop.entities[0].vertices[loop.entities[0].vertices.length-1].bulge = value | ||
| else fillDrawEntity(type, drawType, parseFloat(value)) | ||
| } | ||
| break | ||
| case 72: | ||
| { | ||
| // !Polyline --> 1 = Line; 2 = Circular arc; 3 = Elliptic arc; 4 = Spline | ||
| // Polyline --> bulge | ||
| // Polyline --> hasBulge | ||
| drawType = parseFloat(value) | ||
| loop[isPolyline ? 'bulge' : 'edgeType'] = drawType | ||
| loop[isPolyline ? 'hasBulge' : 'edgeType'] = drawType |
There was a problem hiding this comment.
@ieskudero Could you elaborate on the (breaking change?) rename of bulge to hasBulge on row 121, please? Also in relation to the bulge property on row 112. Not sure I understand why this is needed.
There was a problem hiding this comment.
About the points->controlPoints renaming: it is in fact a breaking change, sorry. I did it to be more consistent with the names of properties, since spline has control points, not points.
About the bulge->hasBulge renaming: After realizing the error I understood the property, so I changed the name accordingly. I don't think is necessary (unless there are bulges with 0 value) but I put it anyway.
Let me know if I answered your questions, and if I didn't I can add a more detailed explanation about bulge/hasBulge properties.
I can set a different pull request separating them both, if you want.
There was a problem hiding this comment.
Sorry about the delay in answering here, lots to do! I'm still a bit confused about the loop.hasBulge property, looks to me like it will be a number (parseFloat on line 120) but the name signals a boolean. Would be great if you could help me understand @ieskudero
There was a problem hiding this comment.
According to the dxf documentation (Boundary path data) has bulge is a flag. I assume it is a true/false flag, since it doesn't mark it as a bit coded flag. I could check some examples to be sure but I bet it is a 0-1 value flag. And the parseFloat must be a mistake, inherited from the previous assumption that this was a bulge value instead of a bulge flag, it can be changed to parse to boolean or integer.
There was a problem hiding this comment.
I have read this several times. The name change for points to vertices is not necessary.
And the bulge change doesn't make sense. It looks like a fix for a special case.
maybe this needs more,test cases?
There was a problem hiding this comment.
I didn't mean to create such a long argument with this pull request, sorry. I am thinking that maybe we can take back this pull request and do other pull requests that are more focused and with less changes, because I think we are mixing things here. Then we can discuss each problem separately:
1.- renaming of properties(points to control points, points to vertices)
2.- bulge property fix
3.- renaming of bulge to hasBulge
There was a problem hiding this comment.
@skymakerolof, any opinion about this?
There was a problem hiding this comment.
I agree that the loop.entities[0].vertices[loop.entities[0].vertices.length-1].bulge part looks like it is handling some specific case? And that we need more test cases. Also like you say, once change per pull request would make things easier to follow and discuss!
There was a problem hiding this comment.
OK, perfect, you can close this one and I will make them separately
fixes #112