From 3c7683a2ae81b7c00a59affdfbbe69dd694fa1d3 Mon Sep 17 00:00:00 2001 From: Max Franz Date: Tue, 4 Feb 2025 12:14:11 -0500 Subject: [PATCH] Add some bezier edge bug fixes - `cachedBoundingBoxImpl` just uses `opts.useCache` directly - Edge point functions, like `edge.controlPoints()` always return `undefined` if the edge doesn't take up space (e.g. `display:none`) - `ele.restore()` precedes bypass application when adding a new element with a bypass - Recalculating the rendered style in the renderer: The rendered style is considered dirty (implicitly) for bundled bezier edges if another edge in the bundle is dirty - The style triggers are cleaned up. Two new behaviours can be fully specified in the style properties, rather than using flags as before. They are `triggersBoundsOfConnectedEdges` and `triggersBoundsOfParallelEdges`. `triggersBoundsOfParallelEdges` needs a reference to the element for `display`, which is probably why it was a flag before. In any case, separating it out makes it more readable and fixes the new renderer test cases. --- playwright-tests/renderer.spec.js | 30 ++++++++++---- src/collection/dimensions/bounds.mjs | 2 +- src/collection/dimensions/edge-points.mjs | 6 +-- src/collection/element.mjs | 8 ++-- .../base/coord-ele-math/rendered-style.mjs | 8 ++++ src/style/apply.mjs | 39 +++++++++---------- src/style/properties.mjs | 27 ++++++++++++- 7 files changed, 83 insertions(+), 37 deletions(-) diff --git a/playwright-tests/renderer.spec.js b/playwright-tests/renderer.spec.js index e82407cde4..cc295b932f 100644 --- a/playwright-tests/renderer.spec.js +++ b/playwright-tests/renderer.spec.js @@ -293,7 +293,7 @@ test.describe('Renderer', () => { test('do not move when setting one edge to visibility:hidden', async ({ page }) => { await page.evaluate(() => { - cy.$('#ab1').style('visibility', 'hidden'); + window.cy.$('#ab1').style('visibility', 'hidden'); }); let pt_ab1_2 = await page.evaluate(() => cy.$('#ab1').controlPoints()[0]); @@ -305,7 +305,7 @@ test.describe('Renderer', () => { test('move when setting one edge to display:none', async ({ page }) => { await page.evaluate(() => { - cy.$('#ab1').style('display', 'none'); + window.cy.$('#ab1').style('display', 'none'); }); let pt_ab1_2 = await page.evaluate(() => cy.$('#ab1').controlPoints()); @@ -317,7 +317,7 @@ test.describe('Renderer', () => { test('move when setting one edge to display:none (bigger bundle)', async ({ page }) => { await page.evaluate(() => { - cy.add([ + window.cy.add([ { data: { id: 'ab3', source: 'a', target: 'b' } }, @@ -342,7 +342,7 @@ test.describe('Renderer', () => { test('move when setting one edge to curve-style:straight', async ({ page }) => { await page.evaluate(() => { - cy.add([ + window.cy.add([ { data: { id: 'ab3', source: 'a', target: 'b' } }, @@ -357,13 +357,29 @@ test.describe('Renderer', () => { cy.$('#ab1').style('curve-style', 'straight'); }); - let pt_ab1_2 = await page.evaluate(() => cy.$('#ab1').controlPoints()); - let pt_ab2_2 = await page.evaluate(() => cy.$('#ab2').controlPoints()[0]); + let pt_ab1_2 = await page.evaluate(() => window.cy.$('#ab1').controlPoints()); + let pt_ab2_2 = await page.evaluate(() => window.cy.$('#ab2').controlPoints()[0]); expect(pt_ab1_2).toBeUndefined(); // because curve-style:straight expect(pt_ab2_2).toBeDefined(); - }); // move when setting one edge to display:none + }); // move when setting one edge to curve-style:straight + + // test('do not move when straight edge added', async ({ page }) => { + // await page.evaluate(() => { + // window.cy.add([ + // { + // data: { id: 'ab3', source: 'a', target: 'b' }, + // style: { + // 'curve-style': 'straight' + // } + // } + // ]); + // }); + + // // TODO... + + // }); // move when setting one edge to curve-style:straight }); // bundled beziers diff --git a/src/collection/dimensions/bounds.mjs b/src/collection/dimensions/bounds.mjs index 9e69ac1e73..93d013d588 100644 --- a/src/collection/dimensions/bounds.mjs +++ b/src/collection/dimensions/bounds.mjs @@ -820,7 +820,7 @@ let cachedBoundingBoxImpl = function( ele, opts ){ let usingDefOpts = key === defBbOptsKey; let currPosKey = getBoundingBoxPosKey( ele ); let isPosKeySame = _p.bbCachePosKey === currPosKey; - let useCache = opts.useCache && isPosKeySame; + let useCache = opts.useCache; let isDirty = ele => ele._private.bbCache == null || ele._private.styleDirty; let needRecalc = !useCache || isDirty(ele) || (isEdge && (isDirty(ele.source()) || isDirty(ele.target()))); diff --git a/src/collection/dimensions/edge-points.mjs b/src/collection/dimensions/edge-points.mjs index f942965778..aa215f5fa7 100644 --- a/src/collection/dimensions/edge-points.mjs +++ b/src/collection/dimensions/edge-points.mjs @@ -1,13 +1,13 @@ import * as math from '../../math.mjs'; const ifEdge = (ele, getValue) => { - if( ele.isEdge() ){ + if( ele.isEdge() && ele.takesUpSpace() ){ return getValue( ele ); } }; const ifEdgeRenderedPosition = (ele, getPoint) => { - if( ele.isEdge() ){ + if( ele.isEdge() && ele.takesUpSpace() ){ let cy = ele.cy(); return math.modelToRenderedPosition( getPoint( ele ), cy.zoom(), cy.pan() ); @@ -15,7 +15,7 @@ const ifEdgeRenderedPosition = (ele, getPoint) => { }; const ifEdgeRenderedPositions = (ele, getPoints) => { - if( ele.isEdge() ){ + if( ele.isEdge() && ele.takesUpSpace() ){ let cy = ele.cy(); let pan = cy.pan(); let zoom = cy.zoom(); diff --git a/src/collection/element.mjs b/src/collection/element.mjs index f7473bc9e8..c0fe03b4c8 100644 --- a/src/collection/element.mjs +++ b/src/collection/element.mjs @@ -114,6 +114,10 @@ let Element = function( cy, params, restore = true ){ this.createEmitter(); + if( restore === undefined || restore ){ + this.restore(); + } + let bypass = params.style || params.css; if( bypass ){ util.warn('Setting a `style` bypass at element creation should be done only when absolutely necessary. Try to use the stylesheet instead.'); @@ -121,10 +125,6 @@ let Element = function( cy, params, restore = true ){ this.style(bypass); } - if( restore === undefined || restore ){ - this.restore(); - } - }; export default Element; diff --git a/src/extensions/renderer/base/coord-ele-math/rendered-style.mjs b/src/extensions/renderer/base/coord-ele-math/rendered-style.mjs index b260fdadbb..563b687e07 100644 --- a/src/extensions/renderer/base/coord-ele-math/rendered-style.mjs +++ b/src/extensions/renderer/base/coord-ele-math/rendered-style.mjs @@ -81,6 +81,8 @@ BRp.onUpdateEleCalcs = function( fn ){ BRp.recalculateRenderedStyle = function( eles, useCache ){ let isCleanConnected = ele => ele._private.rstyle.cleanConnected; + if (eles.length === 0) { return; } + let edges = []; let nodes = []; @@ -101,6 +103,12 @@ BRp.recalculateRenderedStyle = function( eles, useCache ){ rstyle.clean = false; } + if (ele.isEdge() && ele.isBundledBezier()) { + if (ele.parallelEdges().some(ele => !ele._private.rstyle.clean && ele.isBundledBezier())) { + rstyle.clean = false; + } + } + // only update if dirty and in graph if( (useCache && rstyle.clean) || ele.removed() ){ continue; } diff --git a/src/style/apply.mjs b/src/style/apply.mjs index 621b8ba773..6bb0d045bf 100644 --- a/src/style/apply.mjs +++ b/src/style/apply.mjs @@ -798,7 +798,9 @@ styfn.checkTrigger = function( ele, name, fromValue, toValue, getTrigger, onTrig let prop = this.properties[ name ]; let triggerCheck = getTrigger( prop ); - if( triggerCheck != null && triggerCheck( fromValue, toValue ) ){ + if (ele.removed()) { return; } + + if( triggerCheck != null && triggerCheck( fromValue, toValue, ele ) ){ onTrigger(prop); } }; @@ -813,27 +815,22 @@ styfn.checkBoundsTrigger = function( ele, name, fromValue, toValue ){ this.checkTrigger( ele, name, fromValue, toValue, prop => prop.triggersBounds, prop => { ele.dirtyCompoundBoundsCache(); ele.dirtyBoundingBoxCache(); + }); +}; - // if the prop change makes the bb of pll bezier edges invalid, - // then dirty the pll edge bb cache as well - if( // only for beziers -- so performance of other edges isn't affected - prop.triggersBoundsOfParallelBeziers - && ( name === 'curve-style' && (fromValue === 'bezier' || toValue === 'bezier') ) - ){ - ele.parallelEdges().forEach(pllEdge => { - pllEdge.dirtyBoundingBoxCache(); - }); - } - - if( - prop.triggersBoundsOfConnectedEdges - && ( name === 'display' && (fromValue === 'none' || toValue === 'none') ) - ){ - ele.connectedEdges().forEach(edge => { - edge.dirtyBoundingBoxCache(); - }); - } +styfn.checkConnectedEdgesBoundsTrigger = function( ele, name, fromValue, toValue ){ + this.checkTrigger( ele, name, fromValue, toValue, prop => prop.triggersBoundsOfConnectedEdges, prop => { + ele.connectedEdges().forEach(edge => { + edge.dirtyBoundingBoxCache(); + }); + }); +}; +styfn.checkParallelEdgesBoundsTrigger = function( ele, name, fromValue, toValue ){ + this.checkTrigger( ele, name, fromValue, toValue, prop => prop.triggersBoundsOfParallelEdges, prop => { + ele.parallelEdges().forEach(pllEdge => { + pllEdge.dirtyBoundingBoxCache(); + }); }); }; @@ -842,6 +839,8 @@ styfn.checkTriggers = function( ele, name, fromValue, toValue ){ this.checkZOrderTrigger( ele, name, fromValue, toValue ); this.checkBoundsTrigger( ele, name, fromValue, toValue ); + this.checkConnectedEdgesBoundsTrigger( ele, name, fromValue, toValue ); + this.checkParallelEdgesBoundsTrigger( ele, name, fromValue, toValue ); }; export default styfn; diff --git a/src/style/properties.mjs b/src/style/properties.mjs index b3a76df252..6205b87d34 100644 --- a/src/style/properties.mjs +++ b/src/style/properties.mjs @@ -240,7 +240,19 @@ const styfn = {}; ]; let visibility = [ - { name: 'display', type: t.display, triggersZOrder: diff.any, triggersBounds: diff.any, triggersBoundsOfConnectedEdges: true }, + { + name: 'display', + type: t.display, + triggersZOrder: diff.any, + triggersBounds: diff.any, + triggersBoundsOfConnectedEdges: diff.any, + triggersBoundsOfParallelEdges: (fromValue, toValue, ele) => { + if (fromValue === toValue) { return false; } + + // only if edge is bundled bezier (so as not to affect performance of other edges) + return ele.pstyle('curve-style').value === 'bezier'; + } + }, { name: 'visibility', type: t.visibility, triggersZOrder: diff.any }, { name: 'opacity', type: t.zeroOneNumber, triggersZOrder: diff.zeroNonZero }, { name: 'text-opacity', type: t.zeroOneNumber }, @@ -361,7 +373,18 @@ const styfn = {}; { name: 'line-outline-color', type: t.color }, { name: 'line-gradient-stop-colors', type: t.colors }, { name: 'line-gradient-stop-positions', type: t.percentages }, - { name: 'curve-style', type: t.curveStyle, triggersBounds: diff.any, triggersBoundsOfParallelBeziers: true }, + { + name: 'curve-style', + type: t.curveStyle, + triggersBounds: diff.any, + triggersBoundsOfParallelEdges: (fromValue, toValue) => { + if (fromValue === toValue) { return false; } // must have diff + + return ( + fromValue === 'bezier' || // remove from bundle + toValue === 'bezier'); // add to bundle + } + }, { name: 'haystack-radius', type: t.zeroOneNumber, triggersBounds: diff.any }, { name: 'source-endpoint', type: t.edgeEndpoint, triggersBounds: diff.any }, { name: 'target-endpoint', type: t.edgeEndpoint, triggersBounds: diff.any },