Path: add File source to CustomOp #8892
Conversation
original functionality is the default behavior under Text Source
src/Mod/Path/Path/Op/Custom.py
Outdated
| gcode_file = self.findGcodeFile(obj.GcodeFile) | ||
|
|
||
| # could not determine the path | ||
| if not gcode_file: return |
There was a problem hiding this comment.
Probably want a console error for this scenario?
| prospective_path = os.path.join(doc_path, filename) | ||
|
|
||
| if os.path.exists(prospective_path): | ||
| return prospective_path | ||
|
|
||
| def opExecute(self, obj): |
There was a problem hiding this comment.
Is it possible to cache the commandlist so if the file is not found the operation is not wiped, but still shows a warning / error?
There was a problem hiding this comment.
You could probably dump the output of gcode_file into self.Gcode and keep the same commandlist code that existed before. That also allows someone to switch it to "Text" mode and edit the path.
There was a problem hiding this comment.
editing is an interesting idea but handling divergence worries me, what would your behavior look like if the user has made an edit, then the Op is recomputed?
There was a problem hiding this comment.
That is a fair point, it's 'source' instead of 'import'.
src/Mod/Path/Path/Op/Custom.py
Outdated
|
|
||
| with open(gcode_file) as fd: | ||
| for l in fd.readlines(): | ||
| newcommand = Path.Command(str(l)) |
There was a problem hiding this comment.
This worries me a bit, FreeCAD has a fairly strict definition on what GCode Ops are supported and in what format (line numbers/comments/etc...). When a custom op is normally created from a file, it uses the behavior in Post/scripts/gcode_pre.py to only include the "whitelisted" functionality that the post_processors can handle.
On the other hand, it is allowed to put any custom gcode in here regardless. It just may have some odd side effects.
I guess my main concern is that it differs from the "standard" import workflow.
There was a problem hiding this comment.
So far I've been approaching this with the goal of minimizing changes in default behavior so I just copied how it was done previously. I guess the bigger picture question is, should all internal gcode be parsed to this standard format?
This could also be put behind a Property switch to enable strict behavior without changing the default.
There was a problem hiding this comment.
It's a tricky thing to answer. Right now gcode_pre is very limited, but that's by design. You are correct that it's a bigger question of how we should handle this scenario in general.
CustomOp is currently loaded with foot-guns, maybe we keep that for now and don't worry about it here.
src/Mod/Path/Path/Op/Custom.py
Outdated
| obj.addProperty( | ||
| "App::PropertyStringList", | ||
| "Gcode", | ||
| "Path", | ||
| QT_TRANSLATE_NOOP("App::Property", "The gcode to be inserted"), | ||
| ) | ||
|
|
||
| obj.Source = [ "Text", "File" ] |
There was a problem hiding this comment.
I don't believe this is correct, it should be done through opPropertyEnumerations if I understand correctly.
obj.Source is the value, not the potential valid values and should probably default to "Text".
There was a problem hiding this comment.
I cannot remember which Op I was referencing when I wrote this, but it's done in several places. Main/Job.py#525 for example. I'm looking into opPropertyEnumerations now, looks like I can pass this off to the Base class if used right.
There was a problem hiding this comment.
I've submitted PR #9165. If merged it will allow opPropertyEnumerations to be overridden on a subclass to return it's own enumeration values.
There was a problem hiding this comment.
Great I'll update this PR and test to ensure it works as expected.
luzpaz
changed the title
|
Please prepend |
…_custom_source
…_custom_source
This feels a bit heavy but I still feel like it's helpful to inform the path forward to consolidate these default properties.
morganrallen
changed the title
|
@sliptonic this should be good to merge now |
original functionality is the default behavior under Text Source