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: PathArray based on multiple bases, and choice of direction #4812

Merged
merged 16 commits into from May 28, 2021

Conversation

jimzim111
Copy link
Contributor

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 FreeCAD bug tracker issue number in case a particular commit solves or is related to an existing issue on the tracker. Ex: Draft: fix typos - fixes #0004805

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.


@jimzim111
Copy link
Contributor Author

These changes are intended to help reduce tool travel time. Discussion of the first one was at https://forum.freecadweb.org/viewtopic.php?f=15&t=58768&p=505517#p505517.

The first change allows PathArrays to be based on multiple Paths instead of just one. It makes a copy of all of the bases in each arrayed position before moving on to the next position.

The second change allows swapping the direction in which copies are arrayed (X first instead of Y first).

@jimzim111 jimzim111 changed the title Path: PathArray based on multiple Path: PathArray based on multiple bases, and choice of direction May 24, 2021
Copy link
Contributor

@Russ4262 Russ4262 left a comment

Choose a reason for hiding this comment

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

I built and tested. Works as advertised. Code changes look good.

jim and others added 4 commits May 24, 2021 21:00
…tance. Helps create an imperfect "hand-crafted" appearance.

Adds properties:
 - JitterPercent: % of copies that will be randomly offset
 - JitterMagnitude: Maximum distance to offset copies
src/Mod/Path/PathScripts/PathArray.py Show resolved Hide resolved
src/Mod/Path/PathScripts/PathArray.py Show resolved Hide resolved
src/Mod/Path/PathScripts/PathArray.py Show resolved Hide resolved
else:
base = [obj.Base]

if len(base)>0:
Copy link
Member

Choose a reason for hiding this comment

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

The consequenc if len(base) == 0 isn't visible until the end. Looking ahead, nothing happens. So, instead you should check for that case first and and return if it's true, raising an error of necessary. Currently all the subsequent code is indented and dependent on that check which is confusing.

return
if b.ToolController != obj.ToolController:
# this may be important if Job output is split by tool controller
PathLog.warning('Arrays of paths having different tool controllers are handled according to the tool controller of the first path.')
Copy link
Member

Choose a reason for hiding this comment

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

Not required for acceptance of this PR but if you're up for it, you could also check if the base feature is a dressup and allow for arrays of those as well. Currently there's no way to make an array of dressed up operations.

@jimzim111 jimzim111 requested a review from sliptonic May 26, 2021 01:38
@sliptonic
Copy link
Member

Merging.
In the future, unless other people are basing future work on your specific branch, please rebase your PR on upstream master rather than merging those commits back to your branch. Rebasing keeps the history cleaner

@sliptonic sliptonic merged commit a9f6fad into FreeCAD:master May 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants