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

[Brainstorming] Warn if any of the objects exceed the printing area #1254

Closed
wants to merge 13 commits into from

Conversation

Projects
None yet
4 participants
@Javierma
Copy link

commented Mar 4, 2016

After fixing errors and removing non-related files, I try again the pull request. I wasn't able to add only one commit (I don't know why, maybe because previous ones where related with these files) but this time it is much more easy to know the changes that have been done. I believe I solved problems with coding styles (tabs and spaces) but if it is not the case please let me know.

Basically, what it happens is that when you add a gcode file and is analysed, in case that user has logged in and it is detected that a move exceeds the printing area, it will show a warning like the following one:

screenshot from 2016-03-04 12 57 45

Regards,

Javier

@@ -258,8 +260,9 @@ def throttle():

self._gcode = gcodeInterpreter.gcode()
self._gcode.load(self._current.absolute_path, self._current.printer_profile, throttle=throttle_callback)

This comment has been minimized.

Copy link
@foosel

foosel Mar 4, 2016

Owner

Please remove that white space :)

@@ -120,6 +122,13 @@ def _load(self, gcodeFile, printer_profile, throttle=None):
z = getCodeFloat(line, 'Z')
e = getCodeFloat(line, 'E')
f = getCodeFloat(line, 'F')
#Check if x,y or z exceeds printing area

This comment has been minimized.

Copy link
@foosel

foosel Mar 4, 2016

Owner

Hm... that is a bit backwards actually. This way you'll only get a warning if the object exceeds the size of the print area of the default printer profile basically, but not for the currently selected one. So if you get the initial analysis done for a 20x20x20 build volume but then select a printer profile with 10x10x10, it will not be able to detect that it might not fit after all. The gcode interpreter is only run once for each file since it is a very cpu expensive operation. So it should depend on the printer profile as little as possible. In fact, it's only used since we have to assume some starting feedrate if no F parameter is presented, which should rarely happen at all.

I'd rather suggest making the bounding box of the object (min/max X, Y, Z coordinate) part of the analysis result, and then utilizing that in the frontend only to display the notification. This would also have the additional advantage to allow for utilizing that information differently (e.g. for some sort of low res preview).

That would mean adjusting the gcode interpreter to track minimum and maximum coordinates on all three axes, persist that in the file's metadata and then use that data in the frontend only to display a message on load by calculating width, height, length on the fly and comparing with currently selected (!) printer profile.

This comment has been minimized.

Copy link
@foosel

foosel Mar 4, 2016

Owner

As @nophead just pointed out, you'll also need to take care of start/end gcode situations, e.g., the print job contains start gcode that moves the nozzle outside of the print volume (usually X/Y exceeding the boundaries) and extrudes a bit to prime it. This will be tricky to tackle.

A basic approach would be to only keep track of the min/max coordinates of movements that contain extrusion (delta-E != 0) and are actually movements (so delta-X/Y/Z != 0). Might still cause false positives though if for example the start gcode moves the nozzle from the outside position to the start of the model while priming further. Problematic, since we cannot distinguish printing from preparing to print just from the GCODE.

width=float(printer_profile["volume"]["width"])
depth=float(printer_profile["volume"]["depth"])
height=float(printer_profile["volume"]["height"])
if not self.warn and (x>width or y>depth or z>height):

This comment has been minimized.

Copy link
@foosel

foosel Mar 4, 2016

Owner

This only works for print beds which have their (0,0) origin in the lower left corner of the print bed. This is not necessarily the case, it could also be the center of the print bed, in which case a simple check if x/y/z are larger than width/depth/height would produce a false negative (imagine a printbed of 20x20 cm, center in the middle, so 10cm up and left from the lower left corner - an object with max X coordinate of 19 would definitely not fit since that would be 19cm to the right of the origin, so actually 9cm off the bed, but the code would not detect that).

@@ -271,6 +280,8 @@ def _load(self, gcodeFile, printer_profile, throttle=None):
def _parseCuraProfileString(self, comment, prefix):
return {key: value for (key, value) in map(lambda x: x.split("=", 1), zlib.decompress(base64.b64decode(comment[len(prefix):])).split("\b"))}

def getWarning(self):

This comment has been minimized.

Copy link
@foosel

foosel Mar 4, 2016

Owner

This is not Java, we don't need getters for everything :) (Yes, I know, I've done that myself, I'm trying to get rid of such things while refactoring)

This comment has been minimized.

Copy link
@nophead

nophead Mar 4, 2016

Contributor

A further complication is that start end gcode segments might move outside
the maximum print volume.

On 4 March 2016 at 13:19, Gina Häußge notifications@github.com wrote:

In src/octoprint/util/gcodeInterpreter.py
#1254 (comment):

@@ -271,6 +280,8 @@ def _load(self, gcodeFile, printer_profile, throttle=None):
def _parseCuraProfileString(self, comment, prefix):
return {key: value for (key, value) in map(lambda x: x.split("=", 1), zlib.decompress(base64.b64decode(comment[len(prefix):])).split("\b"))}

  • def getWarning(self):

This is not Java, we don't need getters for everything :) (Yes, I know,
I've done that myself, I'm trying to get rid of such things while
refactoring)


Reply to this email directly or view it on GitHub
https://github.com/foosel/OctoPrint/pull/1254/files#r55028146.

This comment has been minimized.

Copy link
@foosel

foosel Mar 4, 2016

Owner

I think that probably was rather meant for the "this is a bit backwards" comment further up? But yes, you are right.. I'll add a comment up there.

@@ -501,8 +512,16 @@ $(function() {
self.uploadSdButton = $("#gcode_upload_sd");
if (!self.uploadSdButton.length) {
self.uploadSdButton = undefined;
if (_.endsWith(filename.toLowerCase(), ".stl")) {

This comment has been minimized.

Copy link
@foosel

foosel Mar 4, 2016

Owner

What does this block have to do with warning about a model exceeding the print area? Merge error? Please remove :)

if(self.loginState.isUser()&&data["gcodeAnalysis"]["warning"])
{
var warning = "<p>" + gettext("Object exceeds printing area") + "</p>";
warning += pnotifyAdditionalInfo("<pre>Object or objects in file '"+data["name"]+"' exceed/s the printing area</pre>");

This comment has been minimized.

Copy link
@foosel

foosel Mar 4, 2016

Owner

Do we really need the additional information here? Does it really add anything? The file name could be included in the initial message (use sprintf so that the translation is still generic and only contains a placeholder for the filename, there are examples throughout the code on how to do that), and the rest of the message is just repetition really. It might make sense to include information which axes exceed the build volume (x, y or z, or maybe more than one?).

result = dict()
result["warning"]=self._gcode.getWarning()

This comment has been minimized.

Copy link
@foosel

foosel Mar 4, 2016

Owner

This is where the min/max coordinates of the object should instead be transferred :)

@@ -17,6 +17,8 @@

import octoprint.util.gcodeInterpreter as gcodeInterpreter

#Added to check if object exceeds printing area
from octoprint.printer.profile import PrinterProfileManager

This comment has been minimized.

Copy link
@foosel

foosel Mar 4, 2016

Owner

Shouldn't actually be needed if you change to the suggested approach of instead tracking the min/max coordinates of the object and then utilize that information in the frontend to display the warning instead. Also, including this here without accounting for it in the unit tests breaks those, which is why your PR is currently marked by Travis as broken.

@foosel

This comment has been minimized.

Copy link
Owner

commented Mar 4, 2016

I added a couple of review comments, please take a look at those.

About the number of commits - yes, this is how git works. Every commit references one or more parent commits, you have a full fledged graph. Since PRs are basically just "merge my graph into your graph please" situations, your full graph gets checked against my full graph and everything you added lands in the PR. You could have fixed that before hand by interactively rebasing your branch and changing your commit history to only contain the small number of changes that are actually necessary here, but I can also do that after the fact by just squashing your commits instead of merging, which is probably the easier method now.

But first - see the review comments :)


Unasked for advice follows ;)

I would suggest you take a look into how git works so you understand why this is happening and how to avoid it from happening in the future - it's a bit difficult to grasp when just learning how to use git and confuses a lot of people, but it's actually not that tricky to understand as long as one takes a bit time to wrap the head around the concepts that are at work in git and are actually quite cool :) This is a good resource for really understanding what is going on. I can hear you say "I need to read a BOOK to understand git?" and yes, I know, that sounds intimidating, but a) it's really short and b) the thing with git is that if you only know the handful of commits that most tutorials teach you, you might know how to do something, but not why, and most importantly when to not do it, so I can only really really recommend to tackle the whole git thing by trying to understand how it works, because then all those various commands and what they do behind the scenes and why for example you see the commits in this here PR you see there makes complete sense.

Further interesting resources: this short article on the git data model and this very video about the whole topic (not exactly the one I wanted to link but seems to cover the topic just as well)

@Javierma

This comment has been minimized.

Copy link
Author

commented Mar 8, 2016

It took some time but I would say that it is finished or almost. About the different notes:

-Minimum and maximum values are now sent through the metadada to be used in the frontend.

-getWarning(): Removed in this last commit (yes, I wrongly applied Java programming).

-Print origin: In last commit it has been tested for both lowerleft and centre, working for the two cases (maybe for centre origin the message shown can be improved).

-Start and end gcode: I agree that it also should be evaluated, although it can be quite difficult indeed. I may be wrong, but as far as I know G28 command should be used to say which axis do the homing, so in case of extrusion in start gcode(e.g. for skirt) a G0 or G1 command would be used.

-Resources: Before these resources I already did online courses (CodeCademy's and Udacity's) but those are not as complete (especially to repair problems as are pretty basic), so thank you for them.

-Files.js: There where things that I probably changed when I didn't have to, so that's why they appeared as changed. In the last commit you will find some of the things that I had to repair. Now that problems are solved.

-GUI: I also changed the way that the message appears, trying to be clear and avoid absurd repetitions. The following image is an example of the change:

screenshot from 2016-03-08 18 38 08

@@ -262,7 +260,7 @@ def throttle():
self._gcode.load(self._current.absolute_path, self._current.printer_profile, throttle=throttle_callback)

result = dict()
result["warning"]=self._gcode.getWarning()
result["printingArea"]={"minX" : self._gcode.minX, "minY" : self._gcode.minY, "minZ" : self._gcode.minZ, "maxX" : self._gcode.maxX, "maxY" : self._gcode.maxY, "maxZ" : self._gcode.maxZ}

This comment has been minimized.

Copy link
@foosel

foosel Apr 11, 2016

Owner

Missing white space in assignment, and shouldn't all be on one line - line breaks and white space in general are a GOOD thing to have in code ;)

@@ -6,6 +6,7 @@ $(function() {
self.loginState = parameters[1];
self.printerState = parameters[2];
self.slicing = parameters[3];
self.printerProfiles=parameters[4];

This comment has been minimized.

Copy link
@foosel

foosel Apr 11, 2016

Owner

Indentation

}
}
}
if(warn)

This comment has been minimized.

Copy link
@foosel

foosel Apr 11, 2016

Owner

Ufff... Yes, that does what it's supposed to do so, but it is way too repetitive and as such highly error prone in case of future adaption/bug fixing.

Take a closer look at your if conditions. Most of them start with either testing for origin type lowerleft vs. center.

So we can extract those tests already (also, please take more care of the code formatting - space between if and condition, also within expressions, starting brackets on the same line, consistent indentation):

if (volumeInfo.origin == "lowerleft") {
    if (printingArea["maxX"] > volumeInfo.width) {
        // ...
    }
    if (printingArea["maxY"] > volumeInfo.depth) {
        // ...
    }
} else if (volumeInfo.origin == "center") {
    if (printingArea["maxX"] > (volumeInfo.width / 2)) {
        // ...
    }
    if (printingArea["minX"] < (-volumeInfo.width / 2)) {
        // ...
    }
    if (printingArea["maxY"] > (volumeInfo.depth / 2)) {
        // ...
    }
    if (printingArea["minY"] < (-volumeInfo.depth / 2)) {
        // ...
    }
}

if (printingArea["maxZ"] > volumeInfo.height) {
    // ...
}

Now, take a look at the two origin-controlled blocks. Still look quite similar. By extracting the min/max values of the volume into custom vars we can make the check even easier:

// set print volume boundaries
var boundaries = {
    minX = 0,
    maxX = volumeInfo.width,
    minY = 0,
    maxY = volumeInfo.depth,
    minZ = 0,
    maxZ = volumeInfo.height
};
if (volumeInfo.origin == "center") {
    boundaries["maxX"] = volumeInfo.width / 2;
    boundaries["minX"] = -1 * boundaries["maxX"];
    boundaries["maxY"] = volumeInfo.depth / 2;
    boundaries["minY"] = -1 * boundaries["maxY"];
}

// find exceeded dimensions
if (printingArea["minX"] < boundaries["minX"] || printingArea["maxX"] > boundaries["maxX"]) {
    info += "Profile's width: (" + boundaries["minX"] + ", " + boundaries["maxX"] + ") vs object's width: (" + printingArea["minX"] + ", " + printingArea["maxX"] + ")\n";
}
if (printingArea["minY"] < boundaries["minY"] || printingArea["maxY"] > boundaries["maxY"]) {
    info += "Profile's depth: (" + boundaries["minY"] + ", " + boundaries["maxY"] + ") vs object's depth: (" + printingArea["minY"] + ", " + printingArea["maxY"] + ")\n";
}
if (printingArea["minZ"] < boundaries["minZ"] || printingArea["maxZ"] > boundaries["maxZ"]) {
    info += "Profile's height: (" + boundaries["minZ"] + ", " + boundaries["maxZ"] + ") vs object's width: (" + printingArea["minZ"] + ", " + printingArea["maxZ"] + ")\n";
}

This comment has been minimized.

Copy link
@Javierma

Javierma Apr 11, 2016

Author

I agree this way is much better than the improvement I applied to the original code.

#Add value to list in case move includes extrusion
if e is not None and (e-previousE)>0:
if x is not None:
xList.append(x)

This comment has been minimized.

Copy link
@foosel

foosel Apr 11, 2016

Owner

Using lists for this is very risky. You are basically creating a full representation of the model in memory, which depending on the size of the model can be too much for the hardware you are running on (Pi1). At the very least you should be using a set here, which makes sure to not keep duplicates, but even that has the risk of causing memory issues.

You are only interested in the min and max values here, so just track those on the go:

min_x = None
min_y = None
min_z = None
max_x = None
max_y = None
max_z = None

# ...

if e is not None and (e - previousE) > 0:
    min_x = x if min_x is None or x < min_x else min_x
    max_x = x if max_x is None or x > max_x else max_x
    min_y = y if min_y is None or y < min_y else min_y
    max_y = y if max_y is None or y > max_y else max_y
    previousE = e

if z is not None:
    min_z = z if min_z is None or z < min_z else min_z
    max_z = z if max_z is None or z > max_z else max_z

# ...

self.minX = min_x
self.maxX = max_x
#...

This comment has been minimized.

Copy link
@Javierma

Javierma Apr 11, 2016

Author

Good to know. I did something similar in a first version but I thought this would be done more slow and spending more memory.

