Symbols not drawn correctly after KiCad start

[Bernhard Stegmaier]

How to reproduce:
(1) Open project attached to bug
(2) Close KiCad
(3) Start KiCad (project manager)
(4) Open schematic

Some symbols are drawn not at all or incomplete - see attached bug.png, pins of transistor are missing and complete connectors are missing.
If I open another project and then switch back to this project the symbols are drawn correctly - or at least more symbols are drawn correctly. See attached bug2.png, transistor and 3-pin connector is now fine, but 4-pin connector still missing.

This happens every time when I start KiCad and open the (previously open) schematic.

I didn't see this in my previous version from beginning of June (16e195).

Version:
========
Application: kicad
Version: (2017-09-02 revision 1fe91e625)-master, release build
Libraries:
    wxWidgets 3.1.1
    libcurl/7.54.0 SecureTransport zlib/1.2.8
Platform: macOS Sierra Version 10.12.6 (Build 16G29), 64 bit, Little endian, wxMac
Build Info:
    wxWidgets: 3.1.1 (UTF-8,STL containers)
    Boost: 1.59.0
    Curl: 7.55.1
    Compiler: Clang 8.1.0 with C++ ABI 1002

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

Note, that the same behaviour can be reproduced with official nightly of same version.

[Bernhard Stegmaier (stegmaier)]

Another note:
If you open schematic after start of KiCad and get incomplete symbols like, e.g., in bug.png, then you can close/reopen eeschema as often as you want. The symbols will always stay that way (being incomplete) and won't change.
The only thing which seem to change situation is changing projects.

[Bernhard Stegmaier (stegmaier)]

Maybe related:
(1) Startup KiCad, open library editor on last project.
(2) Load a library which shows incomplete symbols
(3) Try to open a symbol which is not rendered correctly in component chooser
=> Reproducible crash of library editor.

Backtrace:
<<<
Crashed Thread: 0 Dispatch queue: com.apple.main-thread

Exception Type: EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: KERN_INVALID_ADDRESS at 0x0000000000000000
Exception Note: EXC_CORPSE_NOTIFY

Termination Signal: Segmentation fault: 11
Termination Reason: Namespace SIGNAL, Code 0xb
Terminating Process: exc handler [0]

VM Regions Near 0:
-->
    __TEXT 000000010687c000-0000000106950000 [ 848K] r-x/rwx SM=COW /Applications/KiCad-20170903-1fe91e625/kicad.app/Contents/MacOS/kicad

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0 _eeschema.kiface 0x00000001100944f4 LIB_PART::HasConversion() const + 100
1 _eeschema.kiface 0x00000001101968b4 LIB_EDIT_FRAME::LoadOneLibraryPartAux(LIB_ALIAS*, PART_LIB*) + 884
2 _eeschema.kiface 0x00000001101964aa LIB_EDIT_FRAME::LoadComponentFromCurrentLib(LIB_ALIAS*, int, int) + 42
3 _eeschema.kiface 0x0000000110197577 LIB_EDIT_FRAME::LoadOneLibraryPart(wxCommandEvent&) + 2135
4 libwx_baseu-3.1.1.0.0.dylib 0x000000010750a56f wxEventHashTable::HandleEvent(wxEvent&, wxEvtHandler*) + 239
5 libwx_baseu-3.1.1.0.0.dylib 0x000000010750bbb3 wxEvtHandler::ProcessEvent(wxEvent&) + 275
6 _eeschema.kiface 0x0000000110300a9a EDA_BASE_FRAME::ProcessEvent(wxEvent&) + 26
7 libwx_baseu-3.1.1.0.0.dylib 0x000000010750bca4 wxEvtHandler::ProcessEventLocally(wxEvent&) + 164
8 libwx_baseu-3.1.1.0.0.dylib 0x000000010750bb5c wxEvtHandler::ProcessEvent(wxEvent&) + 188
9 libwx_osx_cocoau_core-3.1.1.0.0.dylib 0x0000000106f4f020 wxWindowBase::TryAfter(wxEvent&) + 224
10 libwx_baseu-3.1.1.0.0.dylib 0x000000010750bbdc wxEvtHandler::ProcessEvent(wxEvent&) + 316
11 libwx_osx_cocoau_aui-3.1.1.0.0.dylib 0x0000000106a0c7e1 wxAuiToolBar::OnLeftUp(wxMouseEvent&) + 977
12 libwx_baseu-3.1.1.0.0.dylib 0x000000010750a56f wxEventHashTable::HandleEvent(wxEvent&, wxEvtHandler*) + 239
13 libwx_baseu-3.1.1.0.0.dylib 0x000000010750bc5a wxEvtHandler::ProcessEventLocally(wxEvent&) + 90
14 libwx_baseu-3.1.1.0.0.dylib 0x000000010750bb5c wxEvtHandler::ProcessEvent(wxEvent&) + 188
15 libwx_baseu-3.1.1.0.0.dylib 0x000000010750c06f wxEvtHandler::SafelyProcessEvent(wxEvent&) + 15
16 libwx_osx_cocoau_core-3.1.1.0.0.dylib 0x0000000106e25ad3 wxWidgetCocoaImpl::DoHandleMouseEvent(NSEvent*) + 83
17 libwx_osx_cocoau_core-3.1.1.0.0.dylib 0x0000000106e0ce84 -[NSWindow(wxNSWindowSupport) WX_filterSendEvent:] + 148
18 libwx_osx_cocoau_core-3.1.1.0.0.dylib 0x0000000106e0ced0 -[wxNSWindow sendEvent:] + 32
19 com.apple.AppKit 0x00007fffa4b11681 -[NSApplication(NSEvent) sendEvent:] + 1145
20 libwx_osx_cocoau_core-3.1.1.0.0.dylib 0x0000000106d36782 -[wxNSApplication sendEvent:] + 98
21 com.apple.AppKit 0x00007fffa438c427 -[NSApplication run] + 1002
22 libwx_osx_cocoau_core-3.1.1.0.0.dylib 0x0000000106e01f6e wxGUIEven...

[Bernhard Stegmaier (stegmaier)]

I can pretty easy reproduce the bug on both of my machines, almost after each startup.
Doing some more tests, I am pretty sure that the commit "Refactored LIB_PART data storage
" (https://git.launchpad.net/kicad/commit/?id=352919658da483142ec9dd51fd389084b2d0082a) caused it.

The version before doesn't show the bug after various tests, the version with the refactor shows it immediately.

[Wayne Stambaugh (stambaughw)]

Bernhard,

Could you please test the patch included in the following bug report to
see if that resolves the issue? I haven't see this issue on windows but
this patch looks valid to me.

https://bugs.launchpad.net/kicad/+bug/1714110

I asked Orson to take a look at it but he may be on holiday as I have
not heard back from him yet. If this resolves the issue, I will merge
it as soon as possible.

Thanks,

Wayne

On 9/6/2017 5:08 AM, Bernhard Stegmaier wrote:
> I can pretty easy reproduce the bug on both of my machines, almost after each startup.
> Doing some more tests, I am pretty sure that the commit "Refactored LIB_PART data storage
> " (https://git.launchpad.net/kicad/commit/?id=352919658da483142ec9dd51fd389084b2d0082a) caused it.
>
> The version before doesn't show the bug after various tests, the version
> with the refactor shows it immediately.
>

[Bernhard Stegmaier (stegmaier)]

Wayne, yes patch looks valid. Unfortunately it doesn't fix my problem.

I also think I found a problem in operator++ of these iterators in some special case. I mailed Orson about that but also didn't hear anything from him yet (was just yesterday...). But, it also looks like this special case doesn't happen ATM.

The best way to check for me right now is to simply browse through libraries with component chooser. I quickly find wrong symbols in the preview, which render correct with the other version.

[Maciej Suminski (orsonmmz)]

Is anyone else able to reproduce the bug? I tried the attached project, a few other ones and browsed a dozen of components and they are all displayed correctly..

[Bernhard Stegmaier (stegmaier)]

I didn't receive any further feedback from ML, but I don't know if anyone tried. I'll ask again specifically the Mac guys (maybe some weird different std::map behaviour?).

I attached my current Devices.lib which I usually use to test (but, other libs also show the problem).
As written above I usually load the library in library editor and then just step through the components in component chooser until I find one that doesn't show a preview.

