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

Add support for Multicolored Wires #17

Closed
wants to merge 15 commits into from

Conversation

aakatz3
Copy link
Contributor

@aakatz3 aakatz3 commented Jun 25, 2020

Closed #12, adding support for multicolored wires, along with example. Also extends the DIN color code, and adds the following color codes:

  • T568A
  • T568B
  • even-count color code/25 pair color code/Telco color code

Copy link
Contributor

@kimmoli kimmoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No solution to provide, but seems the Wire table shows a gradient of two colors, not stripes as the wire
2020-06-25_15-55-25

I tried to make the wires look like as real wires in these DIN cables, but seems graphviz edge can not do it easily.
2020-06-25_15-46-52

@aakatz3
Copy link
Contributor Author

aakatz3 commented Jun 25, 2020

GraphViz can make the wires banded in the stripes, however it causes half of the wire to be one color, the other half to be the other, and would require removing the black borders. I ended up adopting the twisted stripe commonly seen in CAT5/CAT6A cables.

For the gradient, It may be possible to have it add multiple "cells" that have the additional wire colors (this may be the best option so the thickness matches), but I thought it was likely better to have the gradient than to just have the primary color, so I left that default behavior in.

CAT6A

The gradient behavior happens due to line 152 of wireviz.py:
bgcolor = ":".join(wv_colors.translate_color(x, 'hex').split(':')[:2])

This line suppresses the warning about additional colors being ignored. To just have the major/first color, you can either change the 2 to a 1, or replace the line with the following:

bgcolor = wv_colors.translate_color(x[:2], 'hex')

src/wireviz.py Outdated
@@ -141,7 +142,7 @@ def create_graph(self):
for bla in p:
html = html + '<td>{}</td>'.format(bla)
html = html + '</tr>'
bgcolor = wv_colors.translate_color(x, 'hex')
bgcolor = ":".join(wv_colors.translate_color(x, 'hex').split(':')[:2])
html = html + '<tr><td colspan="{colspan}" cellpadding="0" height="6" bgcolor="{bgcolor}" border="2" sides="tb" port="{port}"></td></tr>'.format(colspan=len(p), bgcolor=bgcolor if bgcolor != '' else '#ffffff', port='w{}'.format(i))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest changing height="6" to height="10" for two-colored wires to match the extra thickness of the connecting curved wires. The thickness seems to be 2x the number of color stripes (black edge + base color + stripe color + base color + black edge).

Copy link
Collaborator

@kvid kvid Jun 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An even better improvement (to avoid the color gradient) is to build the striped wire using multiple HTML rows with the port attribute only present in the middle row for each wire. See this excerpt from the upper wire in ex07.gv where the black edges are separated rows:

<tr><td colspan="3" cellpadding="0" height="2" bgcolor="#000000" border="0" sides="tb"></td></tr>
<tr><td colspan="3" cellpadding="0" height="6" color="#ffffff" bgcolor="#00ff00" border="2" sides="tb" port="w1">
<tr><td colspan="3" cellpadding="0" height="2" bgcolor="#000000" border="0" sides="tb"></td></tr>

It might be easier to to use one row (with no border) for each color in the generic case with multiple stripes.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SVG provides dashes ( https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/stroke-dasharray ) which provide a simple and clean way to add this. Draw a solid wire then a dashed wire.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just saw your changes, if you are able to, please check out my solution and let me knnow if you have any comments. I will try to review your pull request shortly.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just created this PR: aakatz3#4 with my suggested implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@njhurst While it would be nice to do this, there is a problem: Graphviz does not support this properly. When drawing dashed lines, you cannot specify an alternate color, and you can't seem to overlay them properly. One COULD modify the SVG after the fact, but it breaks compatibility with GraphViz

# Conflicts:
#	src/wireviz.py
@aakatz3
Copy link
Contributor Author

aakatz3 commented Jun 25, 2020

@kvid I merged your pull request in, please let me know if it looks good

@kvid
Copy link
Collaborator

kvid commented Jun 25, 2020

Thank you. It looks good, except I found a minor issue in the odd case of a wire with more than two colors. See aakatz3#5

Reverse colors to match the curved wires when more than 2 colors
@aakatz3
Copy link
Contributor Author

aakatz3 commented Jun 25, 2020

Fixed and merged to my branch. Thank you!

@X-Ryl669
Copy link

I think once there is a multicolored wire, all wires should be drawn with extended thickness, as it does not look very good with mixed thickness wires.

@kvid
Copy link
Collaborator

kvid commented Jun 26, 2020

@X-Ryl669 Do you prefer equal wire thickness within the same cable or globally in all cables? If we go in this direction, then maybe a difference in cable gauge also optionally could be visualized as a difference in wire thickness between different cables. Equal wire thickness can be implemented using the same number of color entries for these wires internally and for Graphviz, i.e. several equal color entries for unicolored wires.

@X-Ryl669
Copy link

Yes, it makes sense to have wire with same gauge using the same thickness (whether they are multi-colored or not).
Having wire with higher thickness for higher gauge is even better (but as I understand it currently, the gauge value is not interpreted, only used for mapping to AWG).

@formatc1702
Copy link
Collaborator

formatc1702 commented Jun 27, 2020

This looks very promising!

@njhurst While it would be nice to do this, there is a problem: Graphviz does not support this properly. When drawing dashed lines, you cannot specify an alternate color, and you can't seem to overlay them properly. One COULD modify the SVG after the fact, but it breaks compatibility with GraphViz

It is indeed possible to define multi-colored dashed lines in GraphViz (see example here); however, it might be hard to make it look good, especially making it look even between the GraphViz edges and the HTML table cell (which I guess could be subdivided to simulate stripes?).

While it would be cool, it might make the code unnecessarily complicated. What do you think?

@formatc1702 formatc1702 changed the base branch from master to dev June 27, 2020 10:13
@kvid
Copy link
Collaborator

kvid commented Jun 27, 2020

AFAIK, the wire gauge value is not currently inerpreted, but one approach could be to sort all unique gauge values, use the minimum thickness for the smallest mm^2 value (or highest AWG), add a constant to the thickness for the next gauge value, etc. If we need a maximum thicness limit, then close gauge values can be grouped and use equal thickness. Alternatively, a less functional, but simpler implementation could be to add an optional wire thickness attribute in the YAML file.

@formatc1702
Copy link
Collaborator

Perhaps it makes sense to split the wire gauge visualization into a separate issue, so as not to further complicate things. It would be a nice addition but is not necessarily related to banded/striped wires.

It will probably be necessary to allow for both lengthwise stripes (like Ethernet or PE) and color rings (like DIN 47100)... although lengthwise stripes seem to be easier to implement.

@X-Ryl669
Copy link

Agreed. It's just that this current implementation enlarge the wire if it's multicolor but doesn't do that if it's monochrome.

@aakatz3
Copy link
Contributor Author

aakatz3 commented Jun 27, 2020

In order to make the dashed lines work properly in graphviz, they would need to lose the black outlines, and we would need a way to count and calculate the number of stripes that should be placed. While you can put two or more colors into multicolored lines, I couldn't find a way to pattern them without just repeating, over and over, the color code.

A possible solution to enlarging single color wires instead of just tripling up the color may be to change the stroke width, but I do not know if this can be done for only the center spline/stroke.

@formatc1702
Copy link
Collaborator

I've recently made some rather big refactoring changes (#9), is it possible to rebase these changes onto the latest version to make merging easier?

@aakatz3
Copy link
Contributor Author

aakatz3 commented Jun 30, 2020

@formatc1702 I will try my best to do this ASAP. I will have to check out the new code, and will provide an update when I can.

# Conflicts:
#	.gitignore
#	src/batch.py
#	src/wireviz.py
#	src/wv_colors.py
@formatc1702
Copy link
Collaborator

There hasn't been any activity in the last 10 days, but I really would like to merge this ASAP.

@aakatz3 any news?
@kvid do you have the time to take over this issue, look over the requested changes and get this ready to merge into the current dev branch with no merge conflicts?

Only addendum to the code comments: Ideally please revert any changes to the example output files, keeping only the input .yml files, so the diff of the commit is not so big. I will rebuild and publish all examples when we merge into master.

@kvid
Copy link
Collaborator

kvid commented Jul 15, 2020

I'm a bit busy at day-time these days, but I can put in some effort at evenings to get this PR ready. Unless @aakatz3 suddenly gets back, I suggest I clone his branch aakatz3:feature-multicolor-wires and address the change requests and merge conflicts. Then I can create a new PR with my version of the branch referring to this one. This way @aakatz3 or someone else can continue this PR in case I fail or use too much time trying.

@formatc1702
Copy link
Collaborator

Thanks, appreciate it. See if you can rebase your feature branch onto your dev, I want to avoid having a tangle in the commit tree in the end :)

@aakatz3
Copy link
Contributor Author

aakatz3 commented Jul 16, 2020

I've been working on trying to rebase my commit tree, but I keep running into issues within the HTML, so I was trying to look into refactoring the HTML. I don't quite know the best way forward. I may do a squash rebase and submit a new pull request. Do you have any thoughts on the best way to tackle this?

On a separate note, I found the problem with UTF-8, encodings weren't hardcoded or detected, so on windows, it would write utf-8 but read as codepage2512, which caused the issue (or vice versa). My personal git tree is a major mess at this point trying to figure this out.

# Conflicts:
#	examples/ex01.bom.tsv
#	examples/ex01.gv
#	examples/ex01.html
#	examples/ex01.png
#	examples/ex01.svg
#	examples/ex05.gv
#	examples/ex05.html
#	examples/ex05.png
#	examples/ex05.svg
#	src/wireviz/Harness.py
#	src/wireviz/build_examples.py
@formatc1702
Copy link
Collaborator

formatc1702 commented Jul 16, 2020

@aakatz3 glad you're back!

Honestly, so much has changed since work on this issue started, my naïve approach would be to start from the current state of the dev branch, and manually build in the changes there.
No sense in desperately trying to untangle the commit tree! Not sure if the other co-authors take issue with removing the development history of this feature, but pragmatically speaking, it would be the easiest; just act as if you were starting from scratch from the current state of things.

@formatc1702
Copy link
Collaborator

This might mean starting a clean branch from dev, porting your changes to there, and submitting a new PR. Let me know if you want to move forward with this.

@kvid
Copy link
Collaborator

kvid commented Jul 16, 2020

What are the major differences between your last pushed commit and your personal commits? If you push your last commits in a separate temporary branch, then I can have a look and perhaps suggest a way forward.

@kvid
Copy link
Collaborator

kvid commented Jul 16, 2020

I am thinking it's better to merge than to rebase in this case to avoid a lot of non-trivial conflicts because files have been moved in both branches. I sugggest creating a branch before Merge branch 'dev' into feature-multicolor-wires to reduce the tangles, and we also avoid the two commits after this point that was not wanted by @formatc1702 as far as I understand (if any of them is wanted, they can be cherry picked afterwards):

  • Add Jetbrains IDE config
  • Update all examples

I tried this in my personal repo:

git branch aakatz3-multicolor e6317536f75ef5ed45dde0133c973a13145d704f
git checkout dev
git merge aakatz3-multicolor

I had to resolve these conflicts:

  • .gitignore (keeping changes from both branches)
  • src/wireviz.py (deleting file)
  • src/wireviz/build_examples.py (keeping the highest number of examples and tutorials)
  • src/wv_colors.py (deleting file)

After this merger, I could build all examples without errors, but I have not yet checked if all changes was merged in correctly.

@formatc1702
Copy link
Collaborator

formatc1702 commented Jul 16, 2020

I am thinking it's better to merge than to rebase in this case to avoid a lot of non-trivial conflicts because files have been moved in both branches.

That's precisely why I am advocating for a fresh start from dev, cleanly re-implementing the desired changes and getting rid of the messy history.
Choosing rebase over merge (or vice-versa) does nothing to avoid conflicts, they would have to be resolved either way.

aakatz3 added a commit to aakatz3/WireViz that referenced this pull request Jul 16, 2020
@aakatz3 aakatz3 force-pushed the feature-multicolor-wires branch 2 times, most recently from 2c063da to e4a034c Compare July 16, 2020 23:16
@aakatz3 aakatz3 marked this pull request as draft July 16, 2020 23:17
@aakatz3
Copy link
Contributor Author

aakatz3 commented Jul 16, 2020

Dev-reset complete, please see #96

@kvid
Copy link
Collaborator

kvid commented Jul 17, 2020

I did rebase aakatz3-multicolor described above onto dev and adapted the commits to use f-strings:
https://github.com/kvid/WireViz/tree/aakatz3-multicolor-rebased
Then I discovered the new PR #96 from @aakatz3 and stopped. It was a nice excercise though. Each commit is verified by running the build script and inspecting some critical diagrams. It would be fun to compare this with the commits in PR #96 if I have the time. :-)

formatc1702 pushed a commit that referenced this pull request Jul 17, 2020
@formatc1702
Copy link
Collaborator

I've merged #96 into my own feature branch, will tweak and merge into dev soon. Closing this one. Thanks everyone for the hard work.

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

Successfully merging this pull request may close these issues.

[feature] Multiple colors per wire
6 participants