Sort vertices by a precomputed Y band instead of a fuzzy comparison - #224
Open
webdevred wants to merge 3 commits into
Open
Sort vertices by a precomputed Y band instead of a fuzzy comparison#224webdevred wants to merge 3 commits into
webdevred wants to merge 3 commits into
Conversation
Reproduces the non-transitive comparator bug with real node spacing from a gen4-style body file: at y-sorting-threshold 0.1 the frontmost node in a group sorts to the back instead of the front. Asserts the invariant that should hold (no node ends up behind another node more than the threshold further forward), currently violated. Fixture lives in examples/regression_jbeam/, not examples/jbeam/, so it stays out of jbeam-edit-dump-ast's scan (which only reads examples/jbeam/ and would exitFailure on any file that fails to transform) and out of the curated example set the jbeam maintainer keeps.
Treating two vertices as equal when their Y distance is under the threshold is not transitive, so sortBy had no defined result for a chain of vertices with small consecutive gaps and a large total spread. Walk the group in Y order instead, assigning an integer band that changes when a vertex sits more than the threshold from the band's start. Comparing integers cannot contradict itself, and the band is never wider than the configured value. supportThreshold becomes Scientific to match the other config fields.
The accumulator started at Y=0, which is a point in the middle of the vehicle rather than a vertex. Every vertex within the threshold of the centreline therefore shared one band, which could be twice as wide as configured. Two vertices 0.08 apart across zero came out ordered by height instead of by Y at threshold 0.05.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two vertices counted as equal in Y when their distance was under the threshold, which is not transitive: A can equal B and B equal C while A and C are further apart than the threshold.
sortByhas no defined result for such a comparator, so a chain of vertices with small consecutive gaps came out in whatever order the merge happened to compare them, which is what put the frontmost vertex of a group last.Walk the group in Y order once instead, assigning an integer band that changes when a vertex sits more than the threshold from the band's start, and compare bands. Integers cannot contradict themselves, so the bug class is gone regardless of the configured value, and anchoring on the band's start rather than on the previous vertex keeps a band from chaining wider than the threshold. That matters at 0.1, where a plain gap rule collapses twelve of the twenty fixture vertices into one band spanning 46 cm.
aMetaand the support name key keep their places ahead of the band, so metadata still clusters and support vertices still group by origin. Only the Y comparison changed, and the example output is untouched.Worth a second look:
indexBandbreaks on>= thrwhere the old comparator used> thr. A distance of exactly the threshold now starts a new band. That splitsbolide_chassis, whose f6l, f5l and f1l span exactly 0.050 and used to share a band at the 0.05 default. No fixture covers it either way.