Let an item carry its own mirror axes (#664) - #673
Merged
Merged
Conversation
An item's mirror participation was a bool and the axes lived on the layer, so turning symmetry off or moving it from X to Y changed every item made under the old axes: a lump made on +x under X lost its -x twin. Node::own_mirror_axes holds the item's own x|y|z, 0 for none, or inherit (the default, and what every older document loads as). Own axes replace the layer's for that item outright; the participation flag keeps deciding what an inheriting item takes and the radial mode. effective_mirror_axes is the one definition the compiler, bounds, cull pad (seam term and a raise-only own-axes union in the symmetry multiplicity), picking, consolidation and the Move brush read. The Move/magnify drag reaches an item with its own axes through its own reflections, built per (axes, radial) set; inheriting items keep reading the layer's set unchanged. The reported reach adds the dragged items' own axes. SetNodeMirrorCmd sets a placed item's participation and axes as one undo step. C ABI 0.120.1 -> 0.121.0 adds CLAY_MIRROR_AXES_INHERIT, clay_item_set_mirror_axes / clay_item_mirror_axes and clay_layer_set_node_mirror / clay_layer_node_mirror; pyclay gains add(mirror_axes=), set_node_mirror and node_mirror. Scene and container minor 19 -> 20 append one byte per node. Writing below 20 is refused for a document holding own axes, as minor 18 refuses a composition, and a document whose items all inherit still writes 19's bytes. The gallery documents are regenerated at minor 20.
The existing cases checked drag_images directly, but nothing covered the two callers that fold the dragged items' own axes into the reach: dropping prepared_own_mirror_axes from SdfMoveTransaction::begin left every case green. Assert the twin's box through clay_layer_move_surface_regions and a live drag's dirty bounds; each fails when its caller passes 0. Also fix two comments naming functions that do not exist (SetItemMirrorCmd, images_for).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #664
Why
An item's mirror participation was a bool, and the axes lived on the layer. A host that mirrors what is made while symmetry is on, and wants existing items left as they were made, could only get half of that. Turning symmetry off, or switching X to Y, changed every item made under the old axes. This PR takes ask 1 from the issue: per-item axes, plus a setter and a reader for a placed node.
I measured this through the C ABI. The fixture is a lump of radius 0.25 at (0.6, 0.4, 0), made under a layer mirror X. The table reads the field at the centre of the X twin, (-0.6, 0.4, 0), and at the Y reflection, (0.6, -0.4, 0). A value of -0.25 means a copy is there.
What lands
scene::Node::own_mirror_axes:kMirrorX|Y|Z,0for none, orkMirrorAxesInherit(0xFF, the default).scene::effective_mirror_axes(item, layer)returns the item's own axes if it has them. Otherwise it returns the layer's axes when the item participates, and none when it doesn't.emit_item, the geometry and influence bounds, the cull pad (the seam term, plus a raise-onlyCullPadTerms::own_mirror_axesunion inlayer_symmetry_multiplicity), picking's selection bound, consolidation's patch test, and the layer digest.mirror_k) and the mirror planes stay the layer's.SetNodeMirrorCmd {layer, node, mirror, own_mirror_axes}sets both values in one undo step. It has a new journal tag, and it is listed incommand_edited_item.CLAY_MIRROR_AXES_INHERIT, and four new functions:clay_item_set_mirror_axesandclay_item_mirror_axesfor the item builder.clay_layer_set_node_mirror(undoable) andclay_layer_node_mirrorfor a placed node. The reader also reports the axes the item is actually reflected through..clayspaceminor goes from 19 to 20: one byte is appended to each node record, gated on both the write and read sides.layer_blocking_minornames the layer that blocks. The C ABI gives a distinct refusal message for this case.brush/move.h's existing rule: the drag images are the copies the compiler emits of this item.clay_layer_move_surface,clay_layer_magnify_surface, the live move transaction) adds the dragged items' own axes viaprepared_own_mirror_axes.Layer.add(..., mirror_axes=),Layer.set_node_mirror,Layer.node_mirror.add-per-item-mirror-axes, with ADDED requirements in scene-model, c-abi, brush-engine, file-io and python-bindings. Also updated:docs/05,docs/07,brush/move.hand theclay.hnotes.ABI 0.120.1 -> 0.121.0. Version moved in all three files. Format minor 19 -> 20. The nine gallery
.clayspacedocuments are regenerated at minor 20; renders,.plyand.objoutputs are discarded, as the verify skill says.What building it found
AddNodeCmd, so it takes the general invalidation.test_layer_composition's byte-count test compared minor 17 with the current minor. It now compares against 18.Verification
Regression tests.
tests/unit/test_item_mirror_axes.cpp: 10 cases, 209 assertions. It covers:clay_layer_set_node_mirrorPlus
test_pyclay.py::test_an_item_keeps_its_own_mirror_axes_when_the_layer_mirror_changes.Proof that the tests fail without the fix. The tests call the new API, so they cannot compile against main as-is. Instead I put main's behaviour back one mechanism at a time and ran the file:
effective_mirror_axesignores own axesGates, all run on macOS arm64:
ctest --preset cpu-only(tests and pyclay on): 11/11 passed. That covers all four unit shards andpyclay_pytest.tools/release_check.py --skip-slow: every row passes exceptdevice. The passing rows are version, configure, build, tests (11/11), cpu parity, layering, dialect, licenses, task-symbols, bindings (it imported the built pyclay, so the parity comparison is real), kernels, abi, openspec, and the waived hardware rows.deviceFAILS: "engine changed since the gate ran at 7f6cf38". That record predates Carry a seed across a region edit only if no append since reached it #668 and Resolve coincident drag images as one brush (#663) #669, and main already differs from it in 9 engine files, so main fails this row too. It is a release-time gate on the reference iPad, and I did not run it.check_layering,check_kernel_dialect,check_licenses,check_test_shards --binary,check_doc_latency,check_c_abiagainstlibclay_shared.dylib,check_binding_parity --require-import, andnpx @fission-ai/openspec@1.12.0 validate --all --strict(67 passed).check_gallery: I regenerated the gallery first withCLAY_EXAMPLES_FAST=1 examples/run_all.py(76/76 examples succeeded) and committed the nine.clayspacefiles at minor 20. The gate then passes.g++-16 -fsyntax-only -Wall -Wextra -Wpedantic -Wshadow -Werroris clean on every changed engine file,clay_c.cppand the new test. This stands in for the Ubuntu job, which AppleClang cannot reproduce.Concerns
read_nodeanddeserializeinsrc/scene/commands.cppare over the parser target on main already: 40 and 37. They are 41 and 37 here, since the node byte adds one gated read and the new case adds nothing. Every function added or touched in this PR scores 12 or less.18_sampled.clayspaceshrank by 9 KB (314,720 -> 305,397 bytes) when regenerated on this machine. The example uses no mirror or drag code, and the node byte adds bytes rather than removing them. So this looks like the platform float variationcheck_gallery.pydescribes for band-sampled output. I did not confirm that by regenerating on main.BM_MoveDrag*) or the device gate. The inheriting path builds and reads the same image set it did before, and costs one extra branch per item.