Illegal graph components delete graph on reload

[treefrogman]

Some part of the functionality that saves and retrieves projects in localStorage is strict about graphs, so if you create an illegal graph, such as the loop pictured below, and then you reload, you lose your whole project.

[maximecb]

Yeah. I've been wanting to add cycle detection for a little while but never got around to it 😅

Ideally, as soon as you add a new connection that creates a cycle, an error message should pop up and it should be prevented from happening. Cycles are allowed so long as they include a Delay or a Hold node.

Technically, the code in public/compiler.js is already able to detect cycles (see splitNodes and topoSort). Ideally we'd avoid running that code twice because it could be somewhat expensive on larger projects. Though I guess we could benchmark how much time it takes on large projects such as examples/2instruments_1207_nodes.ncft. If it's under 50ms then maybe it's ok to just run that every time a new connection is added 🤔

[TheCrimsonKing92]

Howdy there @maximecb! I have some initial results and wanted to check in on whether you had any other system profiles or constraints in mind beyond the 50ms runtime budget.

Environment:
OS: Windows 11 Home / 10.0.22621 Build 22621
Processor: i9-12900KF @ 3.2GHz
Storage: Western Digital Blue SN570
Browser: Google Chrome 111.0.5563.147 (Official Build) (64-bit)

Methods:

• Modified compiler.js to add console.time calls within topoSort
• Ran server locally
• Used Share functionality to "publish" examples/2instruments_1207_nodes.ncft to local SQLite
• Each set of runs, set CPU throttling if necessary in Chrome Dev Tools
• Each run:
  • End server process
  • Restart server with npm run start
  • Reload 2instruments_1207_nodes page with "Empty Cache and Hard Reload"
  • Record timing
• Aggregated min, max, mean, and median results (rounded to two decimals) over twenty runs per profile

Results:

• No CPU Throttling:
  • Min: 1.00 ms
  • Max: 7.07 ms
  • Mean: 1.81 ms
  • Median: 1.43 ms
• 4x CPU Throttling:
  • Min: 4.40 ms
  • Max: 7.90 ms
  • Mean: 5.81 ms
  • Median: 5.76 ms
• 6x CPU Throttling:
  • Min: 6.65 ms
  • Max: 47.63 ms
  • Mean: 14.18 ms
  • Median: 9.86 ms

Let me know your thoughts about proceeding with the previously mentioned implementation.

[maximecb]

Hi Miles.

Thanks for running these tests. The resulting times seem very reasonable.

Ideally, we would add something like a detectCycles() function to compiler.js and if someone adds a connection that produces a cycle, we would refuse to make the connection and report an error message. There is an errorDialog(message) function that you can use for that in dialog.js :)

[TheCrimsonKing92]

If you'd like to assign this issue to me, I'll work on that.

[maximecb]

If you'd like to assign this issue to me, I'll work on that.

Alrighty, the issue is yours :)

Thank you for helping.

[maximecb]

Fix is now deployed.