Implement Feature/multicolor wires on refactored code #96
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
build_examples supports cleaning examples and intelligently will detect new examples. SUGGESTION: When merging into dev, require ``` build_examples.py clean ``` and then only build when merging into master branch
aakatz3
changed the title
aakatz3
changed the title
aakatz3
force-pushed
the
feature/multicolor-wires
branch
2 times, most recently
from
bbfd131
to
6013d05
Compare
Fixup github actions
aakatz3
force-pushed
the
feature/multicolor-wires
branch
from
6013d05
to
a3f2134
Compare
|
Thanks. I will try it out later today and will let you know my thoughts! I might do some light tweaking before merging into In the meantime, please have a look at my comments in #94, I'm not 100% convinced just cleaning the examples is the best way forward. |
|
Gave this a try this morning, works great!
|
|
I played a little bit directly with graphviz and the Unfortunately it only seems to work by stacking subgraph that have more than one edge. I'm not sure if this is a feasable way to achieve an even better looking output or if I accidentally found a bug in graphviz. 😁 Anyway here is the file content (Done with |
|
I like the color band output as an optional alternative to the stripes as there are wires of both kind of multi-coloring. It is easy to also support banded wire coloring inside the cable node to match the edges. However, as already mentioned by others, we have no knowledge about the length of each edge, and hence, it's hard to decide how many colored bands each edge should be split into. You have selected 5 white + 5 colored bands on each edge, and that fits well in this diagram, but a different diagram might have some long edges and some short ones. A 5+5 split for long edges will have long bands and for short edges will have short bands. I don't know how to keep the bands equal along a short edge on one side, inside the cable node, and along a long edge on the other side. |
|
that does look pretty nice, and you could even achieve the effect inside the cable node by splitting the colored cell vertically into stripes as well! For now, I'm happy to have multicolor lengthwise stripes working, but I think it's fitting to create a new |
I agree. Unfortunately using a solid color as background and using Sadly graphviz is rather limited in styling the edges. A minor annoyance is that the crossings are not drawn correctly because all of the background is drawn first and the colors are drawn after that. It would look nicer if background and foreground of an edge would be drawn together before the next edge is drawn. But when I use a subgraph for each connection or try to draw foreground and background inside the same subgraph graphviz does not draw them on top of each other. That's why I suspect that my working example is actually a bug in graphviz. I will try more the next days. |
|
Hey, playing around further I can produce nice crossings of the edges. I also found out that subgraphs are not needed. Interestingly it is enough to throw in an invisible edge. After that edge graphviz draws edges on top of each other just as we want to.
@kvid For your point about stripes and wire length I have no solution for now. I can use multiple graphs (black, first color and second color dashed) but I don't like the output. It heavenly depends on the format and renderer (in case of for example svg, pdf) how the output will look like. In general I also think that dashes are to short. Left is dashed, right is dotted:
And for example a screenshot of the svg rendered in firefox has uneven dashes and whitespace 🤔
|
|
@SnowMB this is fantastic work! Please start a new issue (something like banded vs. striped wires) so it doesn't get buried in an old PR :) Feel free to copy & paste your existing examples there! |
4 tasks
This was referenced Oct 12, 2020
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a rewrite/merge for #17, after a development reset. I decided to do this in a new branch instead of in the existing one due to the difficulty in directly merging it. This is now fully ready for review! 😃
Please see #17 for additional history
This closes #17 and closes #12
It should also close #52
It also removes prebuilt examples from the dev branch, and adds artifacts to examples from GitHub actions
Requesting review from @formatc1702 and @kvid