GCodeReader(Uranium)
AcceptedPublic

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

Details

Reviewers
kakaroto
Test Plan

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
victor_larchenko retitled this revision from to GCodeReader(Uranium).
victor_larchenko updated this object.
victor_larchenko edited the test plan for this revision. (Show Details)
victor_larchenko edited the test plan for this revision. (Show Details)Dec 4 2016, 9:43 AM
victor_larchenko added a reviewer: kakaroto.
victor_larchenko set the repository for this revision to rCT CuraLE.
victor_larchenko changed the repository for this revision from rCT CuraLE to rU Uranium.
victor_larchenko added a subscriber: nickthetait.

Uranium piece looks pretty good to me.

kakaroto edited edge metadata.EditedDec 5 2016, 2:16 PM

My only and big issue with the changes here is that the .gcode and .g file extensions are hardcoded here, UM should be independent on the type of files that are being loaded. I would much rather see it load all the files, then once the first file got loaded, it can check if it's sliceable or not, if it's not, then it would cancel all the other jobs.
Apart from that, the rest looks good.

EDIT: considering that the gcode_list variable of the scene gets modified by the gcode loader, and because loading files will affect the user with the popup notification with progress bar, then I guess we can't just load all the files and then cancel the rest, as they might conflict with one another.

nallath added a subscriber: nallath.EditedDec 6 2016, 1:50 AM

I'm rather against putting the slicableObjectDecorator in Uranium. Uranium does not know that slicing is a thing at all. This is a Cura specific notion, so it should "live" there as well. It also makes adding new readers a lot trickier, as anyone who makes a new reader should now tell the application that it can be sliced. The default should be imho that all loaded objects can be sliced, except g-codes (so it makes way more sense to add a single g-code exception).

If you add a sceneNodeHelper decorator for instance, you could mark a node as a "helper" node of sorts, indicating that it should not be sliced. Another option would be to simple set the enabled property of the node to false and only slice enabled nodes.

Another option would be to have g-code decorator. If a node has a getGCode or getLayerData function, don't render it as normal. The layerview should handle that. This has the advantage of being much closer to what we already did (using the _hasattr)

I also agree with others that the "non slicable extension" bit is really weird. Uranium has no notion of this! Let the g-code reader handle this.

I understand more why it is worthwhile to move slicableObjectDecorator out of Uranium.

From nallath's comment there are 3 different designs to pick from:
A) sceneNodeHelper decorator
B) Use the enabled property of the node
C) G-Code decorator (think this is my preferred approach)

@victor_larchenko, of these three approaches which do you see as more beneficial? Or can you think of another way to restructure the code so that Uranium does not contain any 3D printing specific concepts?

When we done first review, I wrote about "NonSliceableObjectDecorator" (the same as sceneNodeHelper), but @kakaroto told me to use a way that have been written by @awhiemstra (SliceableObjectDecorator), so now I'm confused what is the best way. The other two, I think are incorrect, because G-code decorator is depended on gcode and not universal, and enabled property is used for another.
The second problem is if Uranium shouldn't know about slicing we need to move file loading part back to Cura, but I think this is not fully correct.

I don't like moving the plugins out of Cura, because most file readers are "agnostic" (the files that they load are not 3D printing specific). STL is a perfectly valid 3d format for other solutions (notably; The Scanner prototype that we have also uses Uranium and also uses the STL loader)

NonSlicable or ignoreSlicing or something like that could work. Another option could be to have a "layerData" decorator, so you don't even need to have a node with meshData (so the node won't be sliced in the first place)

UM/Qt/Bindings/MeshFileHandlerProxy.py
83

Didn't the user in this case expliclty ask for another file to be loaded (and thus, replaced?)

There is no need to have all this code in Uranium. All of this can (and should) be handled in Cura. Moreover, making the reader plugins responsible for adding the SliceableObject decorator is putting the repsonsibility in the wrong place. Right now, it results in the build platform being marked as slicable, for example. This greatly reduces the usefulness of the decorator as my intention with it was to replace all the if type(sceneNode) == SceneNode and sceneNode.getMeshData() and similar constructs.

Most of this code can in fact be moved to CuraApplication::_onJobFinished or a similar location. This is also a better place to make the decision whether or not to add the decorator. Most likely, you only want to add the decorator for objects that get loaded through the "Open File" dialog, as any other mesh loading is going to be programmatic, meaning those objects are unlikely to be slicable. If you need to tweak what the decorator does based on file type, check what file was loaded and act on that. If you want to make it really elegant, you add the information about blocking slicing to the plugin's metadata and query that. That's a bit more work though that could be done in a follow up.

Using metadata would also be the right approach for the current filtering on extension. For now, I would move this to a new function to open files in Cura. This would perform the filtering and could even do the decision making about adding a decorator or not, though that would make it harder to reuse the generic loading code. It would also be able to perform the switch to layer view, which is now done in the wrong place.

UM/Backend/Backend.py
28

Note that Uranium knows nothing about slicing, so this should just be "Disabled".

UM/Qt/Bindings/MeshFileHandlerProxy.py
117

This signal does not exist in Uranium, making this an error if some other application happens to touch this code.

victor_larchenko marked 2 inline comments as done.Dec 9 2016, 2:11 AM
victor_larchenko added inline comments.
UM/Qt/Bindings/MeshFileHandlerProxy.py
83

User can open only one file in this case, so it will load normally.

victor_larchenko edited edge metadata.
victor_larchenko removed rU Uranium as the repository for this revision.
victor_larchenko marked 2 inline comments as done.Dec 9 2016, 2:50 AM
kakaroto accepted this revision.Jan 30 2017, 1:35 PM
kakaroto edited edge metadata.

Marking as accepted so it's removed from my tasks to do.

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