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

Simplify: fix scaling of maximum area deviation setting #1738

Merged
merged 1 commit into from
Nov 15, 2022
Merged

Simplify: fix scaling of maximum area deviation setting #1738

merged 1 commit into from
Nov 15, 2022

Conversation

Piezoid
Copy link
Contributor

@Piezoid Piezoid commented Oct 5, 2022

meshfix_maximum_extrusion_area_deviation is already in millimeters. Using get<coord_t> divides the value by 1000, instead the setting should be acceded with get<size_t> or get<int>.

I've already signaled and fixed this in #1710, but maybe this simple fix has a chance of making it into the 5.2 release?

The frontend commits that recently changed the default value Ultimaker/Cura@03c88c1 should probably be re-evaluated.

meshfix_maximum_extrusion_area_deviation is already in millimeters.
using get<coord_t> divides the value by 1000, instead it should be
get<size_t> or get<int>.
@jellespijker
Copy link
Member

You're right that this is handled incorrectly. Looking at the front end we use even use μm²

I will create a front-end PR to make sure that the values in the profiles, match the changes in this PR.

I myself wasn't fully aware that when we grab a coord_t type from the settings we multiply by a factor of thousand. I will do a scan one of these days to double check all settings.get<coord_t> against their intended use.

@jellespijker
Copy link
Member

jellespijker commented Nov 11, 2022

On second thought I'm not sure if the values in the front-end need to change, since currently the translate to a ridiculous high length deviation value.

>>> from pint import UnitRegistry
>>> u = UnitRegistry()
>>> width = 400 * u.um
>>> old_default = 50000 * u.um**2 * 1000
>>> old_length_dev = old_default / width
>>> old_length_dev.to("mm")
<Quantity(125.0, 'millimeter')>

I will contact @rijkvanmanen or @pkuiper-ultimaker to discuss how they feel about changing the default values in the front-end profiles

@jellespijker
Copy link
Member

Discussed this with @rijkvanmanen it looks like we use this as a cap in the simplify function; This never came into play due to the insanely high value because of that multiplication with a factor 1000. Even without that factor the impact of this change seems limited but it is good defensive programming imo and correct usage of the intended value.

I'm gonna merge this to main, thanks @Piezoid for contributing this fix

@jellespijker jellespijker merged commit 1adf212 into Ultimaker:5.2 Nov 15, 2022
@jellespijker jellespijker removed their assignment Nov 15, 2022
@rburema
Copy link
Member

rburema commented Nov 17, 2022

This shouldn't be in 5.2 I think. I've merged it to main now, but we should probably undo it in the 5.2 branch, or maybe even force push it back to where it was before.

rburema added a commit that referenced this pull request Nov 17, 2022
ntcong pushed a commit to ntcong/SuperCuraEngine that referenced this pull request Dec 29, 2022
…_scaling"

This reverts commit 1adf212, reversing
changes made to 1363b81.
@Piezoid Piezoid deleted the area_deviation_scaling branch January 3, 2023 20:44
jellespijker pushed a commit that referenced this pull request Feb 6, 2023
jellespijker added a commit that referenced this pull request Feb 6, 2023
ThomasRahm added a commit to ThomasRahm/CuraEngine that referenced this pull request Mar 15, 2023
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.

3 participants