Option to skip griffin header validation. #44
Conversation
Files generated by the command line interface of CuraEngine are output in serial mode, meaning the header has to be printed before the print-time and material estimates are known. This will be needed for command-line tools we used to benchmark the engine with in the nightlies. part ofCURA-9495
done as part of CURA-9495 Co-authored-by: Casper Lamboo <[email protected]>
part of CURA-9495
There was a problem hiding this comment.
Looks fine overall, but some small nitpicks.
More importantly, can we capture your changes in some extra unit tests? (The unit test files seem to already be there, but curiously, I don't see their status being reported here in GitHub. Who owns the CI for this one, is that us?)
| try: | ||
| GCodeFile.__validateGriffinHeader(metadata) | ||
| except InvalidHeaderException as ex: | ||
| if not GCodeFile.SkipHeaderValidation: | ||
| raise ex |
There was a problem hiding this comment.
Why not a simple
if not GCodeFile.SkipHeaderValidation:
GCodeFile.__validateGriffinHeader(metadata)? Isn't that more straightforward than the whole try..except? Or maybe I'm missing something?
There was a problem hiding this comment.
We do this because apparently, the validate function also parses part of the header.
Rewriting it so that it first validates, then parses would be a bit out of the scope of this small change.
This is also why we moved the parts in the validation around.
There was a problem hiding this comment.
We do this because apparently, the validate function also parses part of the header.
With that, I presume you mean that __validateGriffinHeader() also returns or modifies some kind of state? I'm probably "looking with my nose", as we'd say, but the only thing I see is the function checking things and raising exceptions. It has neither a return value, nor does it modify (global) state.
So I don't think your concern is actually valid? (Otherwise I'd agree that you'd want to limit the scope of these changes)
There was a problem hiding this comment.
Yes, that's what I meant when I wrote that :-)
... now that I'm re-checking the function, I can see what you say. Strange, I could've sworn that something was saved in that function. I remember trying it without. -- Perhaps I did something wrong when I originally tried that.
If we're not both wrong, we could indeed try to resolve it the way you originally suggested here.
| @@ -22,6 +22,7 @@ class GCodeFile(FileInterface): | |||
| mime_type = "text/x-gcode" | |||
|
|
|||
| MaximumHeaderLength = 100 | |||
| SkipHeaderValidation = False | |||
There was a problem hiding this comment.
In general, double negatives are something you may want to avoid. Better to introduce a setting ValidateHeader with a default of True.
There was a problem hiding this comment.
I agree with the general sentiment. However, the (extra) negative here is meant to imply that this is not standard behaviour.
I'm perfectly fine with changing it if there are any strong feelings about this though.
There was a problem hiding this comment.
No particularly strong feelings, if it's something you've considered already. I do see your point :)
|
|
||
|
|
||
| # Validate extruder train, part I | ||
| for index in range(0, 15): |
There was a problem hiding this comment.
Why the increment from the original 10 to 15? And could you perhaps make this a named constant while you're at it?
There was a problem hiding this comment.
What the user sees in the front-end is a max of 8 extruders. In the engine (and some of the frontend code as well) we assume max 16 extruders in code. I'm not sure where '10' came from.
Yes, a constant would be good.
There was a problem hiding this comment.
Thanks, that's clear! A named constant with a small comment explaining that bit would indeed be helpful for the future. Thanks 🙏🏻
Files generated by the command line interface of CuraEngine are output in serial mode, meaning the header has to be printed before the print-time and material estimates are known. This will be needed for command-line tools we used to benchmark the engine with in the nightlies.
part of CURA-9495