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

Sketcher: Coincidence constraint: fix error introduced by concentric #7794

Merged
merged 1 commit into from Nov 14, 2022

Conversation

PaddleStroke
Copy link
Contributor

Fix bug mentioned by @0penBrain introduced in : #7703 (comment)

Thank you for creating a pull request to contribute to FreeCAD! Place an "X" in between the brackets below to "check off" to confirm that you have satisfied the requirement, or ask for help in the FreeCAD forum if there is something you don't understand.

  • Your Pull Request meets the requirements outlined in section 5 of CONTRIBUTING.md for a Valid PR

Please remember to update the Wiki with the features added or changed once this PR is merged.
Note: If you don't have wiki access, then please mention your contribution on the 1.0 Changelog Forum Thread.


@github-actions github-actions bot added the WB Sketcher Related to the Sketcher Workbench label Nov 14, 2022
@freecadci
Copy link

pipeline status for feature branch PR_7794. Pipeline 693715672 was triggered at 665939a. All CI branches and pipelines.

Copy link
Contributor

@0penBrain 0penBrain left a comment

Choose a reason for hiding this comment

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

Not tested, but I think something like this is more clear:

    bool allConicsEdges = true; //If user selects only conics (circle, ellipse, arc, arcOfEllipse) then we make concentric constraint.
    bool atLeastOneEdge = false;
    for (std::vector<std::string>::const_iterator it = SubNames.begin(); it != SubNames.end(); ++it) {
        int GeoId;
        Sketcher::PointPos PosId;
        getIdsFromName(*it, Obj, GeoId, PosId);
        if (isEdge(GeoId,PosId)) {
            atLeastOneEdge = true;
            if (!isGeoConcentricCompatible(Obj->getGeometry(GeoId))) {
                allConicsEdges = false;
            }
        }
        else {
            allConicsEdges = false; //at least one point is selected, so concentric can't be applied.
        }
        if (atLeastOneEdge && !allConicsEdges) {
            QMessageBox::warning(Gui::getMainWindow(), QObject::tr("Wrong selection"),
                QObject::tr("Select two or more vertices from the sketch for a coincident constraint, or two or more circles, ellipses, arcs or arcs of ellipse for a concentric constraint."));
            return;
        }
    }

//    if (atLeastOneEdge && !allConicsEdges) {
//        QMessageBox::warning(Gui::getMainWindow(), QObject::tr("Wrong selection"),
//            QObject::tr("Select two or more vertices from the sketch for a coincident constraint, or two or more circles, ellipses, arcs or arcs of ellipse for a concentric constraint."));
//        return;
//    }

Commented block can be deleted of course. :)

@freecadci
Copy link

pipeline status for feature branch PR_7794. Pipeline 693756713 was triggered at 4172ba2. All CI branches and pipelines.

@0penBrain 0penBrain merged commit f9ba739 into FreeCAD:master Nov 14, 2022
@PaddleStroke
Copy link
Contributor Author

You got merging rights @0penBrain ? Good thing.

@0penBrain
Copy link
Contributor

You got merging rights @0penBrain ? Good thing.

Yep. Against my own will. 🤣 Don't expect too much from that though. ;)

@PaddleStroke
Copy link
Contributor Author

I don't expect you to be lenient 😄
But at least now you can merge things after reviewing when you deem they can be merged. Rather than having to have a second reviewer reviewing again.

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

3 participants