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.

Sign up for GitHub

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

Jump to bottom

Patch for #4620 - ensure posDetectSurfaces is invalidated on slice change #4679

Merged
merged 3 commits into from
Jan 7, 2019
Merged

Patch for #4620 - ensure posDetectSurfaces is invalidated on slice change #4679

merged 3 commits into from
Jan 7, 2019

Conversation

lordofhyphens
Copy link
Member

@lordofhyphens lordofhyphens commented Jan 6, 2019 •
edited
Loading

Invalidate posDetectSurfaces during make_perimeters() when posSlice would also get invalidated.
Can't use invalidate_step() as that invalidates the perimeters as well.

Fixes #4620

@lordofhyphens lordofhyphens requested a review from alranel as a code owner January 6, 2019 04:41
@lordofhyphens lordofhyphens self-assigned this Jan 6, 2019
@AppVeyorBot
Copy link

✅ Build Slic3r 1.3.0-master-2222 completed (commit 6576c81a74 by @lordofhyphens)

@lordofhyphens lordofhyphens force-pushed the patch-4620-invalidate-posDetectSurface branch from 8d36a25 to e4ee2bd Compare January 6, 2019 05:46
@lordofhyphens
Copy link
Member Author

Also have a regression test added to the CATCH test suite.

@AppVeyorBot
Copy link

✅ Build Slic3r 1.3.0-master-2224 completed (commit 86adaa2c9d by @lordofhyphens)

@VanessaE
Copy link
Collaborator

VanessaE commented Jan 6, 2019 •
edited
Loading

I can't comment on the code itself, but the behavior is spot-on. lgtm.

(no idea why CI barfed on the OSX build, but no problems here on Debian testing/Buster)

@lordofhyphens
Update some language used
6a23d99 
[ci skip]
@lordofhyphens
Completely check pre-condition in test.
59414e1 
Make sure that we have 1 solid bottom layer and 2 solid top layers before invoking our change.
@lordofhyphens lordofhyphens force-pushed the patch-4620-invalidate-posDetectSurface branch from b4cccdd to 59414e1 Compare January 6, 2019 22:38
@alranel
Copy link
Member

alranel commented Jan 6, 2019 •
edited
Loading

I'd turn this->state.invalidate(posSlice); into this->invalidate_step(posSlice); like we were doing before the bisected commit. In order to do this safely, I'd move set_started(posPerimeters) after the if (this->typed_slices) { block.
I think that state.invalidate() should be never called directly, except from invalidate_step() which ensures dependencies are invalidated too

@lordofhyphens
Use step_invalidate() instead to ensure dependencies are invalidated …
fe2b673 
…as well.

Move set_started() until after invalidations.
@lordofhyphens
Copy link
Member Author

Done.

@AppVeyorBot
Copy link

✅ Build Slic3r 1.3.0-master-2234 completed (commit 4ca602b32b by @lordofhyphens)

@lordofhyphens lordofhyphens merged commit 364f2a6 into master Jan 7, 2019
@lordofhyphens lordofhyphens deleted the patch-4620-invalidate-posDetectSurface branch January 7, 2019 00:03
lordofhyphens added a commit that referenced this pull request Mar 30, 2019
@lordofhyphens
Restrict speed setting to fixed-point output with 3 decimal places. I…
bddbc5c 
…ncludes test.

Fixes #4679
@lordofhyphens lordofhyphens mentioned this pull request Mar 30, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alranel alranel alranel approved these changes

@platsch platsch Awaiting requested review from platsch

@supermerill supermerill Awaiting requested review from supermerill

Assignees

@lordofhyphens lordofhyphens

Labels
Perimeters
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lordofhyphens @AppVeyorBot @VanessaE @alranel