-
-
Notifications
You must be signed in to change notification settings - Fork 118
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
Only define geometry for Surface
, Curve
, and Vertex
#2290
Comments
I've been working on this for the past few days. As I explained in the issue description, there are basically two parts to this issue:
I've started with item 1, as those local definitions of the vertex position would need to reference the curve that the definition is local to, so it seemed to make more sense to first get everything in order over there. I have a local branch where I've started to do this, but I'm not fully convinced that I've found the right approach yet. What has come out of that so far, are mostly self-contained cleanups and refactorings (see list of pull requests above). One thing that I hoped to gain from (which I've outlined in the issue description) is more clarity, by making the currently implicit redundant definitions more explicit, bringing the mess that we have to the forefront. I can say with certainty, that this has worked out well (at least in my local branch). Making a mess seems to be one of the things I'm particularly good at 😁 One aspect of making the relationships we already have more explicit, is that the new All of this is a bit of a mess, as I have to pass But either way, it got me thinking: Can the handling of surfaces be simplified, by having a surface that, unlike most surfaces, doesn't represent a surface in 3D space, but represents the whole 2D space? This is possible now. With the geometry layer, assigning geometry to a surface has become optional, and we could just define that special surface and not assign any geometry to it. If we organized it like that, then we would no longer need a special "no surface" case in code or data structures, which sounds like a nice simplification. I'll think about that over the coming long weekend. If I decide it's a good idea, I'll probably do that first, before continuing here, as it makes what I'm working on here less complicated. |
A feature I am considering asking for in the future (but am holding off because I don't need it yet, you seem busy, and I'm not even sure if it's possible) is to take slices of solids, as in cut the thing in half with a plane and get a 2D shape of where the solid intersects that plane. This would be extremely useful for GCode generation and would have big advantages over current slicer tools since curves would be represented in the 2D slice with curves, something slicer programs for 3D printers are currently incapable of (they approximate with poly-lines). It would also be useful for CNC milling complicated features. Anyway, I bring that up because I think a purely 2D space would be good for representing the output of such a function. |
That definitely sounds possible (and reasonable), but it's another one of those things that need a universal intermediate representation (#2118), before it becomes practical. Note that the current plan for that is to use polylines as the intermediate representation. But as the issue notes, the original curves are still around, and you can use them to get more resolution whenever you need it. |
I gave it a read and I'm not sure if I should put my feedback there or here. The approach seems reasonable and I like your "planning only gets you so far" approach. I've seen people plan for months and then discover the idea won't work a weeks into the actual project. Prototyping is a really important process, and I consider Fornjot to be in it's prototyping era. Having a tag of where the polyline came from seems like a reasonable compromise. Knowing where it came from has some additional benefits for me. I want to be able to tag walls with properties to influence gcode generation with things like "when it comes to tolerance, bias toward the inner or outer side of this surface". So many times I found myself wishing I could communicate that to my slicer. |
I'm creating plans to add gcode generation to Command CAD soon, for 2D objects only. I can generate that right from the sketches. Since sketches currently are very limited, I'm thinking about making a temporary solution where I store curves myself and convert them to polylines for Fornjot, similar to what FJ-Text currently does. How well do you think that will bridge into your eventual long term plan? |
Thank you for the feedback, @IamTheCarl!
Whatever ends up happening with curves within Fornjot, I expect that generating your own curves as multi-half-edge polylines will remain a viable approach going forward. So once more powerful curves become available, you can decide to port your code to those, or keep doing what you're doing. So I seen no reason why you shouldn't move ahead with your approach. Once my geometry work progresses to the point where powerful curves and polylines as intermediate representation are available, we'll see if that suit your needs. And if it doesn't, what needs to improve. Me making progress with the design, and you making progress with a use case for that design, is probably the best way to eventually land on something that works well. |
It's been a while since I posted an update here. I've been on vacation in the meantime, but now I'm back, and as you can see from the list of pull requests above this comment, getting back into it! I've implemented the surface representing 2D space that I talked about in my previous update. So far, that has worked out well. I've also managed to land the first pull requests to define the curve geometry. Making sure all places that need to define curve geometry do so, and all places that use curve geometry read it from the right place, is an incremental process that will still take some time to finish. Once that has happened, I should be able to remove the |
As of #2391, all curve geometry has been removed from This is an important step forward, for the reasons explained in the issue description above. Most importantly, there's now a clearer path towards making curve geometry no longer redundant (which will also require #2118). On the negative side though, this has made a lot of code more messy; although I'd argue that it has only brought more of the preexisting conceptual mess to the surface. This is a transitionary phase. Things have gotten a bit worse (and will get worse still, before this issue is finished), on the path to ultimately making things much better. Speaking of that. As of now, all surface geometry is defined on And this is what I'll start working on now. I'm not sure how difficult this is going to be. I hope less difficult then what I've done here so far, but how knows. I'm sure there are many more problems waiting to be uncovered. |
And I'm finally done! The last bit, migrating the remaining half-edge geometry to vertices, didn't hold a lot of surprises. Just the usual: Migrating a little bit, hitting an issue, tediously debugging that issue, fixing it, continue. This means the objective of this issue, defining geometry only in terms of surfaces, curves, and vertices, has been achieved. What this has also done, as I have alluded to multiple times, is make a lot of code messier. While I was a bit surprised by the degree to which that has happened, I'm not surprised that it has happened. What this has done, is put the redundancy that was previously neatly hidden in the object graph into the foreground, where it's visible and can be dealt with more easily. It's fine for now. Most importantly, this paves the way for #2118, which I'm finally ready to start working on now. If this works out, it will become practical to build better tools around converting geometry between different spaces, which will make it possible to just remove the redundancy, have a single canonical geometry definition for each surface/curve/vertex. This should get us out of this weird transitional phase and ideally make everything related to geometry neat and tidy. At least that's the plan. And now I can finally start working on that! |
Current Situation
At this point, geometry is defined for
Surface
andHalfEdge
. But the geometry defined forHalfEdge
doesn't actually belong there:path
defines the geometry of theCurve
that theHalfEdge
is on.boundary
defines the positions of theVertex
instances that bound theHalfEdge
on thatCurve
.Organizing it like this doesn't make much sense, but was necessary until recently. Geometry in Fornjot is always defined in the most local way possible. For example, vertices are defined in terms of 1-dimensional curve coordinates. Curves are defined in terms 2-dimensional surface coordinates. This is the most general way to define them, as it's always possible to go up to a higher dimension; but so far, we lack the tools to project from a higher dimension into a lower one. (This is something that #2118 will help with.)
Vertices can exist on multiple curves and curves can exist on multiple surfaces. Defining their geometry locally means that it is defined redundantly. Storing these redundant definitions within
Vertex
andCurve
would have significantly complicated both those objects themselves and the object graph as a whole.HalfEdge
is already local to a a specific curve and surface, and happens to reference bothCurve
andVertex
. Defining both of their local geometries there, was a convenient way to solve this problem.New Possibilities
Now that geometry is no longer defined as part of the topological object graph (#2116), the old restrictions no longer apply. It should not be a problem to tie the multiple redundant definitions directly to
Vertex
andCurve
respectively.Doing this should provide a number of benefits:
Future Improvements
As I alluded to in the previous section, I believe there is a path to improving the situation by making geometry definitions no longer redundant. Once #2118 is implemented, implementing functionality like projecting into a local coordinate system becomes practical without introducing a maintenance burden. Then using a global definition in a local context, or translating a local definition into another local context, becomes possible, allowing users to define geometry in whatever form is most convenient for their model.
Next Steps
I am going to start working on this right away, as I think its best to take care of this before #2118. The improved clarity should benefit any work that comes after. And doing this cleanup afterwards would likely be significantly more work.
The text was updated successfully, but these errors were encountered: