Authored by victor_larchenko on Dec 4 2016, 9:38 AM.


Test Plan

Diff Detail

Lint Skipped
Unit Tests Skipped
There are a very large number of changes, so older changes are hidden. Show Older Changes
awhiemstra added inline comments.Dec 7 2016, 5:51 AM

In addition to the below comment, this makes this property a class variable while it should be an instance variable.


Since you add the "pre-sliced" property to print information, there is no need for this property since you can just hide the settings if pre-sliced is true. Should there ever be a need to extend that condition we can always look at it then. But in my opinion, the decision to show or hide the settings should be a UI level decision using other properties from the lower level (like pre-sliced).


This runs the risk of ending up with "Pre-sliced file Pre-sliced file Pre-sliced file Pre-sliced file filename.gcode" filenames. I do not really see a reason to add this and instead would just use the original filename.


I would not make the decision based on if there is a non-slicable object in the scene, since that would lead to no slicing after layer view is generated. However, I agree that the two concepts do not necessarily combine into the same class. Should you have a global place to add the decorators (like I mentioned in the Uranium review) you could actually completely remove this property and make the general "handle loaded object" method deal with enabling/disabling slicing.


This "Pre-sliced" label is only shown at the UI level. It is not actually added into the filename itself, so this is a non-issue.


If you inspected a log that says "started parsing file" and never found an end/completion it would seem like a serious problem. So I say leave this in.


That would be nice to have. We'll leave this for a separate task on another day...

awhiemstra added inline comments.Dec 7 2016, 10:31 AM

Oh, fair enough. In that case it should be fine.


I don't mind logging it, I mostly meant the warning level. Guess I should have made that clearer.

kakaroto added inline comments.Dec 7 2016, 12:35 PM

Oh, named tuples! That's new in python 3, didn't know about them. That is indeed a very good idea to use them here, I second that!


since the sidebar is used for the print monitor, I would assume we want it to stay there. Maybe it would be better to auto-switch to print monitor sidebar instead of print setup, but I don't think removing the sidebar entirely is needed.

nickthetait added inline comments.Dec 7 2016, 2:12 PM

Gotcha. Yeah, let's lower the severity level on this message.


That is a great idea to auto-switch to print monitor! I've made a task for this T597. Since this is not something we will implement as part of this code review, victor go ahead and mark these four comments as "done".

victor_larchenko marked 7 inline comments as done.Dec 8 2016, 9:58 AM
victor_larchenko marked 11 inline comments as done.Dec 9 2016, 1:48 AM

@awhiemstra, If we will make this decision when file is loading, it will break undo\redo system, so I think need only change decorator

victor_larchenko edited edge metadata.
victor_larchenko removed rCT CuraLE as the repository for this revision.
victor_larchenko marked 3 inline comments as done.

I'm Changed the decorator, but now it have one problem: if add this decorator only to sliceable objects then cura represents buildplate as gcode node, so we need add this decorator as third state(isSliceable = False) to non-sliceable nodes or have NonSliceable decorator.

I'm Changed the decorator, but now it have one problem: if add this decorator only to sliceable objects then cura represents buildplate as gcode node, so we need add this decorator as third state(isSliceable = False) to non-sliceable nodes or have NonSliceable decorator.

I think you still need the shouldBlockSlicing property on the decorator. Otherwise it becomes impossible to determine whether or not to block slicing, since having a non-sliceable object in the scene should not block slicing. Note that my original suggestion was to move it to the loading method, not to remove the functionality completely. Your comment that it breaks undo/redo is a good one and thus we should keep the functionality in the decorator.


Please pull the implementation of changeToLayerView into this or make changeToLayerView a private method that you call directly. There is no need for the additional signal indirection.

awhiemstra added inline comments.Dec 12 2016, 3:38 AM

For a follow up, we should move this logic to the CuraEngineBackend class, since it can do the same lookup perfectly fine and it is the backend's responsibility to check this.


This should not be a class variable.


Note that if you're going to modify this from external locations (like you do in the G-code reader) then you should add a getter and setter instead of accessing the variable directly. Or make it a proper property if you want to preserve the possibility to append/remove items directly.


Codestyle: This should be "getGCodeList"

victor_larchenko marked 3 inline comments as done.Dec 13 2016, 12:43 AM
victor_larchenko marked an inline comment as done.Dec 13 2016, 12:59 AM
kakaroto requested changes to this revision.Dec 16 2016, 10:59 AM
kakaroto edited edge metadata.

I've rebased all of this work on the ultimaker master and I am now doing a final testing/review phase. I have a couple of comments that would need to be fixed.
First thing is the _readMeshFinished receives a list of nodes from job.getResult(), so it needs to become :

nodes = job.getResult()
for node in nodes:

instead of :

node = job.getResult()
if node != None:

This is only valid for upstream master, so you will only need to remember doing this change (or fixing the conflict) once we merge the upstream master. Note that I've already modified this locally on my rebased branch, so this is mostly a FYI.

Another thing to think of when merging with upstream is that the Cura.qml now handles loading of .curaprofile files as well, so there are some changes there in how the drop.urls are handled. It's trivial though to fix any conflict that might happen, I just used a variable imported_model set to -1 that I set to the index in the drop.urls when a non .curaprofile is loaded, and then I just call the backgroundItem.hasMesh with it if imported_model is != -1.

Secondly, one issue I keep seeing is that the name of the job is wrong, if I load a gcode, it just shows the gcode name, if I then load an stl, it shows "pre-sliced file <stl filename>".. I checked, the PrintInformation receives the setPreSliced correctly, but I think that the job name gets set before pre-sliced gets set, so the job name will basically be dependent on whether the previous file that was loaded was a pre-sliced or not. You need to either find a way to give it the pre-sliced information before, or to update the job name once the pre-sliced property is changed.
I see that you've made it call the setPreSliced on the PrintInformation from the CuraEngineBackend, that's probably why it happens too late. Maybe you should just move that into the readMeshFinished instead, since you already check there for whether or not the extension is in the non_sliceable_extensions.

Another comment I have, is that if you load a gcode file that was not built for your machine, it shouldn't let you print it. More specifically, I've loaded some gcode file that was outside the bounds of the machine, see here :

This makes printing dangerous as it could (could it?) damage the printer. If we try an STL file that is too big to fit on the platform, then Cura will not let you slice/print it, I don't see why a similar security measure shouldn't be done for loading gcode files as well.
I would however not include this as an issue that prevents this work from being merged, I would rather see a new task/issue created to tackle this problem separately.

Another "to be fixed later/separetely" issue would be that all gcode files that get loaded have a 0g, 0m, 0:0:0min print information, I think cura exports that information in the gcode but I'm not sure, if not, then maybe the gcode parser can determine how many meter/grams it takes from the E position at the end of the print, and maybe estimate the print time as well (although that sounds more complicated to do).

The final issue that I see (and fixing it might help fix the non-printable/out-of-bounds gcode issue mentioned above) is that all gcode files have a 10x10x10mm stats, which I think is caused by the _getNullBoundingBox. I think that this can easily be fixed by keeping track of the max/min X/Y/Z values (only after the first layer starts extruding, to avoid start-gcode extrusion at extreme positions) and returning that min/max X/Y/Z as the bounding box instead of the static 0/0/0, 10/10/10 values that are currently hardcoded.


Not visible here but you have trailing whitespaces here in this empty line. I'm not sure about the coding style in Cura, but usually trailing whitespaces need to be removed.
(Doing a 'git diff' will show that line as red, and I use emacs with the 'show-trailing-whitespace' option enabled, so it's immediatly visible to me)


Here, shouldn't it be a not node.getMeshData() and not node.callDecoration("getLayerData") to make it more consistent ? If you're checking for actual data nodes, it's better to check for mesh data or layer data, instead of 'mesh data or whether or not the node blocks slicing" ?


Another trailing whitespace here.


Same here on using "getLayerData".


Another set of trailing whitespaces.


I would personally have chosen TYPE_KEYWORD as the variable name since it's a constant, it would have to be all-uppercase. But I don't know if the UM devs follow this same convention. Maybe @awhiemstra can answer that ?


I don't see a fix for this, even though you've marked it as Done.
I would suggest storing a dictinary of {extruder:position} whenever a switch is done, if a new extruder, e would be 0 I guess, and if not, you'd have the old value in the dictionary. processTCode would of course need to return the new current_position for the main, so the E value changes accordingly.

This revision now requires changes to proceed.Dec 16 2016, 10:59 AM
victor_larchenko requested a review of this revision.Dec 18 2016, 9:16 PM
victor_larchenko edited edge metadata.
victor_larchenko marked 5 inline comments as done.
victor_larchenko added inline comments.

It's fixed in _GCode* methods

kakaroto added inline comments.Dec 19 2016, 1:53 PM

Ah, yes, I see it. You made position.e into a list. The bug that I see with it is that it will only work for dual extruders, but since this gcode parser is machine-independent, it should work for machines with more than 2 extruders as well.
Init :

current_position = self._position(0, 0, 0, [0, 0])

Process T code :

if T is not None:
    current_position = self._processTCode(T, line, current_position, current_path)

and :

def _processTCode(self, T, line, position, path):
    self._extruder = T
    if self._extruder + 1 > len(position.e):
         position.e.extend([0] * (self._extruder - len(position.e) + 1))
    #  [etc..]
    return position
victor_larchenko marked 3 inline comments as done.Dec 20 2016, 1:13 AM
kakaroto accepted this revision.Jan 30 2017, 1:36 PM
kakaroto edited edge metadata.

Marking as accepted since this is done already.

This revision is now accepted and ready to land.Jan 30 2017, 1:36 PM