Skip to content

Commit

Permalink
Add some bezier edge bug fixes
Browse files Browse the repository at this point in the history
- `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.
  • Loading branch information
maxkfranz committed Feb 4, 2025
1 parent 514f252 commit 3c7683a
Show file tree
Hide file tree
Showing 7 changed files with 83 additions and 37 deletions.
30 changes: 23 additions & 7 deletions playwright-tests/renderer.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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]);
Expand All @@ -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());
Expand All @@ -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' }
},
Expand All @@ -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' }
},
Expand All @@ -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

Expand Down
2 changes: 1 addition & 1 deletion src/collection/dimensions/bounds.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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())));

Expand Down
6 changes: 3 additions & 3 deletions src/collection/dimensions/edge-points.mjs
Original file line number Diff line number Diff line change
@@ -1,21 +1,21 @@
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() );
}
};

const ifEdgeRenderedPositions = (ele, getPoints) => {
if( ele.isEdge() ){
if( ele.isEdge() && ele.takesUpSpace() ){
let cy = ele.cy();
let pan = cy.pan();
let zoom = cy.zoom();
Expand Down
8 changes: 4 additions & 4 deletions src/collection/element.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -114,17 +114,17 @@ 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.');

this.style(bypass);
}

if( restore === undefined || restore ){
this.restore();
}

};

export default Element;
Original file line number Diff line number Diff line change
Expand Up @@ -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 = [];

Expand All @@ -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; }

Expand Down
39 changes: 19 additions & 20 deletions src/style/apply.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
};
Expand All @@ -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();
});
});
};

Expand All @@ -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;
27 changes: 25 additions & 2 deletions src/style/properties.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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 },
Expand Down Expand Up @@ -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 },
Expand Down

0 comments on commit 3c7683a

Please sign in to comment.