Skip to content
This repository has been archived by the owner on Oct 2, 2020. It is now read-only.

fix negative thickness of polylines in 4xxx.lib #1059

Closed
wants to merge 2 commits into from
Closed

fix negative thickness of polylines in 4xxx.lib #1059

wants to merge 2 commits into from

Conversation

gaensli
Copy link
Contributor

@gaensli gaensli commented Oct 11, 2018

tl;dr, negative line thickness creates awful filled polygons.

See https://bugs.launchpad.net/kicad/+bug/1797283 for the genesis of this patch.

@CLAassistant
Copy link

CLAassistant commented Oct 11, 2018

CLA assistant check
All committers have signed the CLA.

@evanshultz
Copy link
Collaborator

@poeschlr
This is related to https://www.mail-archive.com/kicad-developers@lists.launchpad.net/msg32081.html, I think. Do you want to handle this differently? Or will the GAL code be changed to render like v5?

@gaensli
Copy link
Contributor Author

gaensli commented Oct 14, 2018

@evanshultz @poeschlr I'm open for feedback. The violations that travis reports were not introduced with this commit.

@antoniovazquezblanco antoniovazquezblanco added Enhancement Improves existing symbol in the library Pending reviewer A pull request waiting for a reviewer labels Oct 15, 2018
@gaensli
Copy link
Contributor Author

gaensli commented Oct 16, 2018

This has already caused 2 duplicate bug reports being opened over at launchpad.
What's blocking from merging this PR?

@evanshultz
Copy link
Collaborator

@poeschlr

@poeschlr
Copy link
Collaborator

I am open to fixing this on the symbol side. But it should also be fixed in opengl.

However the value of -1000 was choosen to result in no outline shown. 10 mil is therefore too much. I would go with the lowest value that is not replaced with the default. So 1 mil.

@gaensli
Copy link
Contributor Author

gaensli commented Oct 17, 2018

Ok, changed to 1 mil, but I doubt that's what users want...
screenshot from 2018-10-17 14-48-23

@evanshultz
Copy link
Collaborator

https://www.mail-archive.com/kicad-developers@lists.launchpad.net/msg32090.html
https://www.mail-archive.com/kicad-developers@lists.launchpad.net/msg32100.html

Those messages discuss doing exactly this, using 1mil line thickness. But the message after it says that Jeff backed out a change. I do not know what version of KiCad you're using and if the rendering you're showing is how it will be in 5.1.

What build are you using? Did you change the symbol in any way to produce the above screenshot? I see a 10mil horizontal line on top and bottom of 4001 to provide a consistent symbol outline thickness. I also see the background filled. Neither of this appear above, but I don't know if that is solely the result of the new GAL.

Last, note that we should be careful because this library will be used for all KiCad users during the 5.0 cycle before 5.1 is released. I did a quick check and I don't see any issues with 1mil lines in 5.0, but note that most of them are covered up by a 10mil line.

@gaensli
Copy link
Contributor Author

gaensli commented Oct 18, 2018

I'm using:
Application: eeschema
Version: (6.0.0-rc1-dev-937-gc22a247), debug build
Libraries:
wxWidgets 3.0.2
libcurl/7.47.0 GnuTLS/3.4.10 zlib/1.2.8 libidn/1.32 librtmp/2.3
Platform: Linux 4.4.0-137-generic x86_64, 64 bit, Little endian, wxGTK
Build Info:
wxWidgets: 3.0.2 (wchar_t,wx containers,compatible with 2.8) GTK+ 2.24
Boost: 1.58.0
Curl: 7.47.0
Compiler: GCC 5.4.0 with C++ ABI 1009

Build settings:
USE_WX_GRAPHICS_CONTEXT=OFF
USE_WX_OVERLAY=OFF
KICAD_SCRIPTING=OFF
KICAD_SCRIPTING_MODULES=OFF
KICAD_SCRIPTING_WXPYTHON=OFF
KICAD_SCRIPTING_ACTION_MENU=OFF
BUILD_GITHUB_PLUGIN=ON
KICAD_USE_OCE=OFF
KICAD_USE_OCC=OFF
KICAD_SPICE=OFF

I had background set to white, so that's why.. Changing it to some yellowish color gives me:
screenshot from 2018-10-18 02-42-31
For that screenshot is used the commits as here https://github.com/KiCad/kicad-symbols/pull/1059/files

@gaensli
Copy link
Contributor Author

gaensli commented Oct 18, 2018

PS: Maybe the confusion is that I was using latest on master, versus 5.0.0 or 5.0.1
All screenshots from above from me where with latest on master. (new gal)
Whereas with 5.0.0 and 5.0.1 everything looks ok. (legacy canvas)

So one could argue, that I'm ahead of the released versions and that's to be fixed later, but it causes bugs being reported among nightly users... I dunno

@evanshultz
Copy link
Collaborator

Yes, I understood that and it's why I'm not in a panic.

It is not clear from the above discussion how the GAL renderer will perform. Using 1mil was suggested by JP but I'm uncomfortable merging this since so many things are up in the air now and we're quite far from even a RC1. And since this doesn't affect 5.0 and is not totally fixing 5.1 I don't see a great impact. If @poeschlr wants to merge or ask me to merge that's fine, since it is some incremental improvement for master, but there's still figuring out to do.

@poeschlr
Copy link
Collaborator

I agree with @evanshultz that we might want to hold of at least till the feature freeze of 5.1.
After that we can look into these issues.

@DanSGiesbrecht
Copy link
Collaborator

Should we use the v6 milestone to track this, understanding that it can be resolved after v5.1, before v6?

@evanshultz
Copy link
Collaborator

While this was definitely an issue, I believe it was one of the teething pains of moving Eeschema to the new rendering engines. The discussions mentioned earlier to the devs appears to have resulting in a fix over the last few months. Here is a recent nightly using the Modern Toolset rendering:
image

I think we can close this PR. (But we should definitely make an update of the logic symbols a major target of the v6 libs IMO.)

Application: kicad
Version: (6.0.0-rc1-dev-1521-g81a0ab4d7), release build
Libraries:
wxWidgets 3.0.4
libcurl/7.61.1 OpenSSL/1.1.1 (WinSSL) zlib/1.2.11 brotli/1.0.6 libidn2/2.0.5 libpsl/0.20.2 (+libidn2/2.0.5) nghttp2/1.34.0
Platform: Windows 8 (build 9200), 64-bit edition, 64 bit, Little endian, wxMSW
Build Info:
wxWidgets: 3.0.4 (wchar_t,wx containers,compatible with 2.8)
Boost: 1.68.0
OpenCASCADE Community Edition: 6.9.1
Curl: 7.61.1
Compiler: GCC 8.2.0 with C++ ABI 1013

Build settings:
USE_WX_GRAPHICS_CONTEXT=OFF
USE_WX_OVERLAY=OFF
KICAD_SCRIPTING=ON
KICAD_SCRIPTING_MODULES=ON
KICAD_SCRIPTING_PYTHON3=OFF
KICAD_SCRIPTING_WXPYTHON=ON
KICAD_SCRIPTING_WXPYTHON_PHOENIX=OFF
KICAD_SCRIPTING_ACTION_MENU=ON
BUILD_GITHUB_PLUGIN=ON
KICAD_USE_OCE=ON
KICAD_USE_OCC=OFF
KICAD_SPICE=ON

@gaensli gaensli closed this Feb 19, 2019
@myfreescalewebpage myfreescalewebpage added Abandoned Original author has stopped working on the PR and removed Pending reviewer A pull request waiting for a reviewer labels Apr 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Abandoned Original author has stopped working on the PR Enhancement Improves existing symbol in the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants