Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

State declarations inside compound states not drawn correctly #116

Closed
sonntam opened this issue Mar 25, 2020 · 15 comments
Closed

State declarations inside compound states not drawn correctly #116

sonntam opened this issue Mar 25, 2020 · 15 comments
Labels

Comments

@sonntam
Copy link

sonntam commented Mar 25, 2020

First of all I think this is the best javascript library for drawing state machines I could find - and i found a lot of libraries. This is the only one that handles compound states very well.

I seem to have found a small bug: The display of state declarations inside compound states are quirky:

state-machine

Expected Behavior

The line separating the state name and its declarations should expand to the whole width of the compound state node.

Current Behavior

The separating line is too short.

Steps to Reproduce (for bugs)

Example state machine:

initial,
"media player off": 
    entry / init()
    do / idle()
    exit / poweron(),
"media player on" : 
    entry / init()
    do / idle()
    exit / poweroff() {
    mpo.initial, stopped, playing, paused;
    mpo.initial => stopped;
    stopped => playing : play;
    playing => stopped : stop;
    playing => paused  : pause;
    paused  => playing : pause;
    paused  => stopped : stop;
};
initial            => "media player off";
"media player off" => "media player on" : power;
"media player on"  => "media player off" : power;

Context

I'm trying to let the program draw state machines with nested compound states and declarations

Your Environment