@@ -454,35 +454,44 @@ $(function() {
}
if(warn)
{
var warning = "<p>" + gettext("Revise file ")+ data["name"] + "</p>";
var warning = "<p>" + _.sprintf(gettext("Revise file %s"), data["name"]) + "</p>";

This comment has been minimized.

Copy link
@foosel

foosel Apr 11, 2016

Owner

Ah... yeah... I should have taken a look into that commit first. I see you improved the structure, also used sprintf, good idea!

The streamlining by extracting the boundaries of the volume first, THEN calculating if it is exceeded or not still would make things cleaner.

And you really need to fix your indentation levels, bracket positioning and white space use. Take a look at how the code looks:

image

That seriously breaks readability :/ Good white space placement is just as important as that the code functions in the first place, because if it takes you minutes to even understand the structure of a piece of code every time you come across it, maintaining it becomes a chore. And code that is hard to maintain is code that quickly becomes buggy and/or messy in other ways. Always imagine that you'll need to open up the file you are working on in a year, with a headache and under time pressure and figure out why something in there doesn't work as expected. And then code so that this future self of you will not want to kill current you with a baseball bat in retrospect.

@foosel

This comment has been minimized.

Copy link
Owner

commented Apr 11, 2016

Sorry that it took me so long to get back to this, I'm having my hands full currently.

I added a couple of comments. Also take a look at those on the "outdated diff" (I realized too late that there were two commits, sorry for that), since they are of general importance.

I realized that this is something you did as part of your coursework for university, and if you do not have the time to look into this anymore, please just say so, I'll try to find the time to pound the PR into shape myself then. Sorry again for the long delays at the moment.

@foosel foosel removed the pr:needs review label Apr 11, 2016

@Javierma

This comment has been minimized.

Copy link
Author

commented Apr 11, 2016

No need to apologize, I understand that you can be busy. Thanks for your patience indeed.

Indeed this feature is part of my final degree project, so I would like to finish it if possible. During the time I was working in reading the book you recommended me to understand git better as well as working in other features while trying to meet the recommendations (sorry if it took too much time to apply fixes in this one).

@Javierma Javierma force-pushed the Javierma:area_warning branch from b345fe2 to 95b6eba Apr 11, 2016

@foosel

This comment has been minimized.

Copy link
Owner

commented May 9, 2016

Again sorry for the delay, some tricky problems are keeping my hands awfully full.

I just took another look at the code, and there are still some comments unaddressed:

Could you take another look at those? If not, please tell me, I'll then try to find the time (... somewhere ...) to tackle that myself.

@gddeen

This comment has been minimized.

Copy link

commented May 10, 2016

I have dual extruders. The X/Y offsets of the 2nd extruder is given to the Marlin firmware. The settings
for Cura are then to have each extruder at 0,0.

This makes things kinda complicated. I can't print from X=0 to 27 with the 2nd nozzle. I can't get to MaxX + 27 with the 1st nozzle...

Just in case this handles the dual extruders...

@Javierma

This comment has been minimized.

Copy link
Author

commented May 11, 2016

@foosel If I'm not wrong, in the last commit (b0cfa28) the overly complicated if structure was changed to your suggestion as it was a much better approach. The other issue wasn't a merge error. It was indeed a line or part of it that I accidentally removed so I had to put it again to solve the problem.

@gddeen Good question. The printers that I use have a single extruder and by mistake didn't thought about having two. Could you send me a gcode file to martinezarrietajavier@gmail.com to check if it would work or not?

@foosel

This comment has been minimized.

Copy link
Owner

commented May 12, 2016

@Javierma sorry, I got confused by Github's presentation of the diff there, you are right.

@foosel

This comment has been minimized.

Copy link
Owner

commented May 12, 2016

About the dual extrusion, if the offset is handled by the firmware there's nothing you can do to properly do an out-of-bounds check from within OctoPrint since there's no way for OctoPrint to know if an extruder is stepping off the bed without knowing its offset from the head origin.

Dual extruders are however something that the min/max calculation in the gcode interpreter in this PR currently doesn't take care of and that needs to be fixed, so big thanks @gddeen for the heads-up, I managed to completely lose sight of that one too.

It can be easily achieved however by moving the min/max calculation for the x and y coordinates a bit further below after pos gets fully updated based on the current G0/G1 command and using pos instead of the coordinate values from the command.

That should actually be done in any case, since it also takes care of absolute vs. relative positioning, absolute vs. relative extrusion and metric vs. imperial coordinates.

So, in a nutshell: Move the min/max block to the end of the G0/G1 if instead of the beginning and modify it to use the pos data, like this:

# If move includes extrusion, calculate new min/max coordinates of model
if e > 0.0:
    self.minX = pos[0] if self.minX is None or pos[0] < self.minX else self.minX
    self.maxX = pos[0] if self.maxX is None or pos[0] > self.maxX else self.maxX
    self.minY = pos[1] if self.minY is None or pos[1] < self.minY else self.minY
    self.maxY = pos[1] if self.maxY is None or pos[1] > self.maxY else self.maxY
    self.minZ = pos[2] if self.minZ is None or pos[2] < self.minZ else self.minZ
    self.maxZ = pos[2] if self.maxZ is None or pos[2] > self.maxZ else self.maxZ

Note that e at that point will have been converted to relative coordinates or set to 0.0 if it was None so a simple check if it's greater than 0 is sufficient to find an actual extruding (and not retracting) move*. Using that post-processed e value will also have the extra benefit that it takes multiple extruders and their previous E value into account. The previousE variable hence isn't necessary anymore and can be removed.

  • That might have to be adjusted for retract-and-lift slicer settings, but let's ignore that for now.
@Javierma

This comment has been minimized.

Copy link
Author

commented May 12, 2016

Analyzing the gcode sent by @gddeen (thanks for your help by the way), it will partially work for dual extruders. What I mean is that if any of them is going to print outside the bed or general printing area (not extruder printing area) it will warn the user. Problem is that it will not be known which extruder does it without thorough research by the user, and in case of extruder change it may warn when it should not.

@Javierma

This comment has been minimized.

Copy link
Author

commented May 12, 2016

@foosel Change done

@foosel

This comment has been minimized.

Copy link
Owner

commented May 12, 2016

Javierma87
@Javierma

This comment has been minimized.

Copy link
Author

commented May 12, 2016

@foosel Sorry, forgot to remove that one

@foosel

This comment has been minimized.

Copy link
Owner

commented May 12, 2016

@Javierma ok, I'll take another look and see if I can get that squashed and merged. Will probably not get around to do this until monday or so though, just so you know. Thank you for your patience with my nitpicking :)

@foosel

This comment has been minimized.

Copy link
Owner

commented May 19, 2016

@Javierma squashed, slightly adjusted and merged to the devel branch.

I had to change the behaviour slightly. What I didn't realize during review but once I went through the squashing was that it would have triggered a message for each file not fitting the current print volume on each reload. That's not that great of a user experience. I now adjusted it in such a way that it triggers the message when you try to select or select&print such a file. I also added the model dimensions to the additional data output.

Thank you again for the contribution and sorry that it took so long to get it integrated. And good luck with your coursework! :)

@foosel foosel closed this May 19, 2016

@Javierma

This comment has been minimized.

Copy link
Author

commented May 19, 2016

Great, and thanks a lot for your help

@Javierma Javierma deleted the Javierma:area_warning branch May 28, 2016

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.