kicad-developers team mailing list archive

[Maciej Sumiński <maciej.suminski@xxxxxxx>]

Apparently clang analyzer has been correct this time. It turns out that
PCB_PAINTER::Draw() and PCB_RENDER_SETTINGS::GetColor() assume that all
objects are EDA_ITEMs, which is not true for VIEW_GROUP class. When a
layer is changed, VIEW::UpdateAllLayersColor() iterates through all
VIEW_ITEMs and eventually calls PCB_RENDER_SETTINGS::GetColor() which
incorrectly casts VIEW_GROUP objects to EDA_ITEM.

The attached patch uses dynamic_cast to verify the downcast results. I
measured performance drop at order of 1% in a debug build, so it does
not seem like a huge loss. I think we have not seen any problems as
RTree::Query() had never returned VIEW_GROUP objects as they had too
large bounding box.

I would rather keep Jon's patch, as it makes sense to simply iterate
through a simple array of items during full world updates, rather than
using RTree algorithms to extract the same list.

Cheers,
Orson

On 02/26/2018 04:12 PM, Wayne Stambaugh wrote:
> Orson,
> 
> That's what I was doing so it sounds like it might be something with
> your setup.
> 
> Wayne
> 
> On 2/26/2018 9:30 AM, Maciej Sumiński wrote:
>> Wayne,
>>
>> I meant selecting any layer in the right-hand layer widget. Perhaps it
>> is a false positive in clang analyzer, I will have a look at it. Pcbnew
>> does not crash here when built with gcc (and no analyzer enabled).
>>
>> Cheers,
>> Orson
>>
>> On 02/26/2018 03:02 PM, Wayne Stambaugh wrote:
>>> Orson,
>>>
>>> What do you mean by "any layer switching"?  I am not have any issues on
>>> windows when selecting a different layer in the layer manager using
>>> either canvas when launched from kicad or stand alone mode.
>>>
>>> Cheers,
>>>
>>> Wayne
>>>
>>> On 2/26/2018 5:01 AM, Maciej Sumiński wrote:
>>>> Jon,
>>>>
>>>> With this patch applied, any layer switching in pcbnew leads to a crash,
>>>> even with no layout loaded. Can anyone else confirm this? See the the
>>>> attached address sanitizer report for details.
>>>>
>>>> Regards,
>>>> Orson
>>>>
>>>> On 02/25/2018 10:01 PM, Jon Evans wrote:
>>>>> This patch uses simple iteration instead of the RTREE search to update the
>>>>> layer order and color in VIEW.  On my Linux system, this results in a ~50%
>>>>> speedup in release build (less in debug build) and is most noticeable when
>>>>> switching layers in GerbView with large Gerber files loaded (i.e. high
>>>>> number of items in the view).
>>>>>
>>>>> -Jon
>>>>>
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> Mailing list: https://launchpad.net/~kicad-developers
>>>>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>>>>> Unsubscribe : https://launchpad.net/~kicad-developers
>>>>> More help   : https://help.launchpad.net/ListHelp
>>>>>
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> Mailing list: https://launchpad.net/~kicad-developers
>>>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>>>> Unsubscribe : https://launchpad.net/~kicad-developers
>>>> More help   : https://help.launchpad.net/ListHelp
>>>>
>>>
>>> _______________________________________________
>>> Mailing list: https://launchpad.net/~kicad-developers
>>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>>> Unsubscribe : https://launchpad.net/~kicad-developers
>>> More help   : https://help.launchpad.net/ListHelp
>>>
>>
>>
>>
>>
>> _______________________________________________
>> Mailing list: https://launchpad.net/~kicad-developers
>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>> Unsubscribe : https://launchpad.net/~kicad-developers
>> More help   : https://help.launchpad.net/ListHelp
>>
> 
> _______________________________________________
> Mailing list: https://launchpad.net/~kicad-developers
> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp
> 

From 5a970815c2463410ebcb1bd46b391252157c1e5f Mon Sep 17 00:00:00 2001
From: Maciej Suminski <maciej.suminski@xxxxxxx>
Date: Mon, 26 Feb 2018 16:51:18 +0100
Subject: [PATCH] PCB_PAINTER: use dynamic_cast to determine whether an object
 is of EDA_ITEM type

---
 pcbnew/pcb_painter.cpp | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/pcbnew/pcb_painter.cpp b/pcbnew/pcb_painter.cpp
index 907ecbe24..52f9c5428 100644
--- a/pcbnew/pcb_painter.cpp
+++ b/pcbnew/pcb_painter.cpp
@@ -212,7 +212,7 @@ void PCB_RENDER_SETTINGS::LoadDisplayOptions( const PCB_DISPLAY_OPTIONS* aOption
 const COLOR4D& PCB_RENDER_SETTINGS::GetColor( const VIEW_ITEM* aItem, int aLayer ) const
 {
     int netCode = -1;
-    const EDA_ITEM* item = static_cast<const EDA_ITEM*>( aItem );
+    const EDA_ITEM* item = dynamic_cast<const EDA_ITEM*>( aItem );
 
     if( item )
     {
@@ -277,7 +277,10 @@ int PCB_PAINTER::getLineThickness( int aActualThickness ) const
 
 bool PCB_PAINTER::Draw( const VIEW_ITEM* aItem, int aLayer )
 {
-    const EDA_ITEM* item = static_cast<const EDA_ITEM*>( aItem );
+    const EDA_ITEM* item = dynamic_cast<const EDA_ITEM*>( aItem );
+
+    if( !item )
+        return false;
 
     // the "cast" applied in here clarifies which overloaded draw() is called
     switch( item->Type() )
-- 
2.13.3

Attachment: signature.asc
Description: OpenPGP digital signature