GCode Reader
AcceptedPublic

Authored by nickthetait on Nov 9 2016, 7:54 AM.

Details

Reviewers
kakaroto
Summary

Seeking a thorough code review of the GCode Reader plugin.

Test Plan

Follow list of requirements in T466

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
There are a very large number of changes, so older changes are hidden. Show Older Changes
nallath added inline comments.Nov 18 2016, 2:01 AM
cura/CuraApplication.py
522

I think you should use a decorator for this. We have sceneNodeDecorators that are perfect for this.

See per-object settings and the group decorator. The decorator in itself would be fairly simple; It needs to have a setGcode and a getGcode.

Checking if a node had a gcode would then be; node.callDecoration("getGcode"). If it has a g-code, it will return the code and None if it doesn't have the decorator.

1003

Codestyle; In some cases you start signal name with a capital, others lowercase.

1005

It's not required, but using a proper decorator is nicer in my opinion.

plugins/CuraEngineBackend/CuraEngineBackend.py
72

Codestyle; Should be _pause_slicing

plugins/GCodeReader/GCodeReader.py
25

Some class documentation would be nice. How is this ment to be used? What does it do.
I know, but that doesn't mean that everyone does.

28

just calling super().init() will work just fine. There is no multiple inheritance going on here.

31

Is cancelled supposed to be publicly accesible? if not, please add the _

62

I prefer to type out variables in functions (especially because python allows you to use those in kwargs). It's better to change this to message instead of m.

70

You're touching the privates of the backend. This is not allowed!

83

why is extruder_offsets plural? It's a single offset right?

also; Documentation. What do all the variables mean and what are the types?

103

The line width should definitely be calculated. 0.5 for instance will be wrong for all UM printers.

If I recall correctly, cura 15.04 does calculate the width.

126

Just because we put it in at some point, doesn't make it right ;) OBJ reader never got a whole lot of love from us, as it's hardly used.

133

Some documentation here would be nice. You're overriding a function, so that warants a bit of explanation.

140

Don't call signals with hardcoded values of the enum

142

Old code that was never fixed. gcode_list is not always set, which will give an attribute error.

As I explained before; this should be done with a decorator. See one of my earlier comments.

147

Use: with open(file_name, "r") as f:

This ensures that if anything fails, the open is closed correctly.

156

Its a layer_data_builder, not a layer_data

179

codestyle, spaces arround operators.

240

Remove this print.

Thanks @nallath for your input. I had missed a few things. What do you think of my earlier comments/suggestions for how to refactor things. Does it make sense for you to change SceneNode the way I suggested ? And do you think using the SceneNode.enabled == False would be enough to represent a pre-sliced node ?

cura/CuraApplication.py
522

The gcode field here is just a boolean to say if it's a gcode type or not. It doesn't actually return the gcode content.
I've already commented (https://code.alephobjects.com/D5#65) that the main code shouldn't be looking at the gcode field at all, but instead the SceneNode would have a different kind of attribute to indicate that the node cannot be sliced and the main cura code would use that to decide what to do.

plugins/GCodeReader/GCodeReader.py
142

Using getattr does not hide the attribute error, do a getattr on a non existing attribute, it still gives an exception :

In [3]: getattr(os, 'pathd')
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-3-debabbba0bab> in <module>()
----> 1 getattr(os, 'pathd')

AttributeError: 'module' object has no attribute 'pathd'

It would only be useful for avoiding an exception if the 3rd argument was given (default value to return in case attribute doesn't exist).

240

The whole 'elif G== 10' section was removed in the latest code. That's also why I didn't review this part.

278

ok, that makes more sense if the is_machine_center_zero is enabled. In the case of the mini/taz, it shouldn't be set.
@nallath , if it's set, but the gcode was built for a machine that didn't have it set, would that make any difference ? I mean, does that property represents how the gcode is to be generated, or does it represent how the firmware itself defines its coordinate system ?

resources/qml/Menus/RecentFilesMenu.qml
29

That was because he needed to change how files get loaded. You can see the implementation for readLocalFile. But I think that it shouldn't be necessary if the changes I suggested are implemented. If any changes are still needed, then they could be done in UM directly in that case.

In D5#159, @kakaroto wrote:

Thanks @nallath for your input. I had missed a few things. What do you think of my earlier comments/suggestions for how to refactor things. Does it make sense for you to change SceneNode the way I suggested ? And do you think using the SceneNode.enabled == False would be enough to represent a pre-sliced node ?

I agree with most of your suggestions, but I don't think we should change the SceneNode. The scene node lives in Uranium and therefore has no knowledge of something like "g-code" existing at all. This could be fixed by using a NodeDecorator.

plugins/GCodeReader/GCodeReader.py
79

If use (0, 0, 0) to (0, 0, 0) it will cause DivideByZero, If use (0, 0, 0) to (1, 1, 1) it will automatically scale model, because it is too small

It is interesting to see what kind of things this exposes about our APIs. If you come across things that make little sense, please document them and (ideally) make issues about them. Otherwise we will just carry around workarounds for everything.

cura/CuraApplication.py
540

The signal solution is the right one, however I would use changeLayerViewSignal = Signal(Signal.Queued)to enforce this always happens on the next run of the event loop. That way it is more clear what is going to happen and you can be sure the behaviour is always the same. The default signal type (Signal.Auto) will only push an event when it is not running on the main loop, which may result in different behaviour depending on the calling context.

731

One thing I have thought about for a while is to make a "SlicableObject" decorator that can be used to indicate objects that can be sliced. This is probably the right solution here since it would allow for Cura-specific information to be included. Such a decorator would also enable the logic mentioned in the global comment about not checking for G-Code specific behaviour. It would allow you to have an "isSliceable" method and a "shouldBlockSlicing" method. Things like the build platform would not have this decorator so they will effectively return "False" for both methods, whereas an object that is not sliceable but should block slicing would need this decorator but can explicitly return "False" for isSlicable and "True" for shouldBlockSlicing.

1005

Note that there used to be a bug where @property.setter would not actually register the proper setter. I worked around this by explicitly including the method in the property decorator, so it becomes @pyqtProperty(bool, fset = setHideSettings, notify = hideSettingsChanged). I am not sure if this bug is still applicable with PyQt 5.6.

cura/PrintInformation.py
72

I do not think this needs a setter. I cannot think of a situation where I would want to set this from QML.

137

It is better to not do anything here and limit the TextField that is used from QML to a certain maximum name length. In addition, the text field can automatically add the ellipsis without needing to modify the source string.

139

Actually, i18nc() and the other i18n functions already perform a call to format() so you can use i18nc("@label", "Pre-sliced file {0}", base_name)

plugins/CuraEngineBackend/CuraEngineBackend.py
72

Additionally, why are you adding an extra property for this when we also have _enabled? What does this property do extra?

plugins/GCodeReader/GCodeReader.py
28

Actually, using super(Class, instance) is wrong in Python 3. For cooperative multi-inheritance to work, you must call super() without arguments. See https://docs.python.org/3/tutorial/classes.html#multiple-inheritance

143

If you are clearing the list of G-Code anyway, why bother retrieving the previous value? Just overwrite the property with the new value.

173

This entire loop is pretty hard to follow. I would recommend splitting it up into a bunch of different functions to handle the different G-Code commands. In fact, if you use a hash of (Gcode, function) it would make it a lot simpler to add support for additional G-Code commands later on.

278

It determines whether the firmware expects coordinates to be relative to the front-left corner or to the build plate centre. CuraEngine generates different G-Code based on this property. This is a pretty tricky thing to get right, I do not really know the expected behaviour here.

284

More specifically, this should be done by the LayerView plugin itself. I believe it already handles this properly, since it listens to parentChanged signals. But I may be wrong.

resources/qml/SaveButton.qml
17

It would be better to use a different backend state for this rather than yet-another property.

nickthetait requested a review of this revision.Nov 21 2016, 3:10 PM
nickthetait edited edge metadata.
nickthetait marked 2 inline comments as done.

The code review is now complete. Go ahead and start working on this refactoring Victor. Include D5 in all commit messages related to code review. Additionally, please standardize all user-visible strings to use G-code.

To keep track of what has been completed, mark each comment as 'done' after you have pushed changes to address it.

resources/qml/Sidebar.qml
190

I have asked marketing for ideas on better wording for this case and the other user-visible text labels.

plugins/CuraEngineBackend/CuraEngineBackend.py
72

_enabled is toggled in another place, so user would enable slicing

cura/CuraApplication.py
731

If we add "SlicableObject" decorator, we need to add it to any model in scene. Maybe need add inverted "NonSlicableObject" decorator, to indicate only gcode nodes?

plugins/GCodeReader/GCodeReader.py
118

If remove this line the cura crashes.

plugins/CuraEngineBackend/ProcessSlicedLayersJob.py
61

But It's still needed, without it scene not clearing as needed

plugins/GCodeReader/GCodeReader.py
103

In cura original I can't see in gcode interpreter any code that calculates line width.

173

This functions would modify local variables, it may cause problems.

nickthetait marked 2 inline comments as done.Nov 23 2016, 10:40 AM
nickthetait added inline comments.
plugins/GCodeReader/GCodeReader.py
103

I agree, this is not an existing capability.

173

Start with extracting everything inside the if G is not None: condition (I believe lines 185 to 244) into a separate function. It could be called processGcodeCommand.
After this refactoring it should be easier to see how to split into smaller functions.

179
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