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

Wall line segments that have a flow < wall_min_flow are converted into travel moves. #721

Merged
merged 8 commits into from May 30, 2018

Conversation

4 participants
@smartavionics
Copy link
Contributor

commented Mar 12, 2018

Companion to Ultimaker/Cura#3479.

@Ghostkeeper

This comment has been minimized.

Copy link
Member

commented Apr 5, 2018

Developers, see issue CURA-5119.

smartavionics added some commits Apr 26, 2018

Omit final travel move if last wall line segment is too thin to print.
There's no point in moving to the end of the line because the next line may involve a
travel move at the beginning. If the next line does start at the end of the current line,
a move will be done anyway.
Don't move to the first point in line in case the first segment is to…
…o thin to print.

Instead, we delay moving until the first segment that is thick enough to print is to be
output.
Don't initialise travel_required to true, only set it true when skipp…
…ing a thin segment.

Instead, when testing to see if a travel move is required we look at both travel_required
and first_line.
// the overlap compensation is not perfect, it can produce short non-flow reduced line segments within a sequence of flow reduced
// line segments and so to try and avoid printing the spurious fat line segments we require that their lengths are above a threshold

const int64_t max_spurious_fat_segment_length = 50; // microns

This comment has been minimized.

Copy link
@ianpaschal

ianpaschal May 24, 2018

Contributor

Some notes about this: According to Ghostkeeper, this should be using coordinates, as well as the global resolution parameters.

We don't think this hack is too bad but are wondering what the consequences of not having it would be...

This comment has been minimized.

Copy link
@smartavionics

smartavionics May 24, 2018

Author Contributor

Some notes about this: According to Ruben, this should be using coordinates, as well as the global resolution parameters.

Not sure I understand what you mean by using coordinates. As for the global resolution params, I don't think they are accessible in this method, are they?

We don't think this hack is too bad but are wondering what the consequences of not having it would be...

If you don't have the hack then tiny line segments can be included which are not really wanted. Let me try and come up with a concrete example...

This comment has been minimized.

Copy link
@smartavionics

smartavionics May 24, 2018

Author Contributor

I can't recreate the situation that made me want to filter out the tiny fat line segments right now but I am sure that I added that because I was seeing a problem. I don't think I just put it in from the beginning in the hope that it would be useful.

This comment has been minimized.

Copy link
@ianpaschal

ianpaschal May 25, 2018

Contributor

Not sure I understand what you mean by using coordinates.

I mean it should be a coord_t

Makes sense regarding the line segments. For me it's OK because it's logical, but was still worth asking.

This comment has been minimized.

Copy link
@smartavionics

smartavionics May 25, 2018

Author Contributor

Ah, understood. I've made that little change.

@Ghostkeeper
Copy link
Member

left a comment

Overall I would've thought that it would be simpler to implement this. You already know the flow, so instead of this one addWallLine call you could simply call addTravel. I don't see why this needs to be implemented in this way with the travel_required flag.

float non_bridge_line_volume = 0; // zero before first non-bridge line is output
double speed_factor = 1.0; // start first line at normal speed
double distance_to_bridge_start = 0; // will be updated before each line is processed

const SettingsBaseVirtual* extr = getLastPlannedExtruderTrainSettings();
const double min_bridge_line_len = extr->getSettingInMicrons("bridge_wall_min_length");
const double wall_min_flow = extr->getSettingInPercentage("wall_min_flow") / 100;

This comment has been minimized.

Copy link
@Ghostkeeper

Ghostkeeper May 25, 2018

Member

We have a function getSettingAsRatio which does exactly what you need.

// the overlap compensation is not perfect, it can produce short non-flow reduced line segments within a sequence of flow reduced
// line segments and so to try and avoid printing the spurious fat line segments we require that their lengths are above a threshold

const coord_t max_spurious_fat_segment_length = 50; // microns

This comment has been minimized.

Copy link
@Ghostkeeper

Ghostkeeper May 25, 2018

Member

You should square this and compare with vSize2(p0 - p1) for performance.

@@ -646,15 +646,14 @@ void LayerPlan::addWall(ConstPolygonRef wall, int start_idx, const GCodePathConf
}
}

Point p0 = wall[start_idx];
addTravel(p0, always_retract);

This comment has been minimized.

Copy link
@Ghostkeeper

Ghostkeeper May 25, 2018

