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

Sketch: Fix algorithm seeking for missing coincidences in Validate #6393

Merged
merged 1 commit into from Feb 28, 2022

Conversation

0penBrain
Copy link
Contributor

@github-actions github-actions bot added the WB Sketcher Related to the Sketcher Workbench label Feb 18, 2022
bool operator()(const VertexIds& x,
const VertexIds& y) const
{
return (x.GeoId < y.GeoId || ((x.GeoId == y.GeoId) && (x.PosId < y. PosId)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't PosIdSketcher::PointPos now an enum class?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AjinkyaDahale yes it is

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then is < appropriate here? Also, if I understand it correctly end=2 and mid=3, which could be troublesome.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a problem. 'LESS' comparing functions should just ensure a transitive ordering in all cases, and that when 2 equal objects are compared, it returns false in both argument orders. ;)

@freecadci
Copy link

pipeline status for feature branch PR_6393. Pipeline 474829595 was triggered at d20c44b. All CI branches and pipelines.

@wwmayer
Copy link
Contributor

wwmayer commented Feb 26, 2022

The PR causes a crash when using chrisb's sketch example. For more details see above comments.

@wwmayer wwmayer changed the title Sketch: Fix algorithm seeking for missing coincidences in Validate [Do not merge]Sketch: Fix algorithm seeking for missing coincidences in Validate Feb 27, 2022
@0penBrain
Copy link
Contributor Author

The PR causes a crash when using chrisb's sketch example. For more details see above comments.

Actually the crash comes from the VertexID_Less comparator (which probably is called with an empty thing). Debugger is warming up. Thx for review.

@0penBrain
Copy link
Contributor Author

0penBrain commented Feb 27, 2022

@wwmayer Should everything be OK now. I tested on more test cases (actually I missed the simplest ones, which actually were the most significant).
Let you edit the title of the PR when you think it's right. ;)

@freecadci
Copy link

pipeline status for feature branch PR_6393. Pipeline 480439209 was triggered at 1b65c81. All CI branches and pipelines.

@wwmayer
Copy link
Contributor

wwmayer commented Feb 27, 2022

Let you edit the title of the PR when you think it's right. ;)

I will have a new look in the next couple of days.

@wwmayer
Copy link
Contributor

wwmayer commented Feb 28, 2022

Works as expected now. Here is the mandatory unit test to see that current master will fail: 6e4c618

@wwmayer wwmayer changed the title [Do not merge]Sketch: Fix algorithm seeking for missing coincidences in Validate Sketch: Fix algorithm seeking for missing coincidences in Validate Feb 28, 2022
@wwmayer wwmayer merged commit c036128 into FreeCAD:master Feb 28, 2022
@0penBrain
Copy link
Contributor Author

Here is the mandatory unit test to see that current master will fail: 6e4c618

Please next time just let me know that a test is expected. Actually I didn't add it because I (wrongly) was pretty sure that this function can't be called from Python. I'll add another one that allowed me to find another flaw in my algorithm. ;)

@wwmayer
Copy link
Contributor

wwmayer commented Feb 28, 2022

Please next time just let me know that a test is expected.

A unit test is basically always expected. Although Qt has a test module to test GUI intersections I don't expect a function for this kind of stuff.

Actually I didn't add it because I (wrongly) was pretty sure that this function can't be called from Python.

I didn't know about the exposed function either and so I simply searched in the code base. If there weren't one I would have written a wrapper for it.

@0penBrain 0penBrain deleted the sketchFindMissing branch February 25, 2023 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WB Sketcher Related to the Sketcher Workbench
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants