Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature - New Group: Helix #389

Closed
phkahler opened this issue Mar 26, 2019 · 56 comments
Closed

Feature - New Group: Helix #389

phkahler opened this issue Mar 26, 2019 · 56 comments

Comments

@phkahler
Copy link
Member

phkahler commented Mar 26, 2019

Several people have wanted the Lathe operation to limit the angle of revolution to less than 360 degrees. This feature would work like Lathe but would allow sweeping the sketch though arbitrary angles while also sliding it along the axis of revolution to produce helical objects. Because NURBS can not exactly represent helical surfaces (the angular vs axial position is not perfectly linear) there should be adjustable number of segments to allow the user to minimize that distortion. When the axial displacement is 0.0 this will produce the same geometry as Lathe but with adjustable sweep angle. All surfaces created this way would be pure NURBS without doing boolean operations.

@Evil-Spirit
Copy link
Collaborator

Evil-Spirit commented Mar 27, 2019

This is excactly how I made it for NoteCAD.
Unity 2018 3 2f1 Personal - NoteCAD unity - NoteCAD - WebGL_ DX11 on DX9 GPU 2019-03-27 13 27 34

@phkahler
Copy link
Member Author

phkahler commented Mar 27, 2019

I started working on this last night and have some questions about internals.

Getting the surface patches (control points) correct was easy but the trim curves are tricky. I've copied SShell::MakeFromRevolutionOf() as a starting point for SShell::MakeFromHelicalRevolutionOf(). The part where trim curves are created is very confusing due to me not understanding all the requirements of the internal data structures. Is there any explanation of what Remap is doing? Would it be OK to trim a surface patch using curves constructed from the same control points that define the surface? That would seem to be an easy function to implement for surface patches that don't have holes. Is there some kind of linkage needed between patches that are part of the same surface? Are the curves reused just to save memory or is it important that shared edges not be replicated?

From a code point of view I also copied SSurface::FromRevolutionOf() to SSurface::FromHelicalRevolutionOf and include two additional distance parameters to specify how far along the axis the start and end of the revolved surface are. Should this stay a separate function or should older code use the new version and pass zeros for these new parameters? It should produce the exact same results in that case with less new code.

@Evil-Spirit
Copy link
Collaborator

Evil-Spirit commented Mar 27, 2019

Only @jwesthues can answer this questions.

@jwesthues
Copy link
Member

jwesthues commented Mar 28, 2019

Remap() is the function that goes e.g. from the hEntity of a point in group 1 to the hEntity of the line in group 2 that's created when we extrude that point. An Entity is the kind of curve that the solver works with, and that's selectable in the UI. Remap() is what keeps the association between constraints and their geometry when you add or delete entities in earlier sketches and then regenerate later ones.

These Entity curves are (unfortunately) distinct from the curves that trim the NURBS surfaces, so you mostly don't care about Remap() in the surface operations. The exception is RemapFaces(), since we track which surfaces belong to plane-face entities to make them selectable, the thing that lets you select a plane face and constrain point-on-face.

SCurves that trim multiple surfaces must be created once and used in multiple STrimBys, not duplicated. The Boolean operations (and the watertightness of the mesh, and probably other stuff) depend on coincident curves being piecewise-linear approximated identically, and nothing else guarantees that. If you duplicate, then the model will look okay at first, and will probably even be watertight since the pwl code is deterministic. Bad things will happen once the Booleans start chopping up the trim curves, though.

A single function for both helical and circular sweeps seems reasonable to me, as long as it generates the exact same output as now when the pitch is zero. I'd suggest using some function of the chord tolerance to choose how finely to approximate the helical surface, rather than adding another user setting.

@phkahler
Copy link
Member Author

phkahler commented Mar 29, 2019

Here's a pic so far. I'm creating a helix with hard coded angle, pitch, and number of segments (5). These will become parameters after there is a way to for the user to determine them. For now I've patched regular rotation to call this until I work my way up toward the UI and add a new method to create a group. That's why the circles are there - they are part of a regular revolve group. One thing I have seen is that when you select a segment and point to revolve around, sometimes the direction of the axis is reversed so it spirals the other way. Or maybe the sketch normal is flipping, I think I've seen that in other experiments. There is a long way to go on this but the code is starting to make more sense.
SS_Helix

@jwesthues I noticed that in the regular revolution tool, the circular trim curves are created directly from the control points of a surface, which are created by rotating a point around an axis. The ones that are copies of the sketch curve are rotated via quaternion. I'm wondering if very slight numerical differences between these are relevant. What do you think?

@jwesthues
Copy link
Member

jwesthues commented Mar 29, 2019

Cool, looks helical. There's no need for the NURBS trim curves and Entitys to be bit-for-bit identical, or even particularly close--two Entitys are generally treated as coincident if they're within something on the order of LENGTH_EPS, and geometry flows only from Entitys to trim curves, never the reverse.

Of course, the Booleans often break when given coincident curved surfaces. That difference is the least of your problems there, though. "Close but not exactly equal" problems in SolveSpace usually involve differences on the order of LENGTH_EPS or of the chord tolerance, not rounding error.

@whitequark
Copy link
Contributor

whitequark commented Mar 29, 2019

@phkahler Very nice progress! I'm looking forward to having this feature.

@phkahler
Copy link
Member Author

phkahler commented Apr 11, 2019

Some comments and questions on doing this.

It will probably have to be split into two kinds of group creation - rotate/revolve and helix. Revolve seems more useful and often will involve rotation around a line in the sketch. When that happens the line/axis does not sweep out a surface, so it is fundamentally different from most hilixes. The NURBS handling in solvespace doesn't seem to like highly twisted surfaces and does like a helical extrusion along a line in the sketch. So these operations seem like they will be easier if handled separately.

I can not figure out how to copy sketch entities to the "new" end of the helix. Getting the surface control points and trim curves there was reasonably easy. But I'm looking at Group::Generate() -> case:Type::EXTRUDE and case:Type::LATHE and it's just confusing. What does CopyEntity() actually do? What are the remap types for? Where are the expressions for extruded end points actually created?

I expected a new point to be an existing point plus the sketch normal scaled by a free parameter, but that does not seem to exist in the code.

@jwesthues this code appears fairly readable, but something is missing and for as clear as some parts are, others are just confusing me.

Also, there is a severe problem with normal vectors. Maybe I'm off base, but I think extruding a sketch should produce the same geometric result regardless of which way you're looking at it. Go ahead, draw a rectangle, extrude it and rotate a little to the side to see that it came "out of the screen". Undo the extrusion, rotate 180 degrees so you're viewing the "back" of the sketch and do a new extrusion. It will come out of the screen at you rather than going where it did the first time. While this may seem intuitive to a CAD user, it does not make sense to me as a programmer, nor can I see where it happens in the code. I fear this is also related to some other problems with normal vectors....

If I do a simple revolve through an angle (non-helical) I get what you'd expect - a partial ring with two end caps. If I drag the points in the original sketch such that the left and right sides (I revolved a rectangle in the upper right quadrant) switch places, the extrusion flips from "into" to "out of" the screen. I gets worse. If I comment out the code:

if(vp.Dot(axis) < 0) {
    axis = axis.ScaledBy(-1);
}

which I borrowed from SShell::MakeFromRevolutionOf(), then sometimes moving the sketch points around will turn the surface red - which I believe means the surface normals are facing inward rather than outward. So that code is fixing a problem in the lathe function, but it's not fixing it all. You don't see the angle reversal in a lathe because it goes a full 360 degrees so it looks the same either way.

This normal problem happens if I use a sketch line for the axis and select the origin as the point to revolve around. It all goes away if I use one of the basis vectors at the origin as the axis and the origin as the point.

I can understand that if my axis is a segment connecting two points, swapping those points may reverse the direction of rotation, but there is more going on and more cases than can be explained by that alone (I think).

There is a similar flip in the straight extrusion code. A while back I created a frustum (surfaces only, couldn't figure out the entities) by scaling one set of end points. As I dragged the sketch around it would flip which end was smaller and larger. Very disturbing and I couldn't identify when it would flip. Again I commented out the code to flip the normal. That problem IIRC went away and instead of swapping big and small ends, the surface would turn red.

At the moment I just want to figure out how to make entities on the new end of my revolve. What and where should the code go for this?

@jwesthues
Copy link
Member

jwesthues commented Apr 11, 2019

See above for some notes on Remap(). For the existing users of CopyEntity(), the transform from the original entity to the copy is determined by the solver, and may be defined in terms of the copied entity itself. So we make copies defined in terms of solver parameters, a translation (like extrude) or rotation. You could use the existing code fairly easily for a circular sweep, since that's just a rotation. You couldn't use that for the helix, since that's a combined rotation and translation. That turns into a not insignificant subproject, so you might want to hold off for now. The lack of endcaps makes subsequent sketching less convenient, but still always possible with the right construction geometry.

When we create a new extrusion group, we default the extrusion vector to the GUI viewport normal gn, which then gets projected along the sketch plane axis by the constraint solver. That's what makes the default extrusion direction follow our view of the plane. That's just a UI decision, totally decoupled from the implementation.

solvespace/src/group.cpp

Lines 420 to 422 in a7b2f28

AddParam(param, h.param(0), gn.x);
AddParam(param, h.param(1), gn.y);
AddParam(param, h.param(2), gn.z);

Tracking normals is always a pain. As you observe, it takes considerable effort to keep that consistent. Since the direction of rotation didn't matter for any existing group types, I wouldn't have bothered. I have no special advice there, beyond carefully thinking through all the geometry, and debugging graphically (render arrows pointing out of all your edges etc., and confirm they point as expected) when in doubt.

@Evil-Spirit
Copy link
Collaborator

Evil-Spirit commented Apr 11, 2019

About axis and Direction - I can suggest you look here
https://github.com/NoteCAD/NoteCAD/blob/master/Assets/Code/Features/RevolveFeature.cs#L338
where I also solved this problem (and this takes a lot of time). As Jonathan mentioned, only visual debugging and carefull thinking can lead you to solving this issue.

@phkahler
Copy link
Member Author

phkahler commented Apr 13, 2019

@whitequark Off topic - I grabbed a fresh copy of master and it won't build per the instructions. There are complaints about Q3DO and flatbuffers. Is there a new dependency/step that isn't covered in the Linux build instructions? Reverting to the commit before that works for me.

@whitequark
Copy link
Contributor

whitequark commented Apr 13, 2019

git submodule update --init

should fix that. It's the same instructions as before, you just need to run them again.

@whitequark
Copy link
Contributor

whitequark commented Apr 13, 2019

Oh wait, you're right--the Linux instructions aren't quite what I described above. One moment.

@whitequark
Copy link
Contributor

whitequark commented Apr 13, 2019

See 945a282.

@phkahler
Copy link
Member Author

phkahler commented Apr 16, 2019

I've pushed some early test code to my geometry branch if y'all want to have a look. It adds Revolve to the menu and creates a fixed rotation angle solid from a sketch. Don't try to revolve around a sketch line yet, it will crash (use a line and separate point). I will probably write a revolve-specific function for creating the shell because that case is important (but difficult or invalid with a general helix).

For now I'm trying to copy entities using stuff from Rotate which should make the end draggable. This belongs in Group::Generate() under Revolve but I haven't worked it out yet.

One interesting bug that has me really confused. The number of sections to rotate is hard coded at 5 in SShell::MakeFromHelicalRevolutionOf. I made a change to allow up to 100. If you change it to higher values than 5, the last section will not work and a bunch of red lines will appear for that last section. No idea why. But for the simpler Revolve function we will not be going beyond 3 sections anyway.

Code reviews welcome on what I've got so far.

@jwesthues
Copy link
Member

jwesthues commented Apr 16, 2019

Small aside: you don't always want to use the minimum number of sections for the angle. A single piece can represent any angle <180 degrees, but as the angle gets bigger the convex hull or axis-aligned bounding box fit looser, slowing coarse intersection tests like those used in the Booleans.

That's why I did 90 degrees before. That also has the advantage that subsequent Boolean operations often slice through axis-aligned planes, and therefore don't need to split the surfaces.

@phkahler
Copy link
Member Author

phkahler commented Apr 17, 2019

My solvespace/geometry branch now has basic REVOLVE support. Still need to fix some things and make the surfaces selectable. But....

@jwesthues I believe there is a bug somewhere in the NURBS code that does not involve booleans. You may check out my branch to reproduce this:
NURBS_error

I'm using 3 sections here, but it happens with 4 and 5 as well. The number of chords that cut across the center hole always matches the number of sections and are evenly spaced. If I'm not mistaken it is half the arc of the section when it happens - perhaps a bug when the patch is bisected? At other angles many surfaces turn red - even with 4 sections totalling less than 360 degrees, but usually over 300 degrees.

In this revolved surface there are no boolean operations and no trimmed surfaces other than the usual outline trim curves, so this looks to be a bug in plain NURBS. I believe I saw it on one of my models once in 2.3 but it manifested as a white line across the bottom of a round hole that did not go all the way through, so I thought it was just an extra line and ignored it.

I feel like having these NURBS issues show up without booleans is a good case to explore, but I have not even looked in to that part of the code yet. I have to fix some things with this feature still ;-)

@phkahler
Copy link
Member Author

phkahler commented Apr 20, 2019

Triangle mesh from one of these. It's similar to above but the top surface is doing something very similar to the bottom. Again, this is not a boolean issue. Still trying to understand a lot of things here...
Mesh_error

@phkahler
Copy link
Member Author

phkahler commented Apr 23, 2019

@jwesthues In the quest to get more consistent behaviour of normal vectors and do fewer checks when making shells, I have an idea. The issues all stem from the function "FindOuterFacesFrom" which determines the loop normal and then sets loop direction CC vs CCW to establish loop normals. This function can flip the loop normal depending on the relative placement of points in the sketch. I propose to pass this function a preferred normal direction, and if one it pick arbitrarily does not match we can swap the u,v vectors (which are arbitrarily picked from points) so the normal matches the one passed in. We can pass in (0,0,0) to leave the behaviour unchanged when called from the places I know nothing about. A quick test suggests this will work. How do I get the sketch/workplane normal vector from in "Group::AssembleLoops" which is where it will be called from? Or is this a bad idea for some reason I don't see yet?

@jwesthues
Copy link
Member

jwesthues commented Apr 23, 2019

I don't think that solves the right problem. The plane-sketch-to-shell operations need to work correctly regardless of the normal of the plane sketch. The normals of the shell faces should be determined only by the extrusion direction, rotation axis, etc. If the incoming plane sketch normal matters, then that's the bug.

@phkahler
Copy link
Member Author

phkahler commented Apr 25, 2019

@jwesthues You are probably correct. Now I turn the revolve inside out similar to how it's done in plain extrusions. It was more complex to implement but solves the problem by making both ends of a helix variable - a capability I don't anticipate using otherwise.

BTW the problem in the above images seems to be triggered when the number of points along the inner and outer curves differ in certain ways. It's like a tessellation problem but only with NURBS and not when you force to triangle mesh. Sometimes I think it's an off-by-one issue.

@phkahler
Copy link
Member Author

phkahler commented Apr 28, 2019

@whitequark @jwesthues

Today the Revolve feature in my branch is mostly working. The end can be dragged and constrained. The above pictured rendering errors are gone. You can build on both end surfaces without additional errors that were happening with booleans and coincident surfaces. Some problems and questions remain:

  1. We still can't revolve around a line in the sketch or it will crash and I have no idea why or where it crashes. To me this is a show stopper. I don't see much different than the LATHE code. I've tried to use the axis-line as a trim curve on both end caps, which is a topological difference from LATHE and EXTRUDE but no luck - I don't think that's the source of the problem (IIRC this happened before I had end caps). I thought I was handling the degenerate surface the same way as lathe.

  2. Flat surfaces are still not selectable. I don't think this is a big deal. If I include some code in Group::Generate() that I borrowed from the LATHE case (currently commented out), flat surfaces become selectable but only if we include the LATHE circles which look terrible for revolve. The end surfaces are not selectable. To enable this will require more understanding.

Getting back to actual helical extrusions:

  1. I still think typing in the angle and axial displacement in the text window is the way to go. Dragging or constraining to get 10 revolutions of a screw thread does not seem like a good idea. Getting the solver to support that might be an adventure.

  2. there are still problems using more than 5 sections in a revolution and I have no idea why. It's hard coded at 3 right now since revolve can be done up to 360 degrees with 3 sections. It fails badly at 6 for revolve. If you add some axial displacement it works with many more but the last section always has a problem. This needs to be fixed for actual helix operations. Using just one section usually doesn't work even for small angles but sometimes does.

From a UI point of view:

  1. I still need to add revolve features to the text window. We need the "difference" check box. Do we want to support 2-sided revolves? I don't think it's needed but that does not look hard to support.

  2. if anyone reading wants to create revolve and helix icons that would be nice.

Thoughts or suggestions on these things are more than welcome.

@whitequark
Copy link
Contributor

whitequark commented Apr 28, 2019

We still can't revolve around a line in the sketch or it will crash and I have no idea why or where it crashes.

I'm a bit confused, how are you debugging the code that you don't know where it crashes? In any case I can take a look at that.

Flat surfaces are still not selectable. I don't think this is a big deal.

That's fine, really. We can always add it later.

I still think typing in the angle and axial displacement in the text window is the way to go. Dragging or constraining to get 10 revolutions of a screw thread does not seem like a good idea. Getting the solver to support that might be an adventure.

I feel like we ought to do better here than just hardcoding values in the text window. Sure, for screws it would be sufficient, what about other use cases?

Can you explain the problems that arise when integrating this with the solver?

Do we want to support 2-sided revolves? I don't think it's needed but that does not look hard to support.

2-sided revolves can be convenient when carving out a thread inside some other model. You put the origin inside the shell and then revolve it both sides slightly past the surface. This is similar to 2-sided extrusions.

if anyone reading wants to create revolve and helix icons that would be nice.

If no one else does it I will draw these icons. Do you mean you need both revolve and helix?

@phkahler
Copy link
Member Author

phkahler commented Apr 29, 2019

Two sided revolve is in, as well as difference.

I'm a bit confused, how are you debugging the code that you don't know where it crashes? In any case I can take a look at that.

It's probably time for me to learn how to use GDB eh? ;-) Got any IDE recommendations on Linux (Fedora)?

