diff --git a/engineering/understand/diff.py b/engineering/understand/diff.py index 67fca14..da8bef7 100644 --- a/engineering/understand/diff.py +++ b/engineering/understand/diff.py @@ -19,8 +19,12 @@ moment it is matched: 2. **Handle.** A record left over on both sides with the same handle and the same type is the same entity, edited. A save that renumbers handles would make this stage pair strangers, so it runs only while handles look stable: - when at least half of the exact matches kept their handle. The report says - which way it went (``handles_stable``). + when there is at least one exact match and at least half of the exact + matches kept their handle. With no exact match there is no evidence either + way - two fresh files hand out handles from the same seed, and a drawing + moved as a whole matches nothing exactly - so the stage does not run. The + report says which way it went (``handles_stable``) and on what evidence + (``handle_evidence``: the exact matches and how many kept their handle). 3. **Position.** Within one (type, layer, space), a leftover new record is paired with the nearest leftover old record whose anchor (the mean of its points, else the centre of its box) lies within ``move_tol``; the closest @@ -29,8 +33,15 @@ moment it is matched: anchor of both revisions - so one stray entity kilometres away cannot inflate it. -What is left is added (new side) or removed (old side). A matched pair that -is not identical is *changed*, and the row says what changed: ``moved_by`` +What is left is added (new side) or removed (old side). A matched pair is +compared field by field, each number within half a ``tol`` of its old value +counting as equal - the same test a move uses. A pair with nothing to report +is *unchanged*: the signature snaps every value to the ``tol`` grid, so a +value one ulp either side of a half-grid point (AutoCAD writes 0.145 for the +0.14500000000000002 it was given) lands in another bucket, misses the exact +stage and is paired later, yet nothing was edited. Such a pair counts under +``unchanged`` and gets no row and no cloud. Any other matched pair is +*changed*, and the row says what changed: ``moved_by`` (dx, dy) when the geometry is the old geometry translated, else ``geometry`` from -> to; ``text``, ``attribs`` (by tag), ``radius``, ``angles``, ``height``, ``rotation``, ``scale``, ``block``, ``layer``, ``space``, @@ -48,8 +59,8 @@ reference, so a redrawn definition changes every drawn copy while every INSERT record stays identical. The 2 % move tolerance, the 1st/99th percentiles, the half-of-the-matches -handle rule and the cluster padding are this module's declared defaults, not -values from a standard. +(and at least one) handle rule, the half-a-``tol`` equality and the cluster +padding are this module's declared defaults, not values from a standard. """ from __future__ import annotations @@ -248,11 +259,21 @@ def _nearest_pairs( def _number_changed(a, b, tol: float, *, modulo: float | None = None) -> bool: + """More than half a ``tol`` apart (around the circle when ``modulo``). + + A difference, never two grid buckets: ``_q`` puts 0.14500000000000002 + and 0.145 in different buckets at ``tol=0.01``. + """ if a is None or b is None: return (a is None) != (b is None) + a, b = float(a), float(b) + if not (math.isfinite(a) and math.isfinite(b)): + return repr(a) != repr(b) + difference = abs(a - b) if modulo is not None: - a, b = a % modulo, b % modulo - return _q(a, tol) != _q(b, tol) + difference %= modulo + difference = min(difference, modulo - difference) + return difference > tol / 2.0 def _movement(o: EntityRecord, n: EntityRecord, tol: float) -> dict: @@ -302,7 +323,9 @@ def _changes(o: EntityRecord, n: EntityRecord, tol: float) -> dict: if o.space != n.space: out["space"] = {"from": o.space, "to": n.space} out.update(_movement(o, n, tol)) - if tuple(_q(b, tol) for b in o.bulges) != tuple(_q(b, tol) for b in n.bulges): + if len(o.bulges) != len(n.bulges) or any( + _number_changed(a, b, tol) for a, b in zip(o.bulges, n.bulges, strict=False) + ): out["bulges"] = {"from": list(o.bulges), "to": list(n.bulges)} if bool(o.closed) != bool(n.closed): out["closed"] = {"from": bool(o.closed), "to": bool(n.closed)} @@ -441,7 +464,9 @@ def diff_snapshots( exact += 1 kept_handle += int(olds[i].handle == rec.handle) left_old = [i for i in range(len(olds)) if i not in used_old] - handles_stable = exact == 0 or 2 * kept_handle >= exact + # No exact match is no evidence: strangers from two fresh files share + # handles, and so do the entities of a drawing that moved as a whole. + handles_stable = exact > 0 and 2 * kept_handle >= exact pairs: list[tuple[int, int, str]] = [] # 2. handle @@ -468,10 +493,16 @@ def diff_snapshots( changed: list[dict] = [] items: list[tuple[str, Box, int]] = [] - for key, (i, j, how) in enumerate( - sorted(pairs, key=lambda p: (olds[p[0]].handle, news[p[1]].handle)) - ): + same = 0 + for i, j, how in sorted(pairs, key=lambda p: (olds[p[0]].handle, news[p[1]].handle)): o, n = olds[i], news[j] + changes = _changes(o, n, tol) + if not changes: + # the signature grid split an unedited pair (a value 1 ulp across a + # half-grid point): unchanged, no row and no cloud + same += 1 + continue + key = len(changed) at = anchor(n) changed.append( { @@ -482,7 +513,7 @@ def diff_snapshots( "space": n.space, "matched_by": how, "at": None if at is None else [at[0], at[1]], - "changes": _changes(o, n, tol), + "changes": changes, } ) for rec in (o, n): @@ -504,10 +535,11 @@ def diff_snapshots( "tol": tol, "move_tol": move_tol, "handles_stable": handles_stable, + "handle_evidence": {"exact_matches": exact, "kept_handle": kept_handle}, "summary": { "old_entities": len(olds), "new_entities": len(news), - "unchanged": exact, + "unchanged": exact + same, "changed": len(changed), "added": len(added), "removed": len(removed), @@ -623,11 +655,16 @@ def read_revision(path: str, *, tol: float = DEFAULT_TOL) -> tuple[Snapshot, dic """One read of a DXF revision: its records and its block signatures. Refused before reading: a file that cannot be stat'ed and one over - ``MAX_DXF_BYTES`` (the size and the variable named); a file ezdxf cannot - parse is refused with ezdxf's reason. + ``MAX_DXF_BYTES`` (the size and the variable named). A file ezdxf cannot + parse or walk is refused as ``not a readable DXF`` with the reason - + whatever ezdxf raised: a truncated file (an interrupted save, a partial + copy) raises ``StopIteration`` from inside its tag reader, which + ``asyncio.to_thread`` cannot hand back to an awaiting caller, so letting it + through would hang ``drawing_diff`` instead of refusing. """ import ezdxf + tol = _positive("tol", tol) limit = int(config.settings.max_dxf_bytes) try: size = os.path.getsize(path) @@ -640,6 +677,12 @@ def read_revision(path: str, *, tol: float = DEFAULT_TOL) -> tuple[Snapshot, dic ) try: doc = ezdxf.readfile(path) - except (OSError, ezdxf.DXFError) as exc: - raise ValueError(f"{path}: not a readable DXF ({exc})") from exc - return records_from_doc(doc, source=str(path)), block_signatures(doc, tol=tol) + return records_from_doc(doc, source=str(path)), block_signatures(doc, tol=tol) + except MemoryError: + raise + except Exception as exc: # noqa: BLE001 - every parse failure is one refusal + if isinstance(exc, StopIteration): + reason = "the file ends early - an interrupted save or a partial copy?" + else: + reason = str(exc) or type(exc).__name__ + raise ValueError(f"{path}: not a readable DXF ({reason})") from exc diff --git a/tests/test_understand_diff.py b/tests/test_understand_diff.py index 26d185c..cfb0d1d 100644 --- a/tests/test_understand_diff.py +++ b/tests/test_understand_diff.py @@ -146,6 +146,29 @@ def test_renumbered_handles_never_pair_strangers_that_share_a_handle(): assert [r["at"] for r in result["added"]] == [[5000.0, 5000.0]] +def test_no_exact_match_is_no_evidence_that_handles_are_stable(): + # two fresh files hand out handles from one seed; every circle moved +1000 in x + # and the new file added them in reverse order, so equal handles are strangers + old = snap( + *(rec(f"{0x20 + i:X}", "CIRCLE", "EQ", ((0, 200 * i),), radius=10.0) for i in range(8)) + ) + new = snap( + *( + rec(f"{0x20 + k:X}", "CIRCLE", "EQ", ((1000, 200 * i),), radius=10.0) + for k, i in enumerate(reversed(range(8))) + ) + ) + result = diff_snapshots(old, new) + assert result["handles_stable"] is False + assert result["handle_evidence"] == {"exact_matches": 0, "kept_handle": 0} + assert result["changed"] == [] + assert (result["summary"]["added"], result["summary"]["removed"]) == (8, 8) + # given a move tolerance that reaches, the position stage finds the true move + moved = diff_snapshots(old, new, move_tol=1001.0) + assert {row["matched_by"] for row in moved["changed"]} == {"position"} + assert {tuple(row["changes"]["moved_by"]) for row in moved["changed"]} == {(1000.0, 0.0)} + + def test_beyond_move_tol_a_line_is_removed_and_another_added(): old = snap(*STILL, line("D", 0, 200, 100, 200)) new = snap(*STILL, line("E", 0, 700, 100, 700)) @@ -154,6 +177,65 @@ def test_beyond_move_tol_a_line_is_removed_and_another_added(): assert result["changed"] == [] +# -- a value split by the signature grid ---------------------------------------------- + + +def test_a_value_one_ulp_across_a_half_grid_point_is_unchanged(): + # AutoCAD writes 16 significant digits: 0.14 + 0.005 = 0.14500000000000002 comes + # back as 0.145, and at tol=0.01 the two snap to buckets 15 and 14 + assert 0.14 + 0.005 != 0.145 + edge = 0.14 + 0.005 + assert signature(line("L", edge, 300, 50, 300)) != signature(line("L", 0.145, 300, 50, 300)) + old = snap(*STILL, line("L", edge, 300, 50, 300)) + for new in ( + snap(*STILL, line("L", 0.145, 300, 50, 300)), # paired by handle + snap(*RENUMBERED, line("1L", 0.145, 300, 50, 300)), # paired by position + ): + result = diff_snapshots(old, new, move_tol=10.0) + assert result["summary"]["unchanged"] == 4 + assert (result["summary"]["changed"], result["summary"]["added"]) == (0, 0) + assert result["summary"]["removed"] == 0 + assert result["changed"] == [] and result["clusters"] == [] + assert result["by_layer"] == {} and result["by_type"] == {} + + +def test_numbers_one_ulp_across_a_grid_boundary_are_not_changes(): + # at tol=1, 12.500000000000002 and 12.5 snap to 13 and 12 + edge = 12.500000000000002 + old = snap( + *STILL, + rec("R", "CIRCLE", "PIPE", ((edge, 0),), radius=edge), + rec("T", "TEXT", "NOTES", ((10, 10),), text="X", height=edge, rotation=edge), + rec("P", "LWPOLYLINE", "PIPE", ((0, 0), (10, 0)), bulges=(edge, 0.0)), + ) + new = snap( + *STILL, + rec("R", "CIRCLE", "PIPE", ((12.5, 0),), radius=12.5), + rec("T", "TEXT", "NOTES", ((10, 10),), text="X", height=12.5, rotation=12.5), + rec("P", "LWPOLYLINE", "PIPE", ((0, 0), (10, 0)), bulges=(12.5, 0.0)), + ) + result = diff_snapshots(old, new, tol=1.0) + assert result["summary"]["unchanged"] == 6 + assert result["changed"] == [] and result["clusters"] == [] + + +def test_a_real_edit_next_to_a_grid_split_is_still_reported(): + old = snap( + *STILL, + line("L", 0.14 + 0.005, 300, 50, 300), + rec("R", "CIRCLE", "PIPE", ((0, 0),), radius=10.0), + ) + new = snap( + *STILL, line("L", 0.145, 300, 50, 300), rec("R", "CIRCLE", "PIPE", ((0, 0),), radius=10.2) + ) + result = diff_snapshots(old, new) + (row,) = result["changed"] + assert row["old_handle"] == "R" + assert row["changes"] == {"radius": {"from": 10.0, "to": 10.2}} + assert result["summary"]["unchanged"] == 4 + assert [c["count"] for c in result["clusters"]] == [1] + + # -- the move tolerance ---------------------------------------------------------------- @@ -260,6 +342,22 @@ def test_read_revision_refuses_a_file_that_is_not_a_dxf(tmp_path): read_revision(str(path)) +def test_read_revision_refuses_a_truncated_dxf(tmp_path): + # an interrupted save: ezdxf's tag reader runs out and raises StopIteration, + # which must surface as the refusal, not escape (it would hang asyncio.to_thread) + path = tmp_path / "half.dxf" + path.write_bytes(b" 0\nSECTION\n 2\nHEADER\n 9\n$ACADVER\n 1\nAC1015\n") + with pytest.raises(ValueError, match=r"half\.dxf: not a readable DXF \(the file ends early"): + read_revision(str(path)) + + +def test_read_revision_refuses_a_non_positive_tol_by_name(tmp_path): + path = tmp_path / "empty.dxf" + ezdxf.new().saveas(path) + with pytest.raises(ValueError, match=r"^tol must be a positive number"): + read_revision(str(path), tol=0) + + # -- the known edits on the synthetic plant -------------------------------------------- diff --git a/tests/test_understand_diff_tool.py b/tests/test_understand_diff_tool.py index 2f718c9..e7c5eda 100644 --- a/tests/test_understand_diff_tool.py +++ b/tests/test_understand_diff_tool.py @@ -125,6 +125,20 @@ async def test_a_revision_that_is_neither_dxf_nor_dwg_is_refused_by_parameter(cl assert "old_path:" in text and "reads a .dxf or a .dwg" in text +async def test_a_truncated_revision_is_refused_not_hung(client, tmp_path): + import asyncio + + half = tmp_path / "half.dxf" + half.write_bytes(b" 0\nSECTION\n 2\nHEADER\n 9\n$ACADVER\n 1\nAC1015\n") + result = await asyncio.wait_for( + client.call_tool("drawing_diff", {"old_path": str(half)}, raise_on_error=False), + timeout=30, + ) + assert result.is_error is True + text = result.content[0].text + assert "old_path:" in text and "not a readable DXF" in text + + async def test_the_diff_is_advertised_as_non_destructive(client): tools = {tool.name: tool for tool in await client.list_tools()} assert tools["drawing_diff"].annotations.destructiveHint is False