To sum up my findings until now:
(1) It happens both with my own builds as well as with Adams nightly (so, I guess it is no build or dependency problem with my own builds).
(2) It happens on both of my machines (so, I guess it is no configuration problem, or I borked both the same way).
(3) It doesn't happen directly before the map refactor, but directly with/after the change.
(4) Interesting: On my MacBook usually the crystal symbol doesn't work. This is very consistent. On my desktop the crystal always works, but some relais symbol down the list doesn't work. The relais symbol doesn't work on my MacBook, too. In general, on my MacBook is see much more symbols not working than on my desktop. My desktop is faster than my MacBook...?
(5) The affected symbols don't draw, because the iterator over drawing items doesn't deliver anything. size() of the list wrapper delivers the correct amount of drawing items. I can access the drawing items via [] and at least the correct types are shown. However, internal data of the drawing items seems to be somehow corrupt, because if I change the draw code to use [] instead of iterators to access them for drawing I get a lot of "<Error>: Error: this application, or a library it uses, has passed an invalid numeric value (NaN, or not-a-number) to CoreGraphics API and this value is being ignored. Please fix this problem." and they also won't draw.

Because of (4) and (5) I am tempted to think of some timing/synchronization issue leading to some data corruption. Is there any multithreading involved in loading/changing library data that could corrupt things? I had a quick look, but didn't see anything obvious...
If it would be some logic problem, I would expect to see the same symbols not working on both of my machines...?

I'll also go through the refactor change, maybe some palce were the map is still treated as the old list?

[Maciej Suminski (orsonmmz)]

Bernhard,

I see you are willing to help debugging the code and I fully appreciate it, thank you. I am unable to reproduce and scrutinize the problem, but if I could - I would step through the faulty loop to see what causes the loop to instantly break. It suppose might be a problem with operator++, operator!= or end().

I read your e-mail about the problem with inconsistent end iterator used by operator++ and end(). I agree it has to be fixed and most likely I will apply the attached patch, but first I would like you to check if it solves the issue you experience.

The second patch solves another potential problem I have found during today code review. I guess it will not solve your problem, but it is worth a shot.

[Maciej Suminski (orsonmmz)]

I am not aware of any multithreaded code related to loading symbol libraries too.

I tried the attached Devices library, both with the current master and the patches attached to my last message - in both cases the symbols are displayed correctly using either library browser or component selector dialog.

I will try later with Windows, but I can already tell that fixing this bug is going to deliver a lot of joy on the way.

[Bernhard Stegmaier (stegmaier)]

Tried the 2 patches and you are unfortunately right, they don't fix my issue.
Apart from that I didn't see any new issues with them.

Yes, I guess it will be a lot of fun... have to setup a debugging environment first. Never tried to import KiCad to XCode, I guess I will give QTCreator a try...

[Bernhard Stegmaier (stegmaier)]