To make revolves work, I borrowed a lot from other group types. The entities are manipulated the same way as Rotate, curved surface creation is a modified version of Lathe, and the end cap creation code was adapted from Extrude. Rarely did it crash during all this.

Do you mean you need both revolve and helix?

Probably. I set out to do Helix, but Revolve was simpler and the UI is nice for just that. There is also the problem of revolving around a line in the sketch, which we can probably fix but will be impossible with helix because the surfaces do some really odd things in that case. IMHO usability issues will warrant separate tools. It's really easy to Revolve something and then constrain the angle between lines on opposite ends. That will be more awkward if the ends can also move up and down. Opinions welcome of course. Try setting the angle between skew lines.

Also, apart from the crash and an Icon I think revolve could be considered done. Please test!

Can you explain the problems that arise when integrating this with the solver?

I didn't do anything at all inside the solver yet. If we want SS style Helix I'll dig in to the solver and get it to handle entities with additional freedom and then we can decide if this new version of Revolve should be renamed Helix, or if having both variations would be better from the users point of view.

@bcmpinc
Copy link
Contributor

bcmpinc commented Apr 29, 2019

The only commands for GDB that you need to know to debug crashes are "r" (run) and "bt" (backtrace), though make sure you compiled with debug symbols enabled (-g). As for IDE's I can recommend kdevelop, though I'm not sure how well GDB integration will work for you. Another thing you can try is to run solvespace in valgrind to see if there are memory allocation and initialization related bugs that might be causing the crash.

@Evil-Spirit
Copy link
Collaborator

Evil-Spirit commented Apr 30, 2019

@phkahler

It's probably time for me to learn how to use GDB eh? ;-) Got any IDE recommendations on Linux (Fedora)?

I use QT Creator. It works good with CMake projects.

@whitequark

I feel like we ought to do better here than just hardcoding values in the text window. Sure, for screws it would be sufficient, what about other use cases?

Actually this is not looks like hardcoding (for me) and can support any of usecases so I have implemented this as option for NoteCAD (sorry for mentioning this again). If we switch on "angle fixed" we just fixing angle (not add parameter to solver) and it works as constraint. If we uncheck this, we let solver to use our angle parameter as free, so we can drag it (try it yourself).
NoteCAD 3D modelling tool - Google Chrome 2019-04-30 09 40 19

@whitequark
Copy link
Contributor

whitequark commented Apr 30, 2019

Actually this is not looks like hardcoding (for me) and can support any of usecases so I have implemented this as option for NoteCAD (sorry for mentioning this again). If we switch on "angle fixed" we just fixing angle (not add parameter to solver) and it works as constraint.

NoteCAD is a great project and I am always interested in seeing how others solved some kind of problem in a CAD, so this is useful. "Step/Angle Fixed" looks like a possible solution to me.

@phkahler
Copy link
Member Author

phkahler commented Apr 30, 2019

@jwesthues Does SolveSpace have a problem with degenerate surfaces? I've been having a crash when Revolving around a sketch line. This would normally create a degenerate surface from the revolved line, but in the code I borrowed from the lathe function it does this:
revs.d[j].v = 0;
for surfaces like that. Null handle. And then it leaves out trim curves that are shared by that surface as well. Prior to my changes, the surface creation probably would have failed trying to compute the control points for such a surface (intersection of parallel lines, actually degenerate lines) but that is no longer the case. The new version can compute the points in a valid way. The condition it's still detected and the exact numeric values are used for the coincident control points just to force them exactly.

So just for grins I tried leaving those surfaces and trim curves in for revolves and it seems to work fine. I'm wondering if this sounds like a valid solution to the crash or if I should find the problem with leaving those surfaces out. It seems like with the NURBS issues this could be a bad thing, but I even tried doing a difference through that edge with no problem.

Do you think this is a valid thing to do, or should I keep trying to find "the problem".

@phkahler
Copy link
Member Author

phkahler commented Jun 1, 2019

@jwesthues I've got a problem creating arcs for the new Revolve group. They seem to work correctly on a new revolve but if you drag the end so the angle becomes negative the arcs go the wrong way around. For example, drag it so it comes out of the screen instead of going in. The direction of the arcs can be reversed by swapping the constants REMAP_LATHE_START and REMAP_LATHE_END in the Revolve section of Group::Generate but that does not help (it always swaps them). The sign of the angle is not known at the time Generate is called so this is not the right place to fix it. Do you have any suggestions how to resolve this?

This is current state of my geometry branch, and has been rebased to master today.

@whitequark this is in really good shape with the last couple commits except for this arc bug.

@whitequark
Copy link
Contributor

whitequark commented Jun 8, 2019

@phkahler OK, now that Revolve is merged, let's discuss the next steps. My understanding is there are generally two things to do:

  1. Add Helix by reusing code for Revolve,
  2. Add chirality for arcs.

I can't do (1) personally, but I can look into (2) if you want. Chirality is something I think is a requirement for #77 to work out long term, so it's definitely an important goal.

@phkahler
Copy link
Member Author

phkahler commented Jun 9, 2019

@whitequark I'm looking into the remapping and solver stuff. The code to create a helical SShell is all there, it's creating the entities with the correct relationships and degrees of freedom that's still new to me. I have a simpler experiment to try first to get a feel for how that's all put together.

@phkahler
Copy link
Member Author

phkahler commented Jun 9, 2019

@jwesthues Regarding your comment above - on April 10 and continuing to use extrude as an example.. I get that the normal is turned into 3 parameters as shown in your comment. I know where those parameters for groups get created. For extrude those 3 parameters are combined into an expression vector, which is used to create 2 constrain equations that keep that vector orthogonal to the sketch plane. I know where all that happens. I can not find where that vector (or those parameters treated as a vector) is added to the points of an entity to calculate the points of a copy of that entity. Can you tell me exactly where that addition takes place in the code?

I need to know in order to create new relations between entities and their copies.

@jwesthues
Copy link
Member

jwesthues commented Jun 9, 2019

solvespace/src/group.cpp

Lines 461 to 469 in 5df53fc

CopyEntity(entity, SK.GetEntity(he), ai, REMAP_BOTTOM,
h.param(0), h.param(1), h.param(2),
NO_PARAM, NO_PARAM, NO_PARAM, NO_PARAM,
CopyAs::N_TRANS);
CopyEntity(entity, SK.GetEntity(he), af, REMAP_TOP,
h.param(0), h.param(1), h.param(2),
NO_PARAM, NO_PARAM, NO_PARAM, NO_PARAM,
CopyAs::N_TRANS);
MakeExtrusionLines(entity, he);

solvespace/src/group.cpp

Lines 897 to 900 in 5df53fc

en.type = Entity::Type::POINT_N_TRANS;
en.param[0] = dx;
en.param[1] = dy;
en.param[2] = dz;

solvespace/src/entity.cpp

Lines 486 to 490 in 5df53fc

case Type::POINT_N_TRANS: {
Vector trans = Vector::From(param[0], param[1], param[2]);
p = numPoint.Plus(trans.ScaledBy(timesApplied));
break;
}

@phkahler
Copy link
Member Author

phkahler commented Jun 9, 2019

Thanks, I had missed that. But that is doing a numeric computation. I was expecting there to be an expression vector computation of the location of the copied point. Otherwise when you drag a point, how are the constraints enforced?

@jwesthues
Copy link
Member

jwesthues commented Jun 9, 2019

PointGetNum() and PointGetExprs() are usually analogous, and that's the case here:

solvespace/src/entity.cpp

Lines 534 to 539 in 5df53fc

case Type::POINT_N_TRANS: {
ExprVector orig = ExprVector::From(numPoint);
ExprVector trans = ExprVector::From(param[0], param[1], param[2]);
r = orig.Plus(trans.ScaledBy(Expr::From(timesApplied)));
break;
}

numPoint was determined when an earlier group was solved--so only the transformation needs to be made symbolic, not the base point.

@phkahler
Copy link
Member Author

phkahler commented Jun 10, 2019

Thanks @jwesthues. Also want to bring in @whitequark again. I've been experimenting on a new branch "frustum" which hijacks the Extrude command to produce a frustum group. This produces an extrusion where the new end is a scaled version of the sketch end, and can be moved in any direction (skew is possible). This is my experiment with new entity copy methods - I need to understand to finish Helix. Some questions:

  1. Is this (frustum) a desired feature?

  2. It requires selecting a point on the sketch which acts like an origin - everything else is scaled relative to that point prior to extrusion, so that point would be projected perpendicular to the sketch. However there are no constraints (I know how to add them) on the extrusion vector so this point is kind of meaningless but doing the scaling requires a point on the sketch. Is there some reasonable origin point on the sketch to use so no selection would be required? Would that be better?

  3. I created a parameter for the scale, but it seems to be treated as a constant. It should be possilbe to specify equal length between corresponding entities on the top and bottom and make them the same but the background turns red and no change in length happens. Maybe it's a stretch, but what might be wrong there? The scale is set to 0.5 by default but I thought it was still a free parameter.

@whitequark
Copy link
Contributor

whitequark commented Jun 11, 2019

  • Is this (frustum) a desired feature?

Yes, absolutely. It has been requested before, although the issue seems to have been lost during repo migration to solvespace/solvespace.

@Evil-Spirit
Copy link
Collaborator

Evil-Spirit commented Jun 11, 2019

@phkahler You should consider what scale - isn't thing that can do real frustums. Scale works good only for rectangles, to make real frusutms you should create offset curves instead of scaling.

@phkahler
Copy link
Member Author

phkahler commented Jun 11, 2019

@Evil-Spirit Scaling it is exactly what's needed for a frustum. It is definitely not what you want for creating constant draft angles, nor is it what you do to the outline to get the control points for fillets or chamfers on the end of an extrusion (those need to be fixed distance offsets, which is a far future thing I'm thinking about). But a constant scaling does produce a mathematically correct frustum and is very useful in a few cases.

Now I've found that the fixed point I chose to scale relative to has a problem. As the underlying surface moves (build a frustum on the end of a revolve and drag the revolve around) the point apparently doesn't move - it's a numeric copy made at the time the frustum is created. But the entities and shell become separated, so maybe one of them is right? Anyway, more to understand... @Evil-Spirit are you familiar with some of this area?

@whitequark
Copy link
Contributor

whitequark commented Jun 11, 2019

Scaling it is exactly what's needed for a frustum. It is definitely not what you want for creating constant draft angles, nor is it what you do to the outline to get the control points for fillets or chamfers on the end of an extrusion (those need to be fixed distance offsets, which is a far future thing I'm thinking about).

Ah, sorry, I misunderstood then. The issue I was talking about used "frustum" to mean "a solid with constant draft angles". I am not sure how useful a mathematically correct frustum would be or whether anyone has asked for it—not to say that it isn't but I haven't given it a lot of thought.

@Evil-Spirit
Copy link
Collaborator

Evil-Spirit commented Jun 11, 2019

@phkahler

Now I've found that the fixed point I chose to scale relative to has a problem.

Just let user to choose this point like for lathe/revolve.

@phkahler
Copy link
Member Author

phkahler commented Jun 15, 2019

Now I've got a helix branch. For now it creates a Helix group in place of the Revolve group (this is temporary, it needs a menu entry etc...). The entities are copied correctly at the ends of the helical surface. To do this I extended the fixed length param[7] array to 8 entries. I'm not sure what that does to file IO but a quick check can load an old file. @whitequark ??

The helical surface can be dragged, but it is very awkward to do so. The reason is how things are handled in EntityBase::PointForceTo() which is where parameters are changed in order to get dragging to follow the mouse. It's not clear yet (I'm tired) how exactly this works, or how it should handle rotation and translation at the same time. I'm a little disappointed that the solver wasn't used to figure out how to minimize the distance from a dragged point to the mouse pointer. Constraints do work BTW and are probably more useful than dragging.

There is currently no text window defined for this group type. We need to copy the one from Revolve and add those checkboxes and parameter value input boxes discussed here previously - as in NoteCAD. If anyone can help, param[3] is the angle and param[7] is axial distance.

You can use an axis orthogonal to a sketch to create a twisted extrusion but there's a normal / inside-out bug depending which way it twists:
Twisted

Because this is deep in the bowels of SolveSpace I guess I'm just posting an update rather than asking for help but it sure would be nice ;-) We shall have this feature one way or another.

@phkahler
Copy link
Member Author

phkahler commented Jun 17, 2019

Since a point has to be selected as the center of rotation, that point is copied along the rotation axis and is very convenient for setting the length. Once axially constrained the rotation is easy to drag. Still thinking a textbox to enter an angle would be good. Menu entry and text window are in now. Booleans are very buggy - Have not been able to do a difference even once. Had similar issues with Revolve so I'm not sure if that's a Helix issue or just normal SS boolean issues. This is all in my helix branch for those following along. Constructive feedback welcome. Still a bit to go...
fan

@whitequark
Copy link
Contributor

whitequark commented Jun 17, 2019

Incredible demo--I now realize how many opportunities this will open besides threads. I am a bit busy with other things at the moment but I will check this out at the earliest opportunity.

@phkahler
Copy link
Member Author

phkahler commented Jun 18, 2019

Looking for help with an entity bug. In the fan model shown above (and attached here) there are arcs in the base sketch - on the blade tips and forming the hub in the centre. Once extruded using Helix, things are fine until you twist it by dragging a point near the outside. As the twist increases, all arcs are shown with increased diameter - the construction lines as well as the white arcs. By making it a double sided extrusion you can see the originals are unaffected. The weird thing is the points don't do this. Also if you replace arcs with line segments or even splines they don't do this. It's only a problem with arcs. I've gone through the code for creating the copies of entities and made sure the quaternions are valid (built from a unit vector and angle) but this just keeps happening and I have no idea why.

My suspicion was in EntityBase::PointForceTo(Vector p) which has a new case for helical objects. But all that does is set the angle and extrusion length. Buggy as it may be, those two values are used for all entities as well as the mesh and everything is fine except arcs, which I don't treat any differently.

Any ideas what may be going on here? Anyone?

Note: to open the attached file you'll need to build from my helix branch.
fan.zip

@phkahler phkahler mentioned this issue Jun 19, 2019
@phkahler
Copy link
Member Author

phkahler commented Jun 19, 2019

I squashed all my helix commits locally but when I push to origin is says everything up to date. Feel free to squash it down to a single commit. AFAICT this is good enough to have people use it and find any issues. At this point I see no need for editing length in the text window - it's easy to set in the drawing which is more in line with the SolveSpace way. Angle can be set by twisting and constraining between skew lines on the ends of the extrusion. We can make changes prior to 3.0 if needed.

Edit: and all previously mentioned bugs have been fixed.

@jwesthues @whitequark This has been entirely cool to work on. Thank you for your help!

@phkahler
Copy link
Member Author

phkahler commented Jun 25, 2019

@jwesthues one final question. In Group::Generate I want to create line (extrude) entities along the axis. I've done that, but it doesn't seem to work for the selected point if it is not in the sketch. In other words, make something, then create a new workplane, then helix around the origin without any sketch elements touching the origin. I don't get an extrusion line from that point. Code is here:
`
case Type::HELIX: {
Vector axis_pos = SK.GetEntity(predef.origin)->PointGetNum();
Vector axis_dir = SK.GetEntity(predef.entityB)->VectorGetNum();

        // The center of rotation
        AddParam(param, h.param(0), axis_pos.x);
        AddParam(param, h.param(1), axis_pos.y);
        AddParam(param, h.param(2), axis_pos.z);
        // The rotation quaternion
        AddParam(param, h.param(3), 30 * PI / 180);
        AddParam(param, h.param(4), axis_dir.x);
        AddParam(param, h.param(5), axis_dir.y);
        AddParam(param, h.param(6), axis_dir.z);
        // distance to translate along the rotation axis
        AddParam(param, h.param(7), 20);

        int ai = 1;

        for(i = 0; i < entity->n; i++) {
            Entity *e = &(entity->elem[i]);
            if(e->group.v != opA.v)
                continue;

            e->CalculateNumerical(/*forExport=*/false);

            CopyEntity(entity, SK.GetEntity(predef.origin), 0, ai, NO_PARAM, NO_PARAM,
                       NO_PARAM, NO_PARAM, NO_PARAM, NO_PARAM, NO_PARAM, NO_PARAM, CopyAs::NUMERIC);

            for(a = 0; a < 2; a++) {
                e->CalculateNumerical(false);
	    CopyEntity(entity, e, a * 2 - (subtype == Subtype::ONE_SIDED ? 0 : 1),
                       (a == 1) ? REMAP_LATHE_END : REMAP_LATHE_START, h.param(0),
                       h.param(1), h.param(2), h.param(3), h.param(4), h.param(5),
                       h.param(6), h.param(7), CopyAs::N_ROT_AXIS_TRANS);
            }
            // For point entities on the axis, create a construction line
            if(e->IsPoint()) {
                Vector check = e->PointGetNum().Minus(axis_pos).Cross(axis_dir);
                if (check.Dot(check) < LENGTH_EPS) {
                    Entity *ep = SK.GetEntity(e->h);
                    Entity en = {};
                    // A point gets extruded to form a line segment
                    en.point[0] = Remap(ep->h, REMAP_LATHE_START);
                    en.point[1] = Remap(ep->h, REMAP_LATHE_END);
                    en.group = h;
                    en.construction = ep->construction;
                    en.style = ep->style;
                    en.h = Remap(ep->h, REMAP_PT_TO_LINE);
                    en.type = Entity::Type::LINE_SEGMENT;
                    entity->Add(&en);
                }
	}
            ai++;
        }
        return;
    }

`
The selected point is the one I want most. The others are more nice to haves.

@whitequark
Copy link
Contributor

whitequark commented Aug 20, 2019

@phkahler Do we have any next steps here / should this be kept open?

@phkahler
Copy link
Member Author

phkahler commented Aug 20, 2019

Do we have any next steps here / should this be kept open?

I'd close it. The feature is done and usable.

@ruevs
Copy link
Member

ruevs commented May 10, 2020

@phkahler
Copy link
Member Author

phkahler commented May 10, 2020

@phkahler I think you'll enjoy this from Aug 22, 2008 :-)

That was interesting. The lathe operation had indication that it wanted to be what is now Revolve. Until very recently it was making 2 copies of the sketch entities LATHE_START and LATHE_END one of which was removed because it's redundant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants