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

Path: Fix PointProjectionFailed #6886

Merged
merged 5 commits into from Jun 10, 2022
Merged

Path: Fix PointProjectionFailed #6886

merged 5 commits into from Jun 10, 2022

Conversation

younghang
Copy link
Contributor

fix the PointProjectionFailed exception when split edge in wrebaseWire().


Thank you for creating a pull request to contribute to FreeCAD! To ease integration, we ask you to conform to the following items. Pull requests which don't satisfy all the items below might be rejected. If you are in doubt with any of the items below, don't hesitate to ask for help in the FreeCAD forum!

  • Your pull request is confined strictly to a single module. That is, all the files changed by your pull request are either in App, Base, Gui or one of the Mod subfolders. If you need to make changes in several locations, make several pull requests and wait for the first one to be merged before submitting the next ones
  • In case your pull request does more than just fixing small bugs, make sure you discussed your ideas with other developers on the FreeCAD forum
  • Your branch is rebased on latest master git pull --rebase upstream master
  • All FreeCAD unit tests are confirmed to pass by running ./bin/FreeCAD --run-test 0
  • All commit messages are well-written ex: Fixes typo in Draft Move command text
  • Your pull request is well written and has a good description, and its title starts with the module name, ex: Draft: Fixed typos
  • Commit messages include issue #<id> or fixes #<id> where <id> is the issue ID number from our Issues database in case a particular commit solves or is related to an existing issue. Ex: Draft: fix typos - fixes #4805

And 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 0.20 Changelog Forum Thread.


@github-actions github-actions bot added the WB CAM Related to the CAM/Path Workbench label May 15, 2022
@freecadci
Copy link

pipeline status for feature branch PR_6886. Pipeline 539544670 was triggered at 7630ee0. All CI branches and pipelines.

@luzpaz
Copy link
Contributor

luzpaz commented May 15, 2022

Is there a forum thread associated with this PR?

@luzpaz luzpaz changed the title Path:Fix PointProjectionFailed Path: Fix PointProjectionFailed May 15, 2022
@luzpaz
Copy link
Contributor

luzpaz commented May 15, 2022

Looks like it's failing the CI, but I don't understand why.
@younghang please run:

git commit --amend -m "Path: fix the PointProjectionFailed exception when split edge in wrebaseWire()" -m "<reference a forum discussion here>"`

@younghang
Copy link
Contributor Author

younghang commented May 16, 2022

Is there a forum thread associated with this PR?

No, I have searched this in the forum. I found the reason for the PointProjectionFailed, that is when

 gp_Pnt pprev(BRep_Tool::Pnt(xp.CurrentVertex()));

is called, there is a tiny difference between pprev and vertex around 0.0000001. which raise the PointProjectionFail exception in

mkEdge1.Init(curve, myBestPt, pprev);

.

Looks like it's failing the CI, but I don't understand why.

@luzpaz I made a mistake the vairable name is wrong. I have made another commit.

@freecadci
Copy link

pipeline status for feature branch PR_6886. Pipeline 539716433 was triggered at 07d7ced. All CI branches and pipelines.

src/Mod/Path/App/Area.cpp Outdated Show resolved Hide resolved
@luzpaz
Copy link
Contributor

luzpaz commented May 16, 2022

CC @sliptonic please review when you get a chance..thanks!

@sliptonic sliptonic requested a review from mlampert May 16, 2022 13:39
@freecadci
Copy link

pipeline status for feature branch PR_6886. Pipeline 540628320 was triggered at 5ae3a08. All CI branches and pipelines.

src/Mod/Path/App/Area.cpp Outdated Show resolved Hide resolved
@realthunder
Copy link
Collaborator

@younghang I suggest to try this projection on curve only if it fails to make either edges.

@freecadci
Copy link

pipeline status for feature branch PR_6886. Pipeline 541638197 was triggered at a21a5a2. All CI branches and pipelines.

@freecadci
Copy link

pipeline status for feature branch PR_6886. Pipeline 541638944 was triggered at 18120f8. All CI branches and pipelines.

@younghang
Copy link
Contributor Author

curve only if it fails to make either edges.

@realthunder The split operation only executes once when the StartPoint is set, and I think the split point should on the edge though not all edges fail in current operation.

Another bug I found in this rebaseWire() is when the StartPoint is set on some edge by force, the closed wire will be broken some edges are missing.
I use

       if (edge.Orientation() != myBestWire->wire.Orientation())
       {
            mkEdge1.Init(curve, myBestPt, pprev);
            mkEdge2.Init(curve, pt, myBestPt);
       }

instead of

      if(reversed) {
           mkEdge1.Init(curve, myBestPt, pprev);
           mkEdge2.Init(curve, pt, myBestPt);
      }

and it seems to work in my project.

@luzpaz
Copy link
Contributor

luzpaz commented May 25, 2022

@younghang can you post to the Path subforum announcing this PR and x-post this URL back to this ticket so we can track it?)

@younghang
Copy link
Contributor Author

@younghang can you post to the Path subforum announcing this PR and x-post this URL back to this ticket so we can track it?)

@luzpaz Okay. I have post it here: https://forum.freecadweb.org/viewtopic.php?f=15&t=68956

@donovaly donovaly requested a review from sliptonic June 9, 2022 22:38
@donovaly
Copy link
Member

donovaly commented Jun 9, 2022

What is the status of this PR?

@sliptonic , can you please have a look?

@donovaly donovaly added this to the 0.20 milestone Jun 9, 2022
@sliptonic sliptonic merged commit 083a27d into FreeCAD:master Jun 10, 2022
@donovaly
Copy link
Member

@younghang , many thanks for the fix!

In future you only need to check that it compiles also when precompiled headers.
I fixed this now by adding the missing include to Precompiled.h: d13ecc1efa177be

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WB CAM Related to the CAM/Path Workbench
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants