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

Simplify use of local forms #691

Closed
hannobraun opened this issue Jun 13, 2022 · 2 comments · Fixed by #761
Closed

Simplify use of local forms #691

hannobraun opened this issue Jun 13, 2022 · 2 comments · Fixed by #761
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

Once upon a time, the Fornjot kernel only dealt in 3D coordinates, converting them into other forms (like local curve or surface coordinates) only where necessary. This changed for various reasons, and now all object types that reference other objects, keep a local form of the referenced object, where appropriate.

While I still believe that this was a necessary change, it certainly has made a lot of the code much more complicated than it needs to be. And this wasn't unexpected, as I was still exploring the design space. This first over-complicated design taught me a lot, and after using and reflecting on it for a while, I think I've come up with something simpler.

First, a rough overview over the current design, to put the following improvements into context:

  • Many objects (for example vertices) have local and canonical forms. The canonical form is the 3D form of the object, which is applicable everywhere, while the local form exists in a specific context (for example, the 1D curve coordinates of the vertex on a specific curve).
  • As a consequence simple references to the canonical form of an object (e.g. Handle<Vertex>) have been replaced with that same reference, side-by-side with the local form of the same object (e.g. LocalForm<Point<1>, Vertex>).
  • Some object types have become generic, as to be able to represent both the local and canonical forms. A good example is Edge, which now exists as Edge<2> and Edge<3>.
  • Only the canonical form as an object exists within Shape. The local form is always stored together with the reference to the canonical form, in the referring object. The LocalForm struct is used to make this a bit easier.

I believe that this design can be simplified in the following ways:

  • Make local forms their own kinds of objects. Store them in Shape, where they reference their own canonical form (and possibly their local context, i.e. the curve or surface) using a regular Handle.
  • Dump LocalForm again, at least in its current form. Replace every use of it with a regular Handle to the local form. This is not any less general, as the local form references its own canonical form, meaning you can always retrieve the canonical form with one extra step.
  • Following the insight that referencing a local form is in fact more general than referencing a canonical form (since you get access to both forms), make Edge and Cycle non-generic again. Instead of having an Edge<2> that references local forms, and an Edge<3> that references canonical forms, just have a simple/non-generic Edge, that references the local form.

I believe that these steps have the potential to make things much simpler again. I am going to start working on this immediately, as realizing those simplifications will make everything else I'm working on much easier.

@hannobraun hannobraun added type: development Work to ease development or maintenance, without direct effect on features or bugs topic: core Issues relating to core geometry, operations, algorithms labels Jun 13, 2022
@hannobraun hannobraun self-assigned this Jun 13, 2022
@hannobraun
Copy link
Owner Author

I've hit a problem while working on this, which I've written about in #695. This issue is now blocked, until #695 is resolved.

@hannobraun hannobraun added the status: blocked Issue or pull request is blocked by another issue or pull request, or some outside circumstance label Jun 15, 2022
@hannobraun
Copy link
Owner Author

This is probably no longer blocked on #695. See my comment there. I'm back to working on this issue.

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.

1 participant