Fun already started.
Built a debug version and problem is gone... :(
Will try a "release with debug info" next.

[Maciej Suminski (orsonmmz)]

I tried release and debug builds on Linux and debug build on Windows 7 - so far everything works as expected.

[Bernhard Stegmaier (stegmaier)]

Could you maybe also try a clang build on Linux?

Seems if I had luck... the debug build doesn't show it, but the "release with debug symbols" build.
I'll see if this is good enough and I can find something with the debugger.

[Maciej Suminski (orsonmmz)]

I use exclusively clang on Linux, due to its excellent address sanitizer.

[Bernhard Stegmaier (stegmaier)]

Hmm... bad.

I added a check function to LIB_PART which compares size() of the list to numbers of items using iterators. I put this check into every method using drawings - either once if not modified or twice at the beginning and end if modified somehow.

It seems that drawing items of broken parts already get corrupt on loading/creating them (check fails when libraries are loaded).
More, it seems that for every broken part the first failed check is consistently the second one of AddDrawItem:
<<<
void LIB_PART::AddDrawItem( LIB_ITEM* aItem )
{
    wxASSERT( aItem != NULL );

    checkDrawings(__func__, __LINE__);
    drawings.push_back( aItem );
    checkDrawings(__func__, __LINE__);
}
>>>

push_back(...) is pretty boring and just calls push_back(...) of the respective map entry, no idea yet what can go wrong there.

Because always the same symbols get broken, I added some code to print state of drawingsMap of this symbol before/after drawings.push_back() (debugger didn't help, seems to have problems to show contents of the map). I just iterate over map entries and print size of the ptr_vector. As soon as I add this code, this symbol is OK (others still not).
If I don't iterate over the whole map before the push_back of this symbol, but just print the size of ptr_vector of this type entry, the symbol gets broken again.

To be honest... I am running out of ideas.

[Bernhard Stegmaier (stegmaier)]

I forgot... as written in #19 I print the state before/after the push_back.
It doesn't have any effect if I iterate over drawingsMap after the push_back().
I.e., symbols still get broken with that.
Symbols are only fine if I add ietrating over drawingsMap _before_ push_back().

[Maciej Suminski (orsonmmz)]

Would you post the patch containing all your debugging changes? Perhaps it will give me an idea.

[Bernhard Stegmaier (stegmaier)]

I can provide it, but I have to tidy up a bit before. It is a complete mess right now.

Problem seems to be with the end() iterator.
I did suspect some problems with ptr_vector, so I changed it to std::vector<std::unique_ptr<LIB_ITEM>>.
Didn't change anything, shows the same problem.
I didn't yet revert this, so don't be surprised if you see this instead of ptr_vector below.

I added this function to check:
<<<
bool LIB_PART::checkDrawings(const char *func, int line) const
{
    size_t nd = 0;
    for(auto it=drawings.begin(); it!=drawings.end(); ++it)
        nd++;

    bool valid = (nd == drawings.size());
    if (!valid) {
        printf(">>> Bad part %s: %lu vs %lu (%s, %d)\n", m_name.mb_str().data(), nd, drawings.size(), func, line);
        for (auto it=drawingsMap.cbegin(); it!=drawingsMap.cend(); ++it)
            printf(" drawingsMap[%d]: %lu\n", it->first, it->second.size());
        printf("<<<\n");
        auto itb = drawings.begin();
        printf(" first: %s\n", itb->GetTypeName().mb_str().data());
        auto ite = drawings.end();
        --ite;
        printf(" last: %s\n", ite->GetTypeName().mb_str().data());
        ++ite;
        for(auto it=itb; it!=ite; ++it)
            printf(" it->type: %s\n", it->GetTypeName().mb_str().data());
    }

    return valid;
}
>>>

Further, I changed LIB_ITEMS_LIST::end a bit (mainly show last item before end):
<<<
...
    else // iterates over all items
    {
        // find a not empty container, starting from the last type
        auto i = m_data.rbegin();

        while( i->second.empty() && i != m_data.rend() )
            ++i;

        if( i == m_data.rend() )
            --i;

        it.m_it = i->second.end();
        --(it.m_it);
        std::unique_ptr< LIB_ITEM >& last = *(it.m_it);
        printf("itend(): %s\n", last->GetTypeName().mb_str().data());
        ++(it.m_it);
        it.m_type = i->first;
    }
...
>>>

Finally, I added a simple operator-- to the iterator (so that I can see what is before end()):
<<<
LIB_ITEMS_LIST::ITERATOR& LIB_ITEMS_LIST::ITERATOR::operator--()
{
    --m_it;
    return *this;
}
>>>
and some debug printf to operator!=
<<<
...
    printf("it!=: %d vs %d\n", m_type, aOther.m_type);
    return aOther.m_it != m_it;
...
>>>

Attached is a screenshot of the debugger.
I managed to debug in XCode, which seems to work a bit better than QTCreator in showing variables - however I don't know if what is shown is more reliable than that what QTcreator doesn't show (due to release build). Breakpoint has been set in checkDrawings if it finds something invalid.

See right bottom screen for console output... interesting part starts at ">>> Bad part ..."
It found a bad part after AddDrawItem() in "CONN-2X04", iterator delivers 9 items vs 16 items by size(). drawingMap really contains 16 items.

Right after the ">>> ... <<<" part the type of the items where begin()/end() iterators point to are output. First is a circle, which looks good. "itend()" and "last" show type of item before end(), in end() itself and in checkItems(). Both are Field, which should be also OK (fields are always at the end due to constructor of LIB_PART).
So, all in all, ...

[Bernhard Stegmaier (stegmaier)]

I can provide it, but I have to tidy up a bit before. It is a complete mess right now.

Problem seems to be with the end() iterator.
I did suspect some problems with ptr_vector, so I changed it to std::vector<std::unique_ptr<LIB_ITEM>>.
Didn't change anything, shows the same problem.
I didn't yet revert this, so don't be surprised if you see this instead of ptr_vector below.

I added this function to check:
<<<
bool LIB_PART::checkDrawings(const char *func, int line) const
{
    size_t nd = 0;
    for(auto it=drawings.begin(); it!=drawings.end(); ++it)
        nd++;

    bool valid = (nd == drawings.size());
    if (!valid) {
        printf(">>> Bad part %s: %lu vs %lu (%s, %d)\n", m_name.mb_str().data(), nd, drawings.size(), func, line);
        for (auto it=drawingsMap.cbegin(); it!=drawingsMap.cend(); ++it)
            printf(" drawingsMap[%d]: %lu\n", it->first, it->second.size());
        printf("<<<\n");
        auto itb = drawings.begin();
        printf(" first: %s\n", itb->GetTypeName().mb_str().data());
        auto ite = drawings.end();
        --ite;
        printf(" last: %s\n", ite->GetTypeName().mb_str().data());
        ++ite;
        for(auto it=itb; it!=ite; ++it)
            printf(" it->type: %s\n", it->GetTypeName().mb_str().data());
    }

    return valid;
}
>>>

Further, I changed LIB_ITEMS_LIST::end a bit (mainly show last item before end):
<<<
...
    else // iterates over all items
    {
        // find a not empty container, starting from the last type
        auto i = m_data.rbegin();

        while( i->second.empty() && i != m_data.rend() )
            ++i;

        if( i == m_data.rend() )
            --i;

        it.m_it = i->second.end();
        --(it.m_it);
        std::unique_ptr< LIB_ITEM >& last = *(it.m_it);
        printf("itend(): %s\n", last->GetTypeName().mb_str().data());
        ++(it.m_it);
        it.m_type = i->first;
    }
...
>>>

Finally, I added a simple operator-- to the iterator (so that I can see what is before end()):
<<<
LIB_ITEMS_LIST::ITERATOR& LIB_ITEMS_LIST::ITERATOR::operator--()
{
    --m_it;
    return *this;
}
>>>
and some debug printf to operator!=
<<<
...
    printf("it!=: %d vs %d\n", m_type, aOther.m_type);
    return aOther.m_it != m_it;
...
>>>

Attached is a screenshot of the debugger.
I managed to debug in XCode, which seems to work a bit better than QTCreator in showing variables - however I don't know if what is shown is more reliable than that what QTcreator doesn't show (due to release build). Breakpoint has been set in checkDrawings if it finds something invalid.

See right bottom screen for console output... interesting part starts at ">>> Bad part ..."
It found a bad part after AddDrawItem() in "CONN-2X04", iterator delivers 9 items vs 16 items by size(). drawingMap really contains 16 items.

Right after the ">>> ... <<<" part the type of the items where begin()/end() iterators point to are output. First is a circle, which looks good. "itend()" and "last" show type of item before end(), in end() itself and in checkItems(). Both are Field, which should be also OK (fields are always at the end due to constructor of LIB_PART).
So, all in all, ...

[Maciej Suminski (orsonmmz)]

There is no need to provide the full patch, you have already posted the relevant pieces, thank you!

Because there are at least a few items available through LIB_ITEMS_LIST::ITERATOR, the symbol graphic should be visible, perhaps incomplete. Is it the first iteration in checkDrawings() {for(auto it=drawings.begin(); it!=drawings.end(); ++it) nd++;} that cures a symbol, so there are at least a few items return via LIB_ITEMS_LIST::ITERATOR?

Is there any pattern that all broken symbols follow? E.g. do they lack any particular item type?

About the end() iterator pointing to a LIB_PIN: IIRC the end iterators never point to a valid element, so I would not interpret the pointed address.

I would love to know what goes wrong here, just for satisfying my curiosity, but if we cannot invent anything better, then I will try to reimplement LIB_ITEMS_LIST::ITERATOR using the type and index number (i.e. to point to N-th item of a specific type) rather than STL iterators.

[Bernhard Stegmaier (stegmaier)]

<<<
Because there are at least a few items available through LIB_ITEMS_LIST::ITERATOR, the symbol graphic should be visible, perhaps incomplete. Is it the first iteration in checkDrawings() {for(auto it=drawings.begin(); it!=drawings.end(); ++it) nd++;} that cures a symbol, so there are at least a few items return via LIB_ITEMS_LIST::ITERATOR?
>>>
Yes, symbols range from being more or less incomplete (a part of them is shown) to not being shown at all.
Depends on when iterator/loop decides to stop.
It can also be at the very beginning, so it doesn't even enter the loop.

<<<
Is there any pattern that all broken symbols follow? E.g. do they lack any particular item type?
>>>
No, doesn't seem so.
But, I have cleaned up the debugging output a lot, so I can dump a little bit better only broken stuff.
I'll try to provide it this evening, maybe you can see more than me.

<<<
About the end() iterator pointing to a LIB_PIN: IIRC the end iterators never point to a valid element, so I would not interpret the pointed address.
>>>
Yes, you are right, didn't think about that.
Really nasty to debug those iterators, especially end()...

<<<
I would love to know what goes wrong here, just for satisfying my curiosity, but if we cannot invent anything better, then I will try to reimplement LIB_ITEMS_LIST::ITERATOR using the type and index number (i.e. to point to N-th item of a specific type) rather than STL iterators.
>>>
Me too... :)

Before you implement it using indexes let me try again. I did switch the drawing method from the for loop using iterators to using indexes. I did get the not-iterated drawing items, but got a lot of graphic errors with those (see post #10, (5)) so instances seemed to be corrupted. But, I don't know if I already had applied the patch about indexes that Wayne sent me, so maybe it just picked the wrong ones.

I also thought of dumping the map and just use something fixed size like a vector or even an array (number of items is fixed and continuous anyway, you just need to apply an offset). Since insertion of a single element seems to corrupt things, maybe some map internal reorganization (triggered by insertion) goes wrong in conjunction with ptr_vector? Google found some stuff about slicing that can happen with ptr_vector (especially with std::reverse iterators?), but I don't know if it applies.

On the other hand, if slicing would be a problem, then it shouldn't have happened when I switched over to std::vector<std::unique_ptr<LIB_ITEM>> instead of the prt_vector...? And why would it only happen for me?

[Maciej Suminski (orsonmmz)]

I have almost managed to build a working executable with OSX Sierra, so
soon I should be able to join the debugging force.

In the meantime, in the attached patch you will find a more proper
iterator implementation that uses separate types for standard and const
iterators. I doubt it will help, but given the difficulty of the
problem, anything goes.

I think using a vector in place of map might be good idea, I will give
it a try. I think it should be also faster for look up operations.

On 09/14/2017 09:46 PM, Bernhard Stegmaier wrote:
> Hi,
>
> I don’t know if this is the problem, but maybe I found something:
> It could be an optimisation issue, because it doesn’t occur with a real debug build.
>
> I started to implement a single container instead of the map and wrapper.
> I like the idea of the overall iterator, so I copied signature of the iterator stuff from your class into my new one.
> It didn’t compile because of the begin()/end() functions being marked as const.
> Well, I guess this is technically correct, because I would get a non-const iterator with which I could modify elements in the container… so maybe the optimiser does weird things because of this (wrong?) const?
> I also don’t know why it didn’t complain with your original implementation… it should have the same problem?
>
> I tried to remove the const for a quick check, but it breaks LIB_PART in a few const methods… so, I didn’t try further.
> This was just an idea what could happen, no idea if it is a problem in reality.
>
> I implemented a const and a non-const iterator now, but there are still some weird places where I would need [] for the overall list.
> I’ll have to see if I can change that - despite the comments about not using iterators in these places, which I yet have to understand...
>
> Just as some info on what’s going on…

[Bernhard Stegmaier (stegmaier)]

I am pretty much done with my container (it internally uses a simple array instead of the map, but could also be changed to a vector).
Until now I don’t see the problem with it any more.

I still have to change 2 spots with [] access from LIB_PART, but I think I can provide a first working patch this evening (latest Monday, I will be out of town over weekend).
If you are short on time, you can just wait to see if my changes are fine for you.

Regards,
Bernhard

> On 15. Sep 2017, at 10:57, Maciej Suminski <email address hidden> wrote:
>
> I have almost managed to build a working executable with OSX Sierra, so
> soon I should be able to join the debugging force.
>
> In the meantime, in the attached patch you will find a more proper
> iterator implementation that uses separate types for standard and const
> iterators. I doubt it will help, but given the difficulty of the
> problem, anything goes.
>
> I think using a vector in place of map might be good idea, I will give
> it a try. I think it should be also faster for look up operations.
>
> On 09/14/2017 09:46 PM, Bernhard Stegmaier wrote:
>> Hi,
>>
>> I don’t know if this is the problem, but maybe I found something:
>> It could be an optimisation issue, because it doesn’t occur with a real debug build.
>>
>> I started to implement a single container instead of the map and wrapper.
>> I like the idea of the overall iterator, so I copied signature of the iterator stuff from your class into my new one.
>> It didn’t compile because of the begin()/end() functions being marked as const.
>> Well, I guess this is technically correct, because I would get a non-const iterator with which I could modify elements in the container… so maybe the optimiser does weird things because of this (wrong?) const?
>> I also don’t know why it didn’t complain with your original implementation… it should have the same problem?
>>
>> I tried to remove the const for a quick check, but it breaks LIB_PART in a few const methods… so, I didn’t try further.
>> This was just an idea what could happen, no idea if it is a problem in reality.
>>
>> I implemented a const and a non-const iterator now, but there are still some weird places where I would need [] for the overall list.
>> I’ll have to see if I can change that - despite the comments about not using iterators in these places, which I yet have to understand...
>>
>> Just as some info on what’s going on…
> <0001-Split-LIB_ITEMS_LIST-ITERATOR-to-standard-and-const-.patch>

[Maciej Suminski (orsonmmz)]

This is great news! Please send me the patch before you leave, no matter
whether complete or not - I will finish it, if necessary.

On 09/15/2017 11:07 AM, Bernhard Stegmaier wrote:
> I am pretty much done with my container (it internally uses a simple array instead of the map, but could also be changed to a vector).
> Until now I don’t see the problem with it any more.
>
> I still have to change 2 spots with [] access from LIB_PART, but I think I can provide a first working patch this evening (latest Monday, I will be out of town over weekend).
> If you are short on time, you can just wait to see if my changes are fine for you.
>
>
> Regards,
> Bernhard

[Maciej Suminski (orsonmmz)]

Bernhard, I have succeeded building KiCad with OS X Sierra, and.. it works here.
http://www.imgpaste.net/image/eJbgN

I do not see much differences between our systems, could it be there is a problem with old boost version?

Application: eeschema
Version: (2017-09-14 revision f299266)-master, release build
Libraries:
    wxWidgets 3.0.3
    libcurl/7.54.0 SecureTransport zlib/1.2.8
Platform: Mac OS X (Darwin 16.7.0 x86_64), 64 bit, Little endian, wxMac
Build Info:
    wxWidgets: 3.0.3 (UTF-8,STL containers,compatible with 2.8)
    Boost: 1.65.1
    Curl: 7.51.0
    Compiler: Clang 8.1.0 with C++ ABI 1002

Build settings:
    USE_WX_GRAPHICS_CONTEXT=ON
    USE_WX_OVERLAY=ON
    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_SPICE=OFF

[Maciej Suminski (orsonmmz)]

Um, a few questions - do I need to use the launcher app to observe the bug? Does everything work correctly in eeschema standalone? Also, does the bug occur only for schematic files opened at least twice?

[Bernhard Stegmaier (stegmaier)]

For boost… I am still using a 1.59.
The nightly with which I could also reproduce did have some early 1.6x, but as far as I remember definitely older than your 1.65.1

Just checked, it also happens for me with eeschema standalone.
I just opened eeschema, no schematics.
I opened library editor from eeschema and there one of my libraries of my default configuration.
Then browsed through components and found some which were not drawn correctly.

I guess meanwhile it doesn’t have anything to with what was loaded before or if loaded twice.
It is just loading the libraries which causes the problem.

I am done with my changes so far and everything looks good.
Some small coding-style things to fix.
I’ll post it this evening.

> On 15. Sep 2017, at 14:04, Maciej Suminski <email address hidden> wrote:
>
> Um, a few questions - do I need to use the launcher app to observe the
> bug? Does everything work correctly in eeschema standalone? Also, does
> the bug occur only for schematic files opened at least twice?
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1714974
>
> Title:
> Symbols not drawn correctly after KiCad start
>
> Status in KiCad:
> New
>
> Bug description:
> How to reproduce:
> (1) Open project attached to bug
> (2) Close KiCad
> (3) Start KiCad (project manager)
> (4) Open schematic
>
> Some symbols are drawn not at all or incomplete - see attached bug.png, pins of transistor are missing and complete connectors are missing.
> If I open another project and then switch back to this project the symbols are drawn correctly - or at least more symbols are drawn correctly. See attached bug2.png, transistor and 3-pin connector is now fine, but 4-pin connector still missing.
>
> This happens every time when I start KiCad and open the (previously
> open) schematic.
>
> I didn't see this in my previous version from beginning of June
> (16e195).
>
>
> Version:
> ========
> Application: kicad
> Version: (2017-09-02 revision 1fe91e625)-master, release build
> Libraries:
> wxWidgets 3.1.1
> libcurl/7.54.0 SecureTransport zlib/1.2.8
> Platform: macOS Sierra Version 10.12.6 (Build 16G29), 64 bit, Little endian, wxMac
> Build Info:
> wxWidgets: 3.1.1 (UTF-8,STL containers)
> Boost: 1.59.0
> Curl: 7.55.1
> Compiler: Clang 8.1.0 with C++ ABI 1002
>
> Build settings:
> USE_WX_GRAPHICS_CONTEXT=ON
> USE_WX_OVERLAY=ON
> KICAD_SCRIPTING=OFF
> KICAD_SCRIPTING_MODULES=OFF
> KICAD_SCRIPTING_WXPYTHON=OFF
> KICAD_SCRIPTING_ACTION_MENU=OFF
> BUILD_GITHUB_PLUGIN=ON
> KICAD_USE_OCE=ON
> KICAD_SPICE=OFF
>
> Note, that the same behaviour can be reproduced with official nightly
> of same version.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/kicad/+bug/1714974/+subscriptions

[Bernhard Stegmaier (stegmaier)]

Attached the patch to refactor LIB_ITEMS_LIST and LIB_ITEMS_MAP into a single container. It basically is still the same approach, but uses a simple array for type-based access instead of a map.
From a design point of view I think that having one container is a more clean approach than having two members for the same data.

With that, I cannot reproduce my problem... for whatever mystical reason or whatever the root-cause was.

The changes in LIB_PART are quite straightforward. For the review, please have a closer look at those three methods:
LIB_PART::GetNextDrawItem(): I changed it to use iterators and to make use of the type-based access if appropriate (instead of always iterating over all items as it was before). Should have better performance than before.
LIB_PART::CopySelectedItems(): I changed it to use iterators. I think the comment is not valid anymore, because of using the tmp vector instead of directly adding items inside the loop. I changed the original comment and think it is fine to use iterators this way (it is also done this way in other places).
LIB_PART::SetUnitCount(): Same as before. Comment is valid, I added the tmp vector and then used iterators.

My first tests show that everything is working fine with the patch. But, for sure I didn't test throughly. I will do some more testing over the weekend.

[Maciej Suminski (orsonmmz)]

Bernard,

I read the patch and I think your changes are fine. I could not find any issues when running the code under Linux, neither clang address sanitizer has indicated any problems, so I assume it is quite safe.

I created a branch [1] that contains your patch, some code formatting and another commit that transforms LIB_ITEMS_CONTAINER to a template. Later I will try it out on Windows, but I do not expect any errors there. If you confirm it solves the problem for OS X, then I will commit the patches and we can close the report.

1. https://code.launchpad.net/~orsonmmz/kicad/+git/kicad/+ref/lib_item_container_fix

[Maciej Suminski (orsonmmz)]

I tested the branch with Windows, seems correct.

[Bernhard Stegmaier (stegmaier)]

Sorry for the delay.
Tested your branch and also works fine for me without any problems.

[KiCad Janitor (kicad-janitor)]

Fixed in revision 0be357ec3e88d4467d59ac93a8e84628c14067dc
https://git.launchpad.net/kicad/patch/?id=0be357ec3e88d4467d59ac93a8e84628c14067dc