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

colors: tweak default colors to work better on black background #5982

Merged

Conversation

oharboe
Copy link
Collaborator

@oharboe oharboe commented Oct 18, 2024

Made some darker colors brighter and easier to see. A side-by-side review of all the colors needs to be done, I don't know exactly where to find them in the GUI though.

Before:

image

After:

image

Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@gadfort gadfort self-requested a review October 18, 2024 12:25
@gadfort
Copy link
Collaborator

gadfort commented Oct 18, 2024

@oharboe it might be nice to have a side by side of all the color changes you are proposing

@oharboe
Copy link
Collaborator Author

oharboe commented Oct 18, 2024

@oharboe it might be nice to have a side by side of all the color changes you are proposing

Yes, but to do that, I would need to know how to create side-by-side screenshots of each color in that table, which I don't...

I think we should hold off on this PR until we have such a side-by-side comparison.

It is a bit tricky, because it requires showing each color in one or more use-cases.

@maliberty
Copy link
Member

Just brightening it will make it confusing with metal2

@oharboe
Copy link
Collaborator Author

oharboe commented Oct 18, 2024

Will have a look. Is that the only conflict?

@maliberty
Copy link
Member

How were these colors derived? Please make a before and after comparison images.

@oharboe
Copy link
Collaborator Author

oharboe commented Oct 19, 2024

How were these colors derived? Please make a before and after comparison images.

I'm going to revert all color changes but the one that is most problematic. The others looked a bit darker to me too, but it is a bridge to far. The problem is to create lots of easy on the eye high contrast colors... Which is enormously tricky. :-)

@oharboe
Copy link
Collaborator Author

oharboe commented Oct 19, 2024

Instead of changing multiple colors, I've changed only metal 5 and tried to introduce an additional different base color, bright orange, that works on black background.

Before:

image

After:

image

@oharboe
Copy link
Collaborator Author

oharboe commented Oct 19, 2024

I'm going to try on more monitors, but on one either was OK, on my laptop the new scheme was definitely better.

Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

1 similar comment
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@gadfort
Copy link
Collaborator

gadfort commented Oct 19, 2024

Attempting to keep the color similar with higher contract with the adjacent layers: QColor(222, 33, 96)

image

This preserves the relative contract between M4 / M6 and increases the contrast with the background (although not as much as the orange).

@oharboe
Copy link
Collaborator Author

oharboe commented Oct 19, 2024

Thanks! Will try on some more monitors.

@maliberty maliberty linked an issue Oct 19, 2024 that may be closed by this pull request
@oharboe
Copy link
Collaborator Author

oharboe commented Oct 20, 2024

@gadfort Used your proposed color. Works better on the monitors I tried. 👍

Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@maliberty maliberty merged commit 9ca8405 into The-OpenROAD-Project:master Oct 21, 2024
10 checks passed
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.

Default colors in ASAP7 are too dark when used on black
3 participants