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

[For testing] Sketcher: optionally merge 'coincident' within 'point-on-object' constraint #7478

Closed
wants to merge 4 commits into from

Conversation

0penBrain
Copy link
Contributor

The merge is activated with User parameter:BaseApp/Preferences/Mod/Sketcher/General/MergePointOnObjectWithCoincident boolean switch

The PR is essentially done to have a test between 2 flavors (merged or not) and decide what is to be kept.

The "feature" is only the last commit. 3 first commits are independent fixes and improvements.

@github-actions github-actions bot added the WB Sketcher Related to the Sketcher Workbench label Sep 12, 2022
@berndhahnebach
Copy link
Contributor

pipeline status for feature branch PR_7478. Pipeline 637873179 was triggered at c674aaa. All CI branches and pipelines.

@donovaly
Copy link
Member

Many thanks.
However, can you please split the PR into at least two. The new feature should become its own PR so that one can test it better and once it is merged we can reference it in the release notes as reference.

To be precise, what are the issues fixed by the first commits?
What is the feature of the 4th commit?

@0penBrain 0penBrain changed the title Sketcher: optionally merge 'coincident' within 'point-on-object' constraint [For testing] Sketcher: optionally merge 'coincident' within 'point-on-object' constraint Sep 24, 2022
@0penBrain 0penBrain marked this pull request as draft September 24, 2022 06:26
@0penBrain
Copy link
Contributor Author

@donovaly This PR was intended mainly for testing. Was not clear, I clarified in title and set to draft.
I will comment the 3 first commits on a new PR with only these 3 ones. To be clear, I'll let this PR as is, then latter rebase if/when 3 first commits are merged into master so this one will only be the last commit.

The last commit allows (with parameter switch in the description) to merge Sketcher 'coincident constraint' into 'point-on-object' constraint. It means that 'coincident' is no more present in the default menu/toolbar, and you can apply a coincident constraint by selecting 2 or more vertices and clicking 'point-on-object'.

@luzpaz
Copy link
Contributor

luzpaz commented Sep 24, 2022

@0penBrain do you want an experimental snap build for testers to test this PR ?

@0penBrain
Copy link
Contributor Author

@0penBrain do you want an experimental snap build for testers to test this PR ?

For myself I have a build. :) Maybe better ask in the forum discussion ;)

@luzpaz
Copy link
Contributor

luzpaz commented Sep 24, 2022

Sure! Link to forum discussion?

@0penBrain
Copy link
Contributor Author

Sure! Link to forum discussion?

Arf. Silly me. Was sure it was on the description : https://forum.freecadweb.org/viewtopic.php?f=8&t=71793

@adrianinsaval
Copy link
Member

I myself also had a build but would appreciate if there is a snap and more testers, hopefully some of the detractors can give it a chance too.

@luzpaz
Copy link
Contributor

luzpaz commented Sep 25, 2022

Snap build triggered at FreeCAD/FreeCAD-snap#62 (x-posting to thread as well)

@abdullahtahiriyo
Copy link
Contributor

@0penBrain

Are you pursuing this? Do you want me to take a look?

@0penBrain
Copy link
Contributor Author

@abdullahtahiriyo This was for community testing and feedback. AFAIK there's no agreement from community to move forward to this. I plan nothing for this ATM. ;)

@abdullahtahiriyo
Copy link
Contributor

OK. Thanks.

@adrianinsaval
Copy link
Member

Now that coincidence can work by selecting circle edges I think this is no longer viable, that is a very useful functionality so it's not worth loosing it for this IMO (and I was a big supporter of this).

@abdullahtahiriyo
Copy link
Contributor

If I understood it correctly, could then we perhaps close the PR?

@0penBrain
Copy link
Contributor Author

Yes I close. No need to let it queued. We can reopen another (clean) PR in the future if we have a need for this. ;)

@0penBrain 0penBrain closed this Nov 27, 2022
@0penBrain 0penBrain deleted the sketcherPOOCoincident branch February 25, 2023 10:02
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

6 participants