Member

I think that this was a much nicer/simpler solution than the business you have below with the first_line boolean variable. Why was it removed?

This comment has been minimized.

Copy link
@smartavionics

smartavionics May 27, 2018

Author Contributor

Because, before, it would always do the first travel even when the whole wall is not printed because all the line segments have a flow that is too low. You only want to do the initial travel move if there are any segments that are to be printed.

}
else
{
travel_required = true;

This comment has been minimized.

Copy link
@Ghostkeeper

Ghostkeeper May 25, 2018

Member

It seems illogical to set some travel_required variable here rather than just travelling immediately to p1.

This comment has been minimized.

Copy link
@smartavionics

smartavionics May 27, 2018

Author Contributor

See reply above.


const coord_t max_spurious_fat_segment_length = 50; // microns

if (flow >= wall_min_flow && (first_line || !travel_required || vSize(p0 - p1) > max_spurious_fat_segment_length))

This comment has been minimized.

Copy link
@Ghostkeeper

Ghostkeeper May 25, 2018

Member

addWallLine is not called if the line is smaller than 50 microns now (unless it's the first line).

This comment has been minimized.

Copy link
@smartavionics

smartavionics May 27, 2018

Author Contributor

That's true - I suppose you could get a first line printed that is shorter than 50 microns that you would otherwise not print but I don't think that is very likely and even if you did print it, so what?

This comment has been minimized.

Copy link
@ianpaschal

ianpaschal May 29, 2018

Contributor

This one is still a bit of a problem because its possible to try and print a circle in which all the lines are less than 50 microns. I mean, unlikely because people often post files with stupidly low resolutions online, but it could happen. Then we'd end up skipping all the entire shape.

This comment has been minimized.

Copy link
@smartavionics

smartavionics May 29, 2018

Author Contributor

OK, I'll take the fat line avoidance stuff out for you (and keep it for me).

@ianpaschal

This comment has been minimized.

Copy link
Contributor

commented May 29, 2018

This is ready for a re-review?

@smartavionics

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2018

Yes, I think so.

@ianpaschal

This comment has been minimized.

Copy link
Contributor

commented May 29, 2018

Just spoke with @Ghostkeeper and we're still concerned about one point.

Remove the hack that avoids printing spurious short segments.
This was requested so that people can happily print walls with sub-50um lines.
@smartavionics

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2018

Hi Ian, I have removed the hack so people can now happily print walls with lines < 50um long.

@smartavionics

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2018

You will, of course, now get spurious travels to random points on curved walls but, hey, that's a small price to pay.

@ianpaschal

This comment has been minimized.

Copy link
Contributor

commented May 30, 2018

That's better than having the curved wall not print at all, though, right? I mean, now I'm all conflicted trying to think what's more likely: long chains of 50 micron segments or random single 50 micron segments...

@smartavionics

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2018

Hi Ian, don't agonise about it. Go with how it is now and in the event of it being a real problem think of that as an incentive to fix the overlap compensation!

@ianpaschal

This comment has been minimized.

Copy link
Contributor

commented May 30, 2018

Yeah I guess neither is very common but we'd rather have small spurs that can be trimmed off with an x-acto knife than holes or a missing wall in the case of a chain of 50 micron .

"Chain so light when a breeze roll by, man it flow-ow-ow, man it flow-ow-ow;"
- The Lonely Island

Before:
screen shot 2018-05-30 at 13 34 12

After:
screen shot 2018-05-30 at 13 34 23

@ianpaschal ianpaschal merged commit 9bd175f into Ultimaker:master May 30, 2018

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
@jackha

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2018

Hi @smartavionics , why are the settings Minimum Wall Flow and Prefer Retract under Compensate Wall Overlaps?

@smartavionics

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2018

Hi @smartavionics , why are the settings Minimum Wall Flow and Prefer Retract under Compensate Wall Overlaps?

Because they are only relevant when the walls are being reduced in width by the overlap compensation. If there's no overlap compensation, all walls will be 100% of their normal width.

@jackha

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2018

We've discussed it and decided that the settings are not actual sub settings of wall overlap compensation (but related to), we're putting it just below. Visibility is the same as before.

@smartavionics

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2018

OK, that's fine. Will you still hide them when the overlap compensation is disabled?

@jackha

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2018

Yes, as you said these settings wouldn't make sense if overlap compensation is off.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.