profectus/Profectus
Template
1
0
Fork 4
Code Issues 38 Pull requests 6 Projects 2 Releases Packages Wiki Activity Actions

Fix branchedResetPropagation #57

Merged
thepaperpilot merged 3 commits from nif/Profectus-Niffix:main into main 2024-02-14 17:39:07 +00:00
Conversation 1 Commits 3 Files changed 1 +15 -28
1 changed files with 15 additions and 28 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

43
src/features/trees/tree.ts
View file

@ -338,34 +338,21 @@ export const branchedResetPropagation = function (
tree: GenericTree, tree: GenericTree,
resettingNode: GenericTreeNode resettingNode: GenericTreeNode
): void { ): void {
const visitedNodes = [resettingNode]; const links = unref(tree.branches);
let currentNodes = [resettingNode]; if (links == null) return;
thepaperpilot commented 2024-02-13 14:21:47 +00:00
Outdated
Review
Copy link

instead of === undefined, can you use == null? That matches the rest of the codebase and has subtly different behavior.

instead of `=== undefined`, can you use `== null`? That matches the rest of the codebase and has subtly different behavior.
nif commented 2024-02-13 15:36:12 +00:00
Outdated
Review
Copy link

You very well could, I didn't realise that == null tested for undefined

You very well could, I didn't realise that `== null` tested for `undefined`
if (tree.branches != null) { const reset: GenericTreeNode[] = [];
const branches = unref(tree.branches); let current = [resettingNode];
while (currentNodes.length > 0) { while (current.length != 0) {
const nextNodes: GenericTreeNode[] = []; const next: GenericTreeNode[] = [];
thepaperpilot commented 2024-02-13 14:20:46 +00:00
Outdated
Review
Copy link

The linter says next, node, and link should all be const

The linter says next, node, and link should all be `const`
nif commented 2024-02-13 15:36:53 +00:00
Outdated
Review
Copy link

that's probably because i use Array.prototype.concat and Array.prototype.push instead of reassigning them

that's probably because i use `Array.prototype.concat` and `Array.prototype.push` instead of reassigning them
currentNodes.forEach(node => { for (const node of current) {
branches for (const link of links.filter(link => link.startNode === node)) {
.filter(branch => branch.startNode === node || branch.endNode === node) if ([...reset, ...current].includes(link.endNode)) continue
.map(branch => { next.push(link.endNode);
if (branch.startNode === node) { link.endNode.reset?.reset();
return branch.endNode; }
} };
return branch.startNode; reset.push(...current);
thepaperpilot commented 2024-02-13 14:22:59 +00:00
Outdated
Review
Copy link

This line is fine, but I wonder why you don't just push like you do the next array.

This line is fine, but I wonder why you don't just `push` like you do the `next` array.
nif commented 2024-02-13 15:37:20 +00:00
Outdated
Review
Copy link

because then that gives a shaped array that can't be iterated over as well

because then that gives a shaped array that can't be iterated over as well
nif commented 2024-02-13 15:37:58 +00:00
Outdated
Review
Copy link

wait no i could have .push(...'ed

wait no i could have `.push(...`'ed
}) current = next;
.filter(node => !visitedNodes.includes(node))
.forEach(node => {
// Check here instead of in the filter because this check's results may
// change as we go through each node
if (!nextNodes.includes(node)) {
nextNodes.push(node);
node.reset?.reset();
}
});
});
currentNodes = nextNodes;
visitedNodes.push(...currentNodes);
}
} }
}; };