Enable face dragging for translate groups. Add the necessary handles … #1657
Conversation
|
I'd like to merge this prior to 3.2 so people will start creating files with point handles in the face entities. That is a prerequisite for dragging linked objects by their faces - which works fine but is currently disabled to prevent crashes. Oh, maybe we can check for a valid handle and only drag if one is present. |
|
Looks like I need to update the files in 4 tests because they are no longer identicle. This adds a point handle to some faces. |
|
@phkahler to summarize your work on dragging faces so far:
So back to this PR:
Overall I think this is not ready. |
Branch deleted, thanks!
When we drag a line, we add the end points to the drag list. It makes sense for dragging faces to add their point to the drag list. The problem is that when linking, all entites are copied with copy type N_ROT_TRANS, which creates a new entity using only numeric values (no free parameters). You can move them around because it's possible to get expressions for the points which are numeric values modified by the group parameters. Some face types just return the expressions for their underlying point which is probably what they all should do. The problem: Some faces never set the entity handle for their point, which is clearly an oversite. This was never a problem because nothing ever tried to access that point by its handle - like adding it to the drag list.
2d sketches do not have a "face" entity. Faces are defined by a plane and can be selected by the corresponding triangles in the mesh - yes, every triangle has an entity handle!. Fun fact. When I wrote the very first Helix code and posted this picture the upper surface would highlight with mouse-over, but it would crash if clicked because there was no actual face entity. The triangles were getting properly tagged because of some code I copied from somewhere. That was a big surprise to me because it's a twisted surface - not flat at all.
My poor description. Trying to drag old linked files will not crash, it just fails to drag. This is because I added the check for a valid handle before using the face-associated point. It does not "work", it just doesn't crash.
Faces tend to light up often so it makes sense to turn them off. You are free to leave them turned off ;-) But when they are turned on it makes sense that they be useful and even draggable. I think this is really important for linked sketches where the main thing we do is position them relative to other parts in an assembly.
I think it is, but maybe I should update the commit message claiming linking old files work. Edit: Oh it's correct already. |
|
@ruevs Ping!! And Happy New Year!! |
src/group.cpp
Outdated
| en.param[2] = h.param(2); | ||
| en.numPoint = a; | ||
| // might use the original point, not the remapped one - but surface face uses TOP | ||
| // using the original point might allow face dragging the sides unconstrained extrusions |
There was a problem hiding this comment.
This is the part that I found "suspect" as well. I do not understand the remapping well.
@jwesthues what do you think? (Happy New Year!).
There was a problem hiding this comment.
Happy New Year! Looking at EntityBase::FaceGetPointExprs(), I see that FACE_NORMAL_PT and FACE_ROT_NORMAL_PT both simply return point[0], while the others calculate the point from numPoint. I think I did that to simplify the generation of the others. I believe suitable point entities do always exist in a real sketch, but I probably thought it would be simpler to repeat the transform calculation than to find that suitably transformed point.
This change seems to keep that behavior in FaceGetPointExprs(), but sometimes additionally set point[0] to a suitable point to drag. So point[0] provides both definition and dragging for FACE_NORMAL_PT and FACE_ROT_NORMAL_PT, but only dragging for others. That's inconsistent, though the use of those members is generally inconsistent between different entity types so that seems fine to me. It perhaps deserves a comment explicitly noting that inconsistent overloading.
There was a problem hiding this comment.
@jwesthues In the future we could also change the other face types to return the expressions from the points once the handles are included and drop the extra code in FaceGetPointExpers(). Would that make things more consistent? I'm not ready to do that on the eve of v3.2.
There was a problem hiding this comment.
@phkahler I think you'd have to keep the old codepath to support linked files saved by earlier versions. So I definitely wouldn't change FaceGetPointXXX() now, and probably not ever.
There was a problem hiding this comment.
This is the part that I found "suspect" as well. I do not understand the remapping well.
@ruevs I thought I finally understood remapping. Went to verify a few details, and now... It does what I thought, but im not sure how exactly. There's a step that seems to be missing, and the mechanism is IMHO rather broken. But it seems to work anyway. More on that later.
There was a problem hiding this comment.
@ruevs I thought I finally understood remapping. Went to verify a few details, and now... It does what I thought, but im not sure how exactly. There's a step that seems to be missing, and the mechanism is IMHO rather broken. But it seems to work anyway. More on that later.
I still don't understand it well enough because I have looked through this part of the code only a few times, superficially. An I've never tried to modify it or debug it to understand better. I should...
But the discussion between you and Jonathan above helped me a lot.
src/group.cpp
Outdated
| // might use the original point, not the remapped one - but surface face uses TOP | ||
| // using the original point might allow face dragging the sides unconstrained extrusions | ||
| en.point[0] = Remap(ep->point[0], REMAP_TOP); | ||
| // en.point[0] = ep->point[0]; |
There was a problem hiding this comment.
At any rate we should decide what the right thing to do is, explain it in a comment and remove the other option.
| } | ||
| en.numPoint = (ep->actPoint).ScaledBy(scale); | ||
| en.numNormal = (ep->actNormal).ScaledBy(scale); | ||
| en.point[0] = Remap(ep->point[0], remap); |
…for face dragging linked sketches. In mouse.cpp StartDraggingByEntity needs some face types uncommented to enable different face dragging options. All rotational face drags are disabled because the points used for those faces are on-axis(I think), this needs to be changed for those faces to be more useful.
|
@ruevs squashed. Some comments cleaned/removed. And I did opt to use the other point for extrusion sides - no idea why it won't drag a rectangle from a previous group by using that, but it will never do it with a remapped point. |
… the original line.
3 participants
…for face dragging linked sketches, but don't enable that feature because existing sketches will crash when dragged. In mouse.cpp StartDraggingByEntity needs some face types uncommented to enable different face dragging options. All rotational movements are disabled because the points used for those faces are on-axis(I think), this needs to be changed for those to be more useful.