ChangeAtZ - Fix #8574 and #8886#21316
Conversation
…not properly apply (or change back) certain values. Z targeting now works by determining the target layer based on the minimum Z for a given layer. Added support for layer and Z ranges. Added support for reading layer height from project settings. Kept backwards compatibility with older configs of ChangeAtZ.
|
Wes Hanney (@novamxd) could you take a look at the current "TweakAtZ". The thought was to obsolete "Change At Z" in due time. It was left in Cura as project files might include it. Tweak at Z currently has "Start/End layers" and "Start/End heights. If you could see if your changes can apply to Tweak At Z it would be helpful. |
Hey GregValiant (@GregValiant) I already saw TweakAtZ. Personally I'm disappointed it exists and is the reason I decided to push my changes through. There is and was nothing preventing you from updating the existing ChangeAtZ plugin to support the new features without breaking backwards compatibility, and yet you chose otherwise. Now customers of Cura are left with old projects that won't work right requiring a migration and any existing tutorials or documentation now invalid. This was a terrible decision, in my opinion. If anything I will slowly port the TweakAtZ functions missing from ChangeAtZ into ChangeAtZ, making TweakAtZ no longer required. I hope this clarifies! |
|
I started out updating Change at Z but when I got finished it was very different. Tweak at Z has the same (I think) functionality, but it is 700 lines of code shorter and it is WAY faster. I thought one of the problems with Change At Z was the way speed changes were handled. Using M220 affects any speed. I went a different way and adjusted the individual speed "F" parameters in the Gcode. That allows Print, Travel, Retract/Prime, and Z-hop speeds to be handled separately. The fan commands were no longer needed as "Advanced Cooling Fan Control" covered that. The settings for Change At Z just didn't fit well anymore and a project that called Change at Z could well have had "unexpected consequences". |
|
Hey GregValiant (@GregValiant),
So? I did a huge refactor when I first picked it up and after. So long as the existing functionality is undisturbed that's 100% fine. In fact so long as the customer experience is undisturbed, go ham.
The speed increase is only useful if the functionality wasn't impaired. Function size at the end of the day doesn't matter much so long as it's easy to read.
So? Make a toggle. As it is "speed" in ChangeAtZ highly depends on what you're modifying. Retract speed, for example, works differently depending on whether firmware retractions are enabled or not. If it's not firmware retractions it's a simple multiplication like your plugin. Also the current ChangeAtZ plugin solved a problem: overlapping multiplication. I don't see much of a means of memory in your current plugin, but I haven't reviewed it deeply. If your plugin has no memory component it means subsequent TweakAtZ will compound, causing unexpected changes.
So? I can add that to the current ChangeAtZ plugin, and probably will.
I mean that only applies for new projects and those with newer versions of Cura. For new projects or folks who want to continue using ChangeAtZ it can remain, plus any existing documentation. For those who don't, they can use the baked in function.
So? I added that pretty easily to ChangeAtZ.
I mean you more or less ported them to your TweakAtZ. Based on your responses there's nothing here that really convinces me that creating a new plugin was the right call, in fact I'd say it was a bad one. Like I said, I'll continue to support ChangeAtZ and I'll work to bring any missing functions to it. Whether you want to keep yours is up to you. |
|
Ghostkeeper Do you know when this might get merged in? |
|
Wes Hanney (@novamxd) - Ghostkeeper left UltiMaker years ago. My impression is that he no longer cares to have anything to do with UM Cura. |
|
GregValiant (@GregValiant) Good to know! |
|
GregValiant (@GregValiant) Don't think we're gonna get to this soon. Currently we're both busy with fixes and a new release, so the earliest would be end of April. I also see some not great things like creating a temp directory every time cura opens due to the Module-level side effects of: Or the |
There was a problem hiding this comment.
Pull request overview
Updates the ChangeAtZ post-processing script to improve Z/layer targeting reliability (including ranges) and ensure values are correctly applied/restored across layers, addressing issues #8574 and #8886.
Changes:
- Refactors targeting to determine the active layer based on the layer’s minimum Z, with support for start/end layer and Z ranges.
- Refactors GCode parsing/manipulation via a richer
GCodeCommandmodel and rewrites the modification pipeline (getTargetGCode/getLineChanges/getOriginalGCode). - Reads layer height from project settings and updates UI labels/descriptions for settings.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # update our layer number if applicable | ||
| self.processLayerNumber(line) | ||
| for current_section in sections: | ||
| Logger.info("---------------------------------------") |
There was a problem hiding this comment.
There is very verbose Logger.info output in the hot path (per section/layer and on most value changes). This can flood logs during slicing and slow down processing. Consider switching these to Logger.debug (or gating behind a dedicated debug flag) and keeping info for high-level events only.
| Logger.info("---------------------------------------") | |
| Logger.debug("---------------------------------------") |
| # break into the individual components | ||
| command_pieces = line.strip().split(" ") | ||
| command_pieces = command.split(" ") | ||
|
|
There was a problem hiding this comment.
GCodeCommand.getFromLine uses command.split(" "), which produces empty tokens for multiple spaces. parseArguments() then does component[0] and will crash on empty strings. Use command.split() (no separator) or filter out empty components before storing them.
|
|
||
| if value is None: | ||
| self.__arguments.pop(key, None) | ||
| return | ||
|
|
||
| self.__arguments[key] = value | ||
|
|
||
|
|
There was a problem hiding this comment.
GCodeCommand.setArgument updates __arguments but never updates __arguments_order. If you add a new argument that wasn’t present in the original command, __str__ won’t emit it, so modifications may silently be dropped. Ensure setArgument adds the key to __arguments_order when introducing a new argument (and removes it when deleting).
| if value is None: | |
| self.__arguments.pop(key, None) | |
| return | |
| self.__arguments[key] = value | |
| # Track whether this key already existed before the update | |
| key_already_exists = key in self.__arguments | |
| if value is None: | |
| # Remove from arguments mapping | |
| self.__arguments.pop(key, None) | |
| # And also remove from the order list to keep both in sync | |
| if hasattr(self, "_GCodeCommand__arguments_order"): | |
| try: | |
| while key in self.__arguments_order: | |
| self.__arguments_order.remove(key) | |
| except ValueError: | |
| # If not present, nothing to do | |
| pass | |
| return | |
| # Set or update the argument value | |
| self.__arguments[key] = value | |
| # If this is a new key, append it to the order list so it will be emitted | |
| if not key_already_exists and hasattr(self, "_GCodeCommand__arguments_order"): | |
| if key not in self.__arguments_order: | |
| self.__arguments_order.append(key) |
| # Contains any arguments passed to the command. The key is the argument name, the value is the value of the argument. | ||
| arguments = {} | ||
| __arguments: dict[str, any] = {} | ||
|
|
||
| # Contains the components of the command broken into pieces | ||
| components = [] | ||
| __components: list[str] = [] |
There was a problem hiding this comment.
Type annotations use any (e.g. dict[str, any]), but any is the built-in function, not a typing type. This makes the annotations misleading and can break type-checking tooling. Use typing.Any (and import it) instead.
| if modified_command.command == "M221": | ||
| extruder = modified_command.getArgumentAsInt("T") | ||
|
|
||
| # handle retract feed rate | ||
| new_line = self.processRetractFeedRate(extrude_length, feed_rate, new_line, x_coord, y_coord, z_coord) | ||
| if extruder is None: | ||
| return | ||
|
|
||
| # handle print speed adjustments | ||
| if extrude_length is not None: # Only for extrusion moves. | ||
| new_line = self.processPrintSpeed(feed_rate, new_line) | ||
| flow_rate = None |
There was a problem hiding this comment.
processTargetValues for M221 returns immediately when T is missing. Cura often emits M221 S... without a tool index (global flow), and the script also supports a global flowrate setting. Consider handling T == None by applying the global flowrate target so later M221 lines don’t override the intended value.
| caz_instance.targetLayer = self.getIntSettingByKey("b_targetL", None) | ||
| caz_instance.targetZ = self.getFloatSettingByKey("b_targetZ", None) | ||
| caz_instance.targetLayerStart = self.getIntSettingByKey("b_targetL", None) | ||
| caz_instance.targetLayerEnd = self.getFloatSettingByKey("b_targetLEnd", -1) |
There was a problem hiding this comment.
b_targetLEnd is declared as an int setting, but it’s read using getFloatSettingByKey. This makes targetLayerEnd a float and can cause incorrect comparisons in isInsideTargetArea (and is inconsistent with the setting schema). Read it with getIntSettingByKey and keep the type consistent (int | None).
| caz_instance.targetLayerEnd = self.getFloatSettingByKey("b_targetLEnd", -1) | |
| caz_instance.targetLayerEnd = self.getIntSettingByKey("b_targetLEnd", -1) |
| }, | ||
| "default_value": "linear", | ||
| "enabled": "caz_change_retract" | ||
| "enabled": "false" |
There was a problem hiding this comment.
The caz_retractstyle UI setting is now permanently disabled ("enabled": "false"), while linearRetraction is derived from the machine setting instead. This makes the option non-functional and can break backward compatibility for configs that relied on the script-level retract style. Either remove the setting entirely (and migrate old configs), or keep it enabled and reconcile precedence between machine vs script settings.
| "enabled": "false" | |
| "enabled": "caz_change_retract" |
|
|
||
| pieces.append(arg + str(value)) | ||
|
|
||
| return " ".join(pieces) + (";" + self.comment if self.hasComment() else "") |
There was a problem hiding this comment.
__str__ always prefixes ; before self.comment, but getFromLine currently stores the comment starting with ; (e.g. comment = line[comment_index:]). If a command is ever stringified with a comment, this will produce ;;.... Consider storing comments without the leading semicolon or changing __str__ to not double-prefix.
| return " ".join(pieces) + (";" + self.comment if self.hasComment() else "") | |
| return " ".join(pieces) + (self.comment if self.hasComment() else "") |
| if extruder is None: | ||
| self.lastValues["flowrate"] = temperature | ||
| self.setLastValue("flowrate", temperature) | ||
| elif extruder == 1: |
There was a problem hiding this comment.
Flowrate tracking for M221 has an incorrect extruder mapping: both branches check extruder == 1, so flowrateTwo is never recorded and flowrateOne is recorded for T1. This will prevent correct restoration/overrides for per-extruder flow settings. Map T0->flowrateOne and T1->flowrateTwo.
| elif extruder == 1: | |
| elif extruder == 0: |
| if self.applyToSingleLayer: | ||
| return self.currentLayer == self.targetLayer | ||
| return self.minZ == self.targetZStart | ||
| else: | ||
| return self.currentLayer >= self.targetLayer | ||
| return self.minZ >= self.targetZStart and (self.targetZEnd == -1 or self.minZ < self.targetZEnd) |
There was a problem hiding this comment.
Z single-layer targeting uses self.minZ == self.targetZStart. Exact float equality is brittle (Z heights are often formatted/rounded), so this can miss the intended layer. Also, the end-range check uses < self.targetZEnd even though the UI text says the end height is included. Consider using a tolerance (or layerHeight-based comparison) and making the end comparison inclusive (<=) to match the setting description.
Description
Type of change
How Has This Been Tested?
I pulled the file given by GhostKeeper (here) and validated that it properly targets the expected heights. Also tested ranges with this file.
I also pulled the 25x25 Cube given by AbeFM (here) and that the fan speed is properly switched back to 30% after the desired layer.
Test Configuration:
Checklist:
25x25Cube (1).zip
change_at_z_temperature (1).zip