← Back to team overview

kicad-developers team mailing list archive

Re: [PATCH] Don't use the RTREE in UpdateAllLayersOrder() / UpdateAllLayersColor()

 

Sounds good to me.  1% hit should not cancel out the gains from not using
the RTree in large worlds (I saw layer-switch time drop from ~80ms to ~30ms
on my machine)
We can consider revisiting this later to eliminate the need for
dynamic_cast by ensuring that anything that VIEW calls on an item it owns
is a member of that object.

-Jon

On Mon, Feb 26, 2018 at 11:03 AM, Maciej Sumiński <maciej.suminski@xxxxxxx>
wrote:

> 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
> >
>
>
> _______________________________________________
> 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
>
>

Follow ups

References