Validate object ownership

[hannobraun]

There are some objects in the graph that are essentially owned by the objects that reference them, meaning that there's typically exactly one object that references them. I believe that for these objects, it's always a mistake to reference them from multiple other objects, within the context of a valid Sketch or Solid.

I believe that the following should be true, for a valid Sketch or Solid:

• [ ] Each HalfEdge is referenced by exactly one Cycle.
• [ ] Each Cycle is referenced by exactly one Region.
• [ ] Each Region is referenced by exactly one Sketch or Face. (edit: not necessary for Sketch; see https://github.com/hannobraun/fornjot/pull/2144#pullrequestreview-1817856036)
• [ ] Each Face is referenced by exactly one Shell.

I believe that these checks won't be hard to implement, so this might be suitable for new contributors:

• Validation checks for Sketch and Solid need to be placed in validate/sketch.rs and validate/solid.rs respectively.
• Each item in the list above should go into a separate check, for clarity. You can check out the other files in the validate module for examples on how these checks are typically structured.
• If you're up for it, it would be ideal if you included a unit test with each check. There are many examples of those in the various validate sub-modules.
• The APIs of Sketch and Solid themselves are quite straight-forward. You can easily access the objects they reference, use those objects to access further objects, and so forth.
• For a start, it would be enough to just iterate over all objects of the respective type and remember which objects they reference. We can leave smarter and better performing approaches for future work.

If you're interested in working on this, please don't hesitate to ask any questions you might have!

[nathan-folsom]

Hey @hannobraun, I just did an initial pass at implementing the check for HalfEdges in #2144. Am I on the right track?

Also a question about the errors themselves: Are there any details that would be useful to surface about the circumstances of the failed validation, e.g. number of times an object was references, what it was referenced by? Or is it enough to just report that an object was referenced more than once?

Interesting project by the way! I've done a bit of CAD over the years (Solidworks and Onshape), but I've never seen how it works under the hood so I'm still putting the pieces together.

[hannobraun]

Hey @nathan-folsom, thank you for your interest! I don't have time for a full review right now, but I took a quick look over the pull request, and what I saw looks good. You're definitely on the right track!

Also a question about the errors themselves: Are there any details that would be useful to surface about the circumstances of the failed validation, e.g. number of times an object was references, what it was referenced by? Or is it enough to just report that an object was referenced more than once?

I think it's probably best to have the object itself, as well as all objects that reference it in the error message. That should be the right amount of information to figure out what's wrong, if someone runs into that error.

But generally, the important thing is that the validation check exists. If I run into one while working on a new feature, and there's zero information for example, it's relatively straight-forward to go into the code and add the information that I need. At least I know that something is wrong.

Interesting project by the way! I've done a bit of CAD over the years (Solidworks and Onshape), but I've never seen how it works under the hood so I'm still putting the pieces together.

Thanks! If you have any questions, always feel free to ask. I'm very interested in improving the documentation, both for end users and developers. If you run into something that you don't understand, it's probably under-documented. Feel free to just open an issue in such a case!

[nathan-folsom]

Ok awesome thanks for the confirmation. No full review necessary, I'll ping you once I've implemented everything from this issue and we can go from there.

For now I think I just need to read more code to understand the bigger picture, but I will let you know if I come across anything. The documentation has been pretty good so far!

[nathan-folsom]

@hannobraun I believe everything is covered now, but it's still a very naive implementation with lots of duplicated code. I'm going on vacation for the next week and won't be doing any coding, but I'm happy to do cleanup and write some abstractions after that.

[hannobraun]

Thank you, @nathan-folsom! I'll have a look when I get a chance. I'm also on vacation, for the rest of the year. Although in my case that means coding every day, but schedule and subject matter follow my whims :smile:

The documentation has been pretty good so far!

Glad to hear that! Be warned though, it's been quite a while since I did a full top-to-bottom review. Quite possible that some of the documentation is simply no longer true, even where it exists and looks reasonable.

I believe everything is covered now, but it's still a very naive implementation with lots of duplicated code.

I still haven't done a full review, but generally speaking, this is fine. I follow the principle that it's always okay to make a mess, as long as you never build on top of an existing one (then it's cleanup time). Cleanup is always appreciated, of course, but I generally don't require code to be perfect. Just that it's an improvement over what we already have.

[nathan-folsom]

Ok, I like the sound of that policy. I’ll do a little bit of cleanup once I’m back, but won’t worry about it too much. Hope you’re having a good vacation!On Dec 18, 2023, at 6:28 AM, Hanno Braun @.***> wrote: Thank you, @nathan-folsom! I'll have a look when I get a chance. I'm also on vacation, for the rest of the year. Although in my case that means coding every day, but schedule and subject matter follow my whims 😄

The documentation has been pretty good so far!

Glad to hear that! Be warned though, it's been quite a while since I did a full top-to-bottom review. Quite possible that some of the documentation is simply no longer true, even where it exists and looks reasonable.

I believe everything is covered now, but it's still a very naive implementation with lots of duplicated code.

I still haven't done a full review, but generally speaking, this is fine. I follow the principle that it's always okay to make a mess, as long as you never build on top of an existing one (then it's cleanup time). Cleanup is always appreciated, of course, but I generally don't require code to be perfect. Just that it's an improvement over what we already have.

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

[hannobraun]

Addressed in https://github.com/hannobraun/fornjot/pull/2144.