Web browser Google Chrome 80.0.3987.149 64-bit
pointed to the online editor (https://state-machine-cat.js.org/)

@sverweij
Copy link
Owner

hi @sonntam thanks for raising this - and I agree it should go edge to edge.

I've tried to get it better than that with the underlying software (GraphViz)

When implementing nested states I ran into the limits of the software state-machine-cat uses as graphical renderer currently. While nested states still look like states, for GraphViz they're not nodes, but 'clusters'. It is possible to make clusters look sort of like a node (/state), but some hacks were required. The hack here is to use a table for the name of the state and actions etc, hide all table borders and cell, except the one below the name of the state:

image

It is possible to make this a bit wider on a per case basis, e.g. here setting the minimum width of the table to 200 units would yield

image

... which is closer, but is still no cigar. Moreover, deciding case by case doesn't scale very well. E.g. it's quite hard to decide what an optimal minimum width would be. In the sample above you see it's already affecting the optimum width... So I made a judgement call and decided to embrace the default that is not perfect (measured against how it looks in many text books), but which does work in all cases.

=> If there's a better solution out there (either with GraphViz or with another render engine) I'm all ear!

One of the reasons I wrote state-machine-cat is to hide these hacks, so I wouldn't have to
apply them each time I wrote a state diagram in GraphViz. Proof that this works is that I
had almost forgotten how this specific hack worked ...

@sonntam
Copy link
Author

sonntam commented Mar 26, 2020

@sverweij Thank you for the feedback! You motivated me to have a look at this myself.

Because you say that it is a limitation of graphviz/viz.js I came up with something you probably won't like. I added a hack that manipulates the viz.js output so that the line is correct in the final product. For this I had to add a dependency for a DOM manipulation library. I used jsdom - maybe you know of another library that isn't that huge.

Here's a peek:

state-machine2

The placing/justification of the text labels could be manipulated like this as well.

If you are interested I'll open up a fork and push my hack to it.

@sonntam
Copy link
Author

sonntam commented Mar 26, 2020

I couldn't resist 😅

Here is the commit: 85e8fbd

@sonntam
Copy link
Author

sonntam commented Mar 27, 2020

Parallel states are drawn differently. I added even more hacks to handle those as well. See the tip of the issue_116 branch for details.

state-machine3

@sonntam
Copy link
Author

sonntam commented Mar 27, 2020

@sverweij Do you think we could incorporate such a workaround in your repository. If so, what would your preferred approach be?

Some possibilities could be

  • a new render function, e.g. svgmod that encapsulates the default svg renderer and includes the modification code for viz.js output
  • add a configuration option for the svg renderer that can enable/disable the hack (also move the modification code to a separate .js file)
  • Just hacking viz.js svg output like I did above

Also what about the jsdom dependency?

If you let me know how you would approach this I could try to assist you by e.g. generating a PR. I'd also understand if you don't want that kind of hack in your program - I would also be fine with that decision 😉

@sverweij
Copy link
Owner

@sonntam hacking on the generated svg - that's creative 😄 , thanks for the link to your proof of concept!

The jsdom dependency would indeed a be concern - it's ~2Mb (520kb zipped), which would increase state-machine-cat's foot print quite a bit (which is already large with a 2.1Mb viz.js dependency...). I'm not sure I'm willing to incur that - especially for running in the browser. A way to mitigate would be to in the browser use native dom functions and in node jsdom.

Do know that, given the state of viz.js (archived) - and limitations like you bumped into I'm looking into alternative render methods (e.g. with elk/ sprotty or directly in d3.js). It'll not be on the short term, but it's inevitable...

For parallel states I had to compromise as well as you've encountered. For vertical orientations it's supposed to have the orthogonal regions divide the state up (left picture), but with GraphViz I haven't been able to do that (right picture). One more reason to look into another render engine...
imagevsimage

@sonntam
Copy link
Author

sonntam commented Mar 28, 2020

Thanks, I absolutely agree with you!

The main issue with using a different renderer (I tried e.g. d3, cytoscape, webcola and mxGraph) will be with the layouting. None of the available layouting algorithms I found were able to produce satisfying results (which, of course, could have something to do with me failing to use their potentials...).

For D3.js there is dagre-d3 layouting which is based on dagre.js which in turn is based on graphviz‘ dot engine rewritten in js. This, of course, has got the exact same limitations as GraphViz/viz.js but may be a way out as its development is still on-going and improvements are being made.

Sent with GitHawk

@sverweij
Copy link
Owner

yeah - have tried the ones you mentioned as well and kept getting back to graphviz because it has most features needed to pull this of at all, where most of them just don't have enough of them (nesting, render speed, decent labeling, proper layout(!))

I do have high hopes for ELK.js (based on the Eclipse Layout Kernel)/ sprotty, though (check this humongous state chart for instance), although its java heritage still scares me a bit.

@stale
Copy link

stale bot commented Apr 4, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale label Apr 4, 2020
@sonntam
Copy link
Author

sonntam commented Apr 6, 2020

This is maybe a bit offtopic: I used your library within a statecharts library for node-red based on xstate. It works really well for me.

I incorporated my "hack" I posted above in that package and also use DOM modifications/CSS to "animate" the chart and show the live states without reloading. It works really nicely!

However one user reported that the rendering of a simple machine took about 40 seconds on his RaspberryPi2. So probably moving over to another library at least for rendering may be a good idea 😉

P.S.: I had good fun playing with smcat, thanks - nice work!

@stale stale bot removed the stale label Apr 6, 2020
@sverweij
Copy link
Owner

sverweij commented Apr 6, 2020

Hi @sonntam - thanks for the links - that looks very cool, I'll check it out tomorrow, I'm super curious! (Also didn't know node-red till 20 minutes ago, seems worth checkout out as well :-) )

For the performance problems on a RaspberryPi1 - using native graphviz might fix that ...

My bet is that the graphviz-compiled-to-javascript thing (viz.js) state-machine-cat uses is the culprit. Even on my 2017 Mac the difference between that and the native graphviz is very noticeable. The only reason for using viz.js vs the real thing a.t.m. is that I wanted it to run in the browser.

I did build in an escape-hatch though, so I could use the native graphviz when not in the browser: switch the output type to 'dot' and pipe it through graphviz.

The atom-state-machine-cat plugin has an option to do that for instance (check renderGraphVizWithCLI.js - which is triggered when the useGraphvizCommandLine is switched). I now see there's something on npm that's doing something similar.

But anyway - another reason to the ever growing list to find a better render lib!

@sonntam
Copy link
Author

sonntam commented Apr 9, 2020

@sverweij Good hint! I'm trying to do exactly that now and get a few performance measurements. Thanks for the links.

I think we can close the issue with a "do not fix" 😊

@sonntam
Copy link
Author

sonntam commented Apr 9, 2020

Preliminary measurements with a small test example:

1d48331a.6cd23d/initial,
1d48331a.6cd23d.count/atomic [label="count" type=regular],
1d48331a.6cd23d.reset/atomic [label="reset" type=regular] :
    entry / resetCounter()
    do / doStuff();

1d48331a.6cd23d/initial => 1d48331a.6cd23d.count/atomic;
1d48331a.6cd23d.count/atomic => 1d48331a.6cd23d.reset/atomic : [maxValueReached];
1d48331a.6cd23d.count/atomic => 1d48331a.6cd23d.count/atomic : after 1 s / incrementCounter();
1d48331a.6cd23d.reset/atomic => 1d48331a.6cd23d.count/atomic : after 5 s;

On a RaspberryPi4:

npx smcat -Tdot: 0.997s
npx smcat -Tsvg: 1.775s
npx smcat -Tdot | dot -Tsvg: 1.027s (!)

Here it makes quite a difference

On my windows 10 machine with Core i5-750

npx smcat -Tdot: 1.224s
npx smcat -Tsvg: 1.285s
npx smcat -Tdot | dot -Tsvg: 1.264s

Not much difference and quite astonishing the the Pi4 is sometimes quicker than my fully fledged desktop workstation (with an old processor from 2009 though). Some of the overhead may come from using npx.

@sverweij
Copy link
Owner

sverweij commented Apr 9, 2020

hmm @sonntam - interesting, thanks. The performance differences on the RPI4 seem to be similar to what I recall. I wonder where those 40s rendering time on the RPI came from. The lack of difference on win10 is really surprising. Warmup difference?

Some of the overhead may come from using npx.
npx will definitely give an overhead, but it's fixed and should not influence the absolute differences between the runs too much.

(Note to self - there is no repeatable performance test for state-machine-cat yet. Might be smart to add that some time in the future, even if it's something as simple as currently set up for dependency-cruiser (with hyperfine)).

@stale
Copy link

stale bot commented Apr 17, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants