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

Vertex validation #242

Closed
hannobraun opened this issue Feb 23, 2022 · 3 comments · Fixed by #278
Closed

Vertex validation #242

hannobraun opened this issue Feb 23, 2022 · 3 comments · Fixed by #278
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

Due to floating-point accuracy issues, it is possible get slightly different floating point values, even though those should be the same. This is a pervasive problem. Instead of relying on workarounds, which would be required in every piece of code that touches floating point values, even indirectly through points, vectors, vertices, etc, I've opted to address this problem at an architectural level. See #78 for some context, or the documentation on vertex uniqueness.

I'm reasonably happy with that approach so far, but it's time to take the next step: Validating that all vertices are actually unique. Here's how I imagine that should happen:

  • Vertices are not created through a constructor, but through an API method that creates a unique id for the created vertex and returns a handle that can henceforth be used to refer to it.
  • The current rule on vertex uniqueness is adapted to forbid the creation of vertices that share the same points. The existing vertex must be used in such a situation, which can be easily achieved by cloning the existing vertex handle.
  • The main difference to the current situation is that, with unique ids and centralized storage, vertices can now be validated. A dedicated validator can scan all vertices, and throw an error if duplicate ones are detected.
  • The error message should be clear, explaining the issue concisely, listing options to address it, and pointing to further resources.
  • To address the problem of floating point accuracy, not only identical vertices must be recognized as duplicate, but also vertices that are close to each other.
  • Initially, a global epsilon value (the minimum distance between two vertices) can be used for that comparison. It should be chosen such that it works correctly for typical use cases.
  • Later on, this epsilon value can be configurable on a per-model basic, to support non-typical use cases.

I think with this approach, we can keep the kernel code simple and correct (no workaround required; floating point values can just be compared directly), while still reliably catching any problems arising from floating point accuracy issues. This comes at the cost of requiring the user to uphold the design rule that different vertices must not share points. It's likely we'll need to provide specific tools for that, for example if the user wants to transform a body such that it would share vertices with another body at the new position.

I think this is a worthwhile trade-off, as the alternative is code that is hard to make reliable, and that will blog up in the user's face as soon as they create a model that is untypically small (making the epsilon value too big) or untypically large (making the epsilon value too small).

@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 Feb 23, 2022
@hannobraun hannobraun self-assigned this Feb 28, 2022
@hannobraun
Copy link
Owner Author

I've started working on this.

@hannobraun
Copy link
Owner Author

I've landed quite a few pull request preparing for vertex validation, and I even have a pull request that implements it (#278). I can't merge it, unfortunately, because it turns out there are lots of dodgy pieces of code that create invalid vertices, and forcing vertex validation would basically mean that every model stops working.

I knew that there were some of those problems, which is why I wanted to implement vertex validation in the first place, before making too many other mistakes. But man, the problem is so much worse than I thought. I'm glad I started working on this when I did.

So, all that dodgy code needs to be fixed before vertex validation can be merged. Unfortunately, I found that this is not easy. The problem is, that the way shapes are represented is so redundant and easy to get wrong, that writing correct code that works with shapes is almost too hard to be feasible.

I actually opened an issue to address this (#280) earlier today, thinking I'd need to be working on this some time soon-ish. Well, turns out that soon-ish is right now. Further work on this issue is therefore paused for the time being, until #280 has been addressed.

@hannobraun
Copy link
Owner Author

#280 has been wrapped up. I'm back to working on this issue directly.

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