Conversation
|
In math (and physics) positive angles go counter-clockwise from the X+ axis. Please do not change that. Otherwise |
|
The arc one is tough - it's been discussed before, but I can not find where right now... |
|
The trick with arcs is that the end points need to be swapped. You can see code for that in the paste-transformed. The reason is that arcs are defined as counter-clockwise from the start point to the end point. This also why arcs are not created for Revolve extrusions - they were either correct or went the wrong way around depending which way you dragged it. For the paste you may run into other issues if you swap them as there may be constraints or something that won't be right. An alternative to swapping the end points is to flip the normal. Arcs have a normal and that needs to be copied with a flip. This is the same issue with flipping text entites which I haven't had enough time to look into. Maybe you or I will get them both fixed at once. BTW what does it mean to flip a normal? It's either scale it by -1 or a member function to flip it... I think... maybe. :-) |
597c936 to
b9eb94a
Compare
|
I found out that the ARC_LINE_TANGENT only uses the Lines 625 to 634 in b316a88 Swapping that when scale < 0 seems to do the trick. The same goes for CUBIC_LINE_TANGENT, reference: Lines 649 to 658 in b316a88 CURVE_CURVE_TANGENT uses both the Lines 672 to 686 in b316a88 Swapping those, and the entities also seems to work, but i'm not sure if it's the right / cleanest solution. |
b9eb94a to
ad70c2c
Compare
|
My limited? test: |
ad70c2c to
cb16d64
Compare
|
EDIT: I removed this feature, it was not working correctly and is probably too much "magic"
|
cb16d64 to
736e733
Compare
|
In hindsight I shouldn't have added the last changes to this PR, it's not working 100% correctly and it is probably out of scope of what this PR was trying to achieve in the first place (fixing bugs in the current implementation). Will update this one soon. |
|
I'm for fixing bugs for 3.0. After that i was wondering if copy-paste should use the entity copy-as stuff (at the end of group.cpp) so that all the copies will change if the original is changed. This is how copies happen for new groups - like the 2 ends of an extrude. The copies are created by applying one of several transformations to the original. All copies use the same transform parameters so they move together. Maybe that's worth opening a separate issue to discuss? @vespakoen did you flip the arc normals or fix the tangent constraints? Also, drop a text entity in there ;-) |
|
Copy-paste in my opinion should leave the copies completely independent - and thus independently editable. That is how copy-paste works "everywhere". On the other hand step translating does almost what you describe. Perhaps something like "clone multiple" which makes N clones but allows them to be moved freeley? |
|
At any rate all of this should be post 3.0 |
736e733 to
2702e92
Compare
|
Reverted the last change, now it only fixes the H / V constraints and tangent problems. |
|
(So no more dependencies between copies) It simply:
@ruevs I think this can safely be included in 3.0, it's a pretty annoying bug and I think is worth including the fix asap. |
ruevs
left a comment
There was a problem hiding this comment.
This does not quite work. It moves the constraints to the "other"/wrong end of the curves sometimes.
|
This does not quite work. It moves the constraints to the "other"/wrong end of the curves sometimes. |
|
|
|
The constraints between the vertical line and the bezier and the horizontal line and the bezier are swapped. |
|
Thanks testing! I'll take another look at this. |
2702e92 to
fc44f80
Compare
|
Think I fixed it, works with both of our tests now. |
|
Hmm not quite. Lines tangent to beziers still get "swapped" incorrectly. |
ruevs
left a comment
There was a problem hiding this comment.
Still not correct for two lines tangent at the ends of a bezier.
21017b6 to
8aa3335
Compare
|
I guess I found the correct solution, but this duplicates code from constraint.cpp. I should probably move these into a function that can be called from the clipboard.cpp as well. |
8aa3335 to
2e8f4aa
Compare
|
Just moved the duplicated stuff into new functions, seems to work good now, let me know if this is the right way / place of things. |
|
@vespakoen swap(a,b) literally swaps the "values" two variables of the same type. By values I mean all members, so Vectors will exchange x,y, and z values. For example, line/arc tangent constraint appears to be unsure of which parameter is the arc because they are the same type? So it checks and if the one whose variable name is line happens to be the arc, it swaps line and arc. Why? because one has 2 points and one has 3 plus a normal, so it's important to have them the right way when making the constraint equations. An example would be mirroring an arc. The entity contains handles to the 2 end points, so the mirror image should mirror all points and then in the arc entity it should swap the handles for the 2 end points to ensure it goes the right way around. Either that or flip the normal. |
|
I think the implementation is fine. You have to try all 3 kinds of constraints you modified code for, both pasted and regularly applied to existing entities. Then I'm happy with it. |
2e8f4aa to
1ba8d09
Compare
|
I just did a whole bunch of testing and actually found a problem with my code, but that is now fixed and it is working perfectly now. I have tested all the changed constraints, in both "directions" of selecting, and also mirrored and pasted, then removed and re-applied constraints. All is working fine. |
|
OK now it works for all test I tried. And it looks good. I will do an extra small clean up. |
This fixes a regression introduced in 96958f4 (solvespace#833.) Three `return` statements got "swallowed" by the newly created functions. (e.g .solvespace@96958f4#diff-49abc03ed071148c0ebae0c64aafb625fa6223135f77aeecdb47dab4cf8b940cL638 ) Because of this it was possible to constrain arcs and cubics tangent to each other or line segments, without them sharing an endpoint. This kind of worked, but always chose the "starting" points of the curves and lines. In the future this can be turned into a feature. See the discussion in solvespace#937.
This fixes a regression introduced in 96958f4 (#833.) Three `return` statements got "swallowed" by the newly created functions. (e.g .96958f4#diff-49abc03ed071148c0ebae0c64aafb625fa6223135f77aeecdb47dab4cf8b940cL638 ) Because of this it was possible to constrain arcs and cubics tangent to each other or line segments, without them sharing an endpoint. This kind of worked, but always chose the "starting" points of the curves and lines. In the future this can be turned into a feature. See the discussion in #937.
…edge The functions Constraint:: ConstrainArcLineTangent ConstrainCubicLineTangent ConstrainCurveCurveTangent were introduced in 96958f4 in order to properly paste arcs tangent to other objects when mirrored. See the original pull request for details solvespace#833 and test files. However these functions use the original logic in constraint.cpp that relies on the two entities sharing an end point. This will interfere implementing tangents on entities without coincident points so I changed the logic to remove the dependency. As a side benefit it makes it cleaner and it's intent more obvious. See solvespace#937
This fixes a regression introduced in 96958f4 (solvespace#833.) Three `return` statements got "swallowed" by the newly created functions. (e.g .solvespace@96958f4#diff-49abc03ed071148c0ebae0c64aafb625fa6223135f77aeecdb47dab4cf8b940cL638 ) Because of this it was possible to constrain arcs and cubics tangent to each other or line segments, without them sharing an endpoint. This kind of worked, but always chose the "starting" points of the curves and lines. In the future this can be turned into a feature. See the discussion in solvespace#937.
…edge The functions Constraint:: ConstrainArcLineTangent ConstrainCubicLineTangent ConstrainCurveCurveTangent were introduced in 96958f4 in order to properly paste arcs tangent to other objects when mirrored. See the original pull request for details solvespace#833 and test files. However these functions use the original logic in constraint.cpp that relies on the two entities sharing an end point. This will interfere implementing tangents on entities without coincident points so I changed the logic to remove the dependency. As a side benefit it makes it cleaner and it's intent more obvious. See solvespace#937
This fixes a regression introduced in d1906c2 (solvespace#833.) Three `return` statements got "swallowed" by the newly created functions. (e.g .solvespace@d1906c2#diff-49abc03ed071148c0ebae0c64aafb625fa6223135f77aeecdb47dab4cf8b940cL638 ) Because of this it was possible to constrain arcs and cubics tangent to each other or line segments, without them sharing an endpoint. This kind of worked, but always chose the "starting" points of the curves and lines. In the future this can be turned into a feature. See the discussion in solvespace#937.
…edge The functions Constraint:: ConstrainArcLineTangent ConstrainCubicLineTangent ConstrainCurveCurveTangent were introduced in d1906c2 in order to properly paste arcs tangent to other objects when mirrored. See the original pull request for details solvespace#833 and test files. However these functions use the original logic in constraint.cpp that relies on the two entities sharing an end point. This will interfere implementing tangents on entities without coincident points so I changed the logic to remove the dependency. As a side benefit it makes it cleaner and it's intent more obvious. See solvespace#937
I found some more oddities with paste transformed.
I am hoping I can solve them and am planning to add some new tests for these as well.
The first thing I noticed is that rotating happens in counter clockwise direction (at least in the workplane that is opened by default when starting a new sketch)stupid me, scrap that.The second thing I noticed is that when flipping an arc with tangent constraints, the arc gets inverted, removing the constraints fixes this, but there should be a better way to actually invert the arc which I haven't found yet.
Here is a little sketch to try out.
rotate-and-flip-me.slvs.zip