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

Alternate wall direction #1250

Closed
wants to merge 8 commits into from
Closed

Alternate wall direction #1250

wants to merge 8 commits into from

Conversation

BagelOrb
Copy link
Contributor

@BagelOrb BagelOrb commented May 4, 2020

Alternate printing directions of walls with each inset and each layer in order to homogenize directional stress buildup.

@smartavionics
Copy link
Contributor

I would have thought that this could cause a lot of problems as the reversed polygons will look like holes and have -ve area. This PR is going to need some good testing to ensure that it has no adverse effects anywhere.

@BagelOrb
Copy link
Contributor Author

BagelOrb commented May 4, 2020

I made the PR so that some colleague of mine can do some testing before it actually gets merged.

It's not gonna be interesting for all materials, so it's just an option and disabled by default.

It will just be interesting for materials where the stress buildup has more significant effect than the tiny hole introduced by the 180* turn to print the next inset. For some materials the stress buildup is huge.

@BagelOrb
Copy link
Contributor Author

BagelOrb commented Jun 5, 2020

We are also getting external requests for this feature. His reasoning here is solid and is approximately the same as the reason why we developed it (ask Johan).

Please consider merging this PR.

@BagelOrb BagelOrb requested a review from Ghostkeeper June 5, 2020 09:45
@BagelOrb BagelOrb force-pushed the alternate_wall_direction branch 2 times, most recently from f5913e1 to ee98750 Compare July 6, 2020 08:40
@Ghostkeeper
Copy link
Collaborator

Ghostkeeper commented Aug 11, 2020

I agree with Smartavionics here that this is quite a dangerous implementation. Regardless of whether or not it currently goes wrong, when writing code, I would normally assume that positive shapes are winding counter-clockwise and negative shapes are winding clockwise. So does the ClipperLib::Area function, which is used in a dozen places, and the ClipperLib::Orientation function similarly.

It does go wrong already though. This implementation currently breaks features which check for area or orientation on the original part shapes, such as Minimum Tower-Supported Diameter, bridging, Fuzzy Skin Outside Only, and some utility functions like ensureInsideOrOutside. To test something easy and tangible going wrong, try enabling Fuzzy Skin Outside Only and Alternate Wall Direction.

I think a better implementation would be located at the FffGcodeWriter stage, by iterating in reverse over the wall line segments. That would keep the actual parts in the correct orientation, but only print the walls in the correct order.

@BagelOrb
Copy link
Contributor Author

It's not changing the direction of original part, only those of the insets.

My implementation does seem problematic for Fuzzy skin on outside only, but I find it harder to believe it impacts support towers or bridging.

How about I perform the polygon.reverse() just before the call to addPolygonsByOptimizer ? Would that be okay?

@BagelOrb
Copy link
Contributor Author

Thanks for pointing out those weaknesses. I had underestimated the problem that Mark tried to point out. I'm sorry for that.

@BagelOrb BagelOrb force-pushed the alternate_wall_direction branch 2 times, most recently from 3ab6c7a to 0aa9e93 Compare August 12, 2020 08:53
@BagelOrb
Copy link
Contributor Author

I've looked at the code and it's rather awkward to change the inset polygons in FffGcodeWriter because the whole SliceMeshStorage is const at that point.

Copying the insets and altering the copy is really cumbersome, because it would introduce changes all the way into InsetOrderOptimizer. That means a lot of changes and therefore very bug-prone.

Instead I opted to alter the insets at the very end of processDerivedWallsSkinInfill, which is called almost at the end of FffPolygonGenerator. However, generateSupportInfillFeatures is still called after it, but I assume that the gradual support etc doesn't depend on the walls; generateOverhangAreas is called way before processDerivedWallsSkinInfill...

Please see the new implementation.

With coasting enabled you can see in a single screenshot that it works (at least on the outer wall):
image

@BagelOrb
Copy link
Contributor Author

Somehow something went wrong during the last rebase just before the last push. Now it's fixed.

@maruhe
Copy link

maruhe commented Jul 9, 2021

what happened to this feature?
I'd be happy to use it

@jsourice
Copy link

jsourice commented Jul 9, 2021

what happened to this feature?
I'd be happy to use it

It's natively available on ideamaker now, for ceramic and metal filament that need sintering it's crucial.

@BagelOrb
Copy link
Contributor Author

This feature wasn't merged because it had to do with the walls, while the Arachne version of Cura would interfere with that, so it was decided to postpone merging this in favor of reimplementing it after Arachne would become a part of mainline Cura.

The fact that it has taken more than a year to merge Arachne means it might have been better to merge this feature after all, but that's a bit late now. Now it's more efficient to reimplement the feature after Arachne is merged and not bother with this version at all.

@BagelOrb
Copy link
Contributor Author

This feature has been reimplemented after Arachne was merged. This PR is stale and superfluous now.

@rburema
Copy link
Member

rburema commented Mar 11, 2022

It turned out to be easier to just start again 'post'-Arachne. See: #1541 and #1545

Note: The first one seems 'closed', but the relevant commits have been cherry-picked. The last one is the same feature, but for support.

Will be in 5.0, but it might be that you need to make material profiles to adjust this setting (not enabled for frontend by default).

@rburema rburema closed this Mar 11, 2022
@rburema rburema deleted the alternate_wall_direction branch March 11, 2022 14:06
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

7 participants