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

Only define geometry for Surface, Curve, and Vertex #2290

Closed
hannobraun opened this issue Mar 25, 2024 · 12 comments · Fixed by #2419
Closed

Only define geometry for Surface, Curve, and Vertex #2290

hannobraun opened this issue Mar 25, 2024 · 12 comments · Fixed by #2419
Assignees
Labels
topic: core Issues relating to core geometry, operations, algorithms type: development Work to ease development or maintenance, without direct effect on features or bugs

Comments

@hannobraun
Copy link
Owner

Current Situation

At this point, geometry is defined for Surface and HalfEdge. But the geometry defined for HalfEdge doesn't actually belong there:

  • path defines the geometry of the Curve that the HalfEdge is on.
  • boundary defines the positions of the Vertex instances that bound the HalfEdge on that Curve.

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 and Curve 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 both Curve and Vertex. 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 and Curve respectively.

Doing this should provide a number of benefits:

  • It will put the geometric definitions into more logical places, meaning the whole system will be easier to explain and understand.
  • Putting those multiple redundant definitions together will improve clarity. Yes, it will still be confusing, but the situation is already confusing right now, in addition to being obfuscated.
  • It will provide a clearer path towards improving the situation by making geometry definitions no longer redundant.

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.

@hannobraun
Copy link
Owner Author

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:

  1. Move the SurfacePath that defines curve geometry from HalfEdgeGeom to a new CurveGeom and integrate that into the geometry layer.
  2. Move the CurveBoundary that defines the positions of vertices from HalfEdgeGeom to a new VertexGeom.

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 CurveGeom references a surface for each of its local definitions. And this reference must even be optional, as the curve might be defined as part of a sketch, which is a pure 2D construct, and does not have a surface to locate it in 3D.

All of this is a bit of a mess, as I have to pass Option<Handle<Surface>> to a lot of places where something like that was previously not required. Hopefully that mess is just a temporary artifact of the ongoing transition. Once the new structure is in place, it will hopefully be possible to refactor the affected APIs in a way that makes more sense with the changed circumstances.

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.

@IamTheCarl
Copy link
Contributor

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.

@hannobraun
Copy link
Owner Author

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.

@IamTheCarl
Copy link
Contributor

IamTheCarl commented Mar 30, 2024

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.

@IamTheCarl
Copy link
Contributor

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?

@hannobraun
Copy link
Owner Author

hannobraun commented Mar 31, 2024

Thank you for the feedback, @IamTheCarl!

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?

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.

@hannobraun
Copy link
Owner Author

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 SurfacePath from HalfEdgeGeom, finishing that part of this issue. After that, I can focus on migrating the rest of HalfEdgeGeom into a new vertex geometry definition.

@hannobraun
Copy link
Owner Author

As of #2391, all curve geometry has been removed from HalfEdgeGeom, meaning curve geometry is not defined exclusively on Curve. This change was literally months in the making!

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 Surface objects, all curve geometry is defined on Curve objects, and only vertex geometry is still defined on HalfEdge. Addressing that last item is what remains to be done, before I can close this issue.

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.

@hannobraun
Copy link
Owner Author

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: core Issues relating to core geometry, operations, algorithms type: development Work to ease development or maintenance, without direct effect on features or bugs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants