fix: address review findings in masks substrate

- Remove dead ternary in getPreferredEdges (always returned undefined; the
  all-false object was never reached by Object.values().some(Boolean))
- Document why drawWithFeather descriptors zero the feather field in the
  compositor (analytical renderer handles softness itself; zeroing prevents
  a second JFA feather pass)
- Remove legacyType from V30→V31 migration (not consumed anywhere; a
  down-migration can map type:"freeform" back to "custom" directly)
- Add V30→V31 test coverage: skip-guard branches, freeform passthrough,
  elements without a masks array
- Add snap tests for vertical-edge (bottom) and uniform-scale handle paths

Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
Maze Winther 2026-05-02 01:28:19 +02:00
parent 99137fe493
commit f66cdbfeb2
5 changed files with 116 additions and 12 deletions

View File

@ -324,6 +324,42 @@ describe("mask snapping", () => {
expect(result.activeLines).toEqual([{ type: "vertical", position: 100 }]);
});
test("snaps vertical edge resize for box masks", () => {
// bounds.height=100 → localCanvasSize.height/2=50 is the bottom snap target.
// height=0.2 → baseHeight=20 → aabbHalfH=10.
// At height=0.98 → proposedScaleY=4.9 → bottomEdge=0+10*4.9=49.
// |49-50|=1 < threshold(8) → snaps to height=0.2*5=1.0; line at position 50.
const result = snapBoxMaskInteraction({
handleId: { kind: "edge", side: "bottom" },
startParams: buildRectangleParams(),
proposedParams: buildRectangleParams({ height: 0.98 }),
bounds,
canvasSize,
snapThreshold,
});
expect(result.params.height).toBe(1);
expect(result.activeLines).toEqual([{ type: "horizontal", position: 50 }]);
});
test("snaps uniform scale handle for box masks", () => {
// bounds.width=200 → localCanvasSize.width/2=100 is the right snap target.
// width=0.4, scale=1 → aabbHalfW = (0.4*200)/2 * 1 = 40.
// At scale=2.48 → rightEdge=0+40*2.48=99.2; |99.2-100|=0.8 < threshold(8)
// → snaps to scale=1*(100/40)=2.5; line at position 100.
const result = snapBoxMaskInteraction({
handleId: { kind: "scale" },
startParams: buildRectangleParams({ scale: 1 }),
proposedParams: buildRectangleParams({ scale: 2.48 }),
bounds,
canvasSize,
snapThreshold,
});
expect(result.params.scale).toBe(2.5);
expect(result.activeLines).toEqual([{ type: "vertical", position: 100 }]);
});
test("snaps text mask movement using intrinsic text bounds", () => {
const params = buildTextMaskParams({
centerX: 0.03,

View File

@ -55,16 +55,7 @@ function getPreferredEdges({
};
}
const preferredEdges = {
left: false,
right: false,
top: false,
bottom: false,
} satisfies ScaleEdgePreference;
return Object.values(preferredEdges).some(Boolean)
? preferredEdges
: undefined;
return undefined;
}
function snapMaskPosition({

View File

@ -401,6 +401,10 @@ function buildMaskArtifacts({
body.kind === "drawWithFeather" &&
mask.params.feather === 0 &&
Boolean(body.opaqueFastPath);
// drawWithFeather renderers encode feathering analytically in their canvas output
// (e.g. split mask uses a linear gradient instead of JFA). The descriptor feather is
// zeroed so the GPU compositor copies the mask texture as-is and does not run a second
// JFA feather pass on top of an already-soft texture.
const feather = body.kind === "drawWithFeather" ? 0 : mask.params.feather;
const maskTextureId = `${path}:mask`;

View File

@ -50,8 +50,80 @@ describe("V30 to V31 Migration", () => {
expect(mask).toMatchObject({
id: "mask-1",
type: "freeform",
legacyType: "custom",
params,
});
expect(asRecord(mask).legacyType).toBeUndefined();
});
test("leaves a type:freeform mask in a v30 project unchanged", () => {
const result = transformProjectV30ToV31({
project: {
id: "project-v30-already-freeform",
version: 30,
scenes: [
{
tracks: {
main: {
elements: [
{
id: "elem-1",
masks: [{ id: "m1", type: "freeform", params: {} }],
},
],
},
},
},
],
},
});
expect(result.skipped).toBe(false);
const scene = asRecordArray(result.project.scenes)[0];
const main = asRecord(asRecord(scene.tracks).main);
const element = asRecordArray(main.elements)[0];
expect(asRecordArray(asRecord(element).masks)[0]).toMatchObject({
id: "m1",
type: "freeform",
});
});
test("leaves elements without a masks array unchanged", () => {
const result = transformProjectV30ToV31({
project: {
id: "project-v30-no-masks",
version: 30,
scenes: [
{
tracks: {
main: {
elements: [{ id: "elem-1", type: "video" }],
},
},
},
],
},
});
expect(result.skipped).toBe(false);
const scene = asRecordArray(result.project.scenes)[0];
const main = asRecord(asRecord(scene.tracks).main);
const element = asRecordArray(main.elements)[0];
expect(element).toMatchObject({ id: "elem-1", type: "video" });
});
test("skips a project that is already v31", () => {
const project = { id: "p1", version: 31, scenes: [] };
const result = transformProjectV30ToV31({ project });
expect(result.skipped).toBe(true);
expect(result.reason).toBe("already v31");
expect(result.project).toBe(project);
});
test("skips a project that is not v30", () => {
const project = { id: "p1", version: 29, scenes: [] };
const result = transformProjectV30ToV31({ project });
expect(result.skipped).toBe(true);
expect(result.reason).toBe("not v30");
expect(result.project).toBe(project);
});
});

View File

@ -91,9 +91,10 @@ function migrateMask({ mask }: { mask: unknown }): unknown {
return mask;
}
// "custom" is the only freeform-path mask type; a down-migration can map
// type === "freeform" back to "custom" without any additional marker.
return {
...mask,
type: "freeform",
legacyType: "custom",
};
}