Cura: Request for code review of new feature: GCode Reader

[nickthetait]

For ~2 months Aleph Objects has been working on the ability to import and render GCode files in Cura. At this point the feature has matured to a usable level so we conducted an internal code review. I would like to hear Ultimaker's input before we start refactoring and cleanup.

Here is the code review page: https://code.alephobjects.com/D5.
Our tracking task which covers the history of design and implementation: https://code.alephobjects.com/T466
If you would like to give it a spin, checkout the GCODEReader branches of https://code.alephobjects.com/diffusion/CT/ and https://code.alephobjects.com/diffusion/U/

Screen shots
Reference Benchy model

Imported GCode

Inspecting some layers

Inspecting a single layer

Features

• Robust: It is more generic than Legacy Cura's implementation, allowing it to work with the output from Cura (Legacy and 2.x) and even Slic3r. It may cope with files produced by proprietary slicers, but this has not been tested.

• Performance: my non-scientific test shows Cura 2 (in a development environment) imports/renders ~2x faster than Legacy Cura (pre-built and installed) using a regular sized Benchy

• Drag & drop works

• Only one GCode file can be imported at a time

• Pause the slicer + explanation to user why GCode files can't be re-sliced

• Display filename in the usual place

• Probably others that I forgot :sleeping:

Known deficiency
This feature is based on a slightly out-dated (pre 2.3) codebase. Needs to be ported over to master branch.

Potential TODOs
Additional features could make it more useful. Many of these require some information which is not (yet) provided in the GCode file such as nozzle size, filament density, or E Steps.

• Beautification and integration with UI theme system :nail_care:
• Better support for multi-extrusion: there is currently no way to know the offset between nozzles so a Marvin in two colors looks like this:
  
• Use color to show different information like nozzle temperature, movement/acceleration speeds, or feature type (shell vs support vs infill)
• More accurate volumetric representation of extrusions
• Estimated print statistics:
  
  
  
  • Length of filament used
  
  
  • Total printed weight
  
  
  • Range of temperatures used by all nozzles or bed
  
  
• Calculate the center of mass: the viewport has no reference point for oreintation, so it is difficult to get exactly the position/angle desired

[Ghostkeeper]

Is nozzle offset actually written to g-code?

It's one of those things that may be a deal breaker for Ultimaker if it doesn't account for nozzle offset, with the pressure for dual extrusion. But it looks like it may need to be added to the Griffin header. If that's the case, it is not really your problem to tackle.

It's also a shame that it lost the colour coding for walls, support, travel moves, etc. I guess that may be a technical limitation with how it's creating one mesh with the same colour. That _is_ actually all written to g-code by Cura, though not as detailed as the normal layer view (e.g. there is no distinction between outer walls and inner walls if I recall correctly).

I think that our best course of action on Ultimaker's side right now would be to create an issue in our internal issue tracker review this, and request our team leader to give it some priority. Thanks for your great effort, Nick, Karoto and Victor!

[nallath]

We have an internal issue for this already @Ghostkeeper (see; CURA-1261)

[nallath]

I've made a large number of comments on this. I'm sorry to say, but it's going to need quite a lot of changes / fixes before i'm happy with this.

If you guys need help with how to handle certain issues, don't hesitate to contact any of us. You know how to reach us.

[nickthetait]

Dual extrusion
ghostkeeper: No, the nozzle offset(s) are not included. While using the griffin header may be an option, it is quite specific to one printer (UM3). How about something more generic?

Feature differentiation

It's also a shame that it lost the colour coding for walls, support, travel moves

Travel moves are displayed in a different color (currently white which is a little hard to see). Take another look at screenshots 3 and 4 above. This is determined by looking at changes in E### values instead of relying upon the tags that CuraEngine produces (such as ;layer:0 and ;TYPE:WALL-INNER). This allows it to handle slic3r output. I believe it would be possible to build a special case for gcode made by CuraEngine that can display this information.

Code review feedback
Nallath's comments are about codestyle/design/documentation problems. We should be able to resolve all of these concerns, except for extrusion width. I do not believe this can be calculated as the nozzle size is not published in the gcode file. If Legacy does have this feature, I can neither reproduce it nor find it. https://github.com/daid/LegacyCura/blob/SteamEngine/Cura/util/gcodeInterpreter.py

Spelling
What is the standard convention for displaying the text "gcode" to a user? I have seen many variants over the internet. (GCode GCODE g-code gcode...) What do we want to stick with in Cura?

Bonus
Just want to mention that it can show wire-printing too!


In this case the warning actually does not apply :laughing:

[Ghostkeeper]

Regarding the nozzle offset, I've talked with @daid about it and he said that we should eventually be working towards having no offset in the g-code but having the printer handle the offset. That fixes the problem generally (currently we have zero offset for other multi-extrusion printers). It also allows the g-code to be read by other applications such as RepetierHost or legacy Cura, as well as this new g-code viewer. I have no time window for when this could happen, though. I imagine some work will be needed in UM3's firmware. For CuraEngine the solution is quite simple (treat the offset as if it were 0), though Cura's front-end needs to treat the offsets as they are now with regard to the disallowed areas.

[awhiemstra]

@nickthetait My personal preference is to use either GCode or G-Code, with the latter being more correct according to my spell checker. Definitely not GCODE as it is like shouting it. ;)

One thing I would like to see is to completely disable solid/x-ray view when a g-code is loaded, since we cannot show anything useful in those views anyway. This probably requires some changes in Uranium though.

[nallath]

I'd leave the disabling of solid / x-ray view for now.

[awhiemstra]

Oh wait, looking at the actual code, you completely disable the View button. Which works just as well.

[nickthetait]

Seems like G-code is the most common/standardized. Going to roll with it.
http://reprap.org/wiki/G-code
https://en.wikipedia.org/wiki/3D_printing

[Drayson80]

just a short note to @Ghostkeeper 's "offset handle by printer". Please take care that other printers as well as "older" like UMO need to be supported too.... :-)

[nickthetait]

Ready for another round of reviews.
Okay @Ghostkeeper it can now detect/display feature types (wall/skirt/support), just like Legacy Cura.
https://code.alephobjects.com/D6
https://code.alephobjects.com/D7

[Ghostkeeper]

Nice! As you know, @nallath is assigned to review this stuff. You guys are keeping contact, I hear.

[nickthetait]

Thanks for the comments already nallath.

Here's a screenshot (taken by victor) of the feature detection currently:

[nickthetait]

Tremendous cleanup and redesign has gone on since this ticket was first opened. I believe we are ready to wrap this up. Would appreciate one last review by nallath

[nallath]

Nice work! As far as I'm concerned this can be moved into the main repo :)