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

Indent settings #39

Open
wants to merge 4 commits into
base: release
Choose a base branch
from
Open

Conversation

Sophist-UK
Copy link

This PR assumes that #38 has been merged.

1. Improve descriptions for some settings.
2. Improve description and default value for `arcwelder_min_arc_segment` to match the Marlin firmware configuration setting `MIN_CIRCLE_SEGMENTS`.
@fieldOfView
Copy link
Owner

As discussed here, I will not be merging this, unless the Cura team decides to merge your Cura PRs. What I argued about CuraEngine using only leaf settings was a strong convention with the team when I worked at Ultimaker, and the years after. But since most of that team has moved on, perhaps you can change their minds. I will follow what mainline Cura does, instead of making my plugin less consistent with the rest of Cura.

@Sophist-UK
Copy link
Author

I have checked the code: A) Cura does not filter parent settings; and B) there are several examples of CuraEngine referencing parent settings.

The example you gave was a rule which makes logical sense but is not a physical restraint - where there are sub-settings that are calculated from the parent, then CuraEngine should (somewhat obviously) use the child settings and not the parent one.

So we will need to see (when the Ultimaker folks have the time to review Community PRs) whether they merge them or not.

But I do fully understand that you want to wait and see with this one. And I would do the same in your position.

@fieldOfView
Copy link
Owner

I am done discussing this in this repo.

I will keep this PR open until a Cura dev either merges or rejects the PRs against Cura.

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.

2 participants