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

Convert edge builder API into partial edge API #1135

Merged
merged 25 commits into from
Sep 23, 2022
Merged

Conversation

hannobraun
Copy link
Owner

This is another step towards addressing the object identity problems that are currently holding up #1079.

@hannobraun
Copy link
Owner Author

This is failing the CI build. It's a validation error, because the half-edges of a cycle do not connect (caught by the export validator in the star model). There's only a minuscule difference between vertices that are supposed to be the same, so it's a floating-point accuracy issue. It seems to start with this commit: 0c7c508

I don't think that this commit actually introduces the bug. I might be mistaken, but I think it just exposes a bug that was already there. The vertices were already computed redundantly, and it just happened to work. The new code just happens to compute slightly different vertices redundantly, therefore exposing the bug.

This is exactly the kind of bug that would no longer be possible, if validation were based on object identity, not object equality. Then it would always cause a validation error, hence making it impossible for such a bug to hide. It's one of the reasons I want #1021, and why I'm doing all this work with the builder / partial object API.

I'll try to locate and fix the buggy piece of code, but I might not be able to finish that today. If so, I'll leave this open until next week.

@hannobraun hannobraun mentioned this pull request Sep 23, 2022
6 tasks
@hannobraun
Copy link
Owner Author

I've located the bug and pushed a commit that fixes it before it is uncovered (meaning all the commits in this pull request should pass the full test suite). Here it is: 73bdcdb

The fixed code isn't exactly beautiful, but it's fine for now. It's in a builder I have yet to update, so I'll have an opportunity to clean it up soon.

@hannobraun hannobraun merged commit dd66a76 into main Sep 23, 2022
@hannobraun hannobraun deleted the ready/partial-edge branch September 23, 2022 15:46
@hannobraun hannobraun changed the title Convert edge build API into partial edge API Convert edge builder API into partial edge API Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant