kicad-developers team mailing list archive

[Jon Evans <jon@xxxxxxxxxxxxx>]

We need a solution that allows GetBoundingBox to be called blindly, without
knowing if (or how) an EDA_ITEM subclass needs to have the bounding box
updated.

Does anyone have ideas about how complicated it would be to give the BOARD
class knowledge of whether or not anything inside it has changed?  This is
what I tried to implement at first, but Orson pointed out that the way I
did it would miss some kinds of changes.

Figuring out the above also seems useful for other reasons -- for example,
autosave / backup features are easier to implement if there is a global
"dirty/clean" flag for things like BOARD.

If there is no good way to implement caching, I guess another way would be
to implement caching at the sites that use the BOARD bounding box heavily
(autorouter etc)

-Jon

On Fri, Feb 24, 2017 at 9:22 AM, Wayne Stambaugh <stambaughw@xxxxxxxxx>
wrote:

> On 2/24/2017 4:16 AM, Maciej Sumiński wrote:
> > Hi Jon,
> >
> > The current version looks much better to me. From what I see there is no
> > actual bounding box caching, as GetBoundingBox() always calls
> > ComputeBoundingBox(). I am fine with that, but before I push the patch I
> > need to ask: does anyone know why the board bounding box is cached? I
> > believe it must have been done to boost performance of certain parts of
> > the code, but I wonder if it is really necessary. Especially that one
> > needs to know that it has to be updated using ComputeBoundingBox().
>
> It was cached so it didn't need to be recalculated every time the
> bounding box was required.  It should or at least it did recalculate
> only when the changes were made since the last call to fetch the
> bounding box.  On very complex boards, this can take a while.  Getting
> the bounding box is called fairly often so this can add up.  I don't see
> any reason to recalculate the bounding box on every get bounding box
> call.  If the current bounding box behavior is broken, then it should be
> fixed.  I don't see any reason to get rid of the caching method.  If you
> are operating outside of the normal methods for adding and/or editing
> board objects that prevents the correct bounding box from being
> calculated, that is your fault and you need to fix your code accordingly.
>
> >
> > Just by looking at the code, I noticed that the autorouter calls
> > BOARD::GetBoundingBox() frequently, but I could not see much difference
> > with caching disabled. Likely, the routing algorithm takes significantly
> > more time than the bounding box calculations.
> >
> > Personally I would completely remove m_BoundingBox field instead of
> > making it mutable. Also, I have noticed there are a few places where the
> > bounding box is overridden with SetBoundingBox(). It seems to me a bit
> > dubious, as bounding box should depend solely on the items belonging to
> > the board. I attach a patch to show what I would change. Any comments,
> > especially from Wayne & Jean-Pierre?
> >
> > Regards,
> > Orson
> >
> > On 02/23/2017 01:49 PM, Jon Evans wrote:
> >> Hi Orson,
> >>
> >> Here's an updated patch with the changes you requested.  The only issue
> is,
> >> without some kind of caching, I had to change the call sites that were
> >> interested in the board bounding box with edges only, so the patch has
> >> grown in scope.  Let me know if this looks better.
> >>
> >> Best,
> >> Jon
> >>
> >> On Thu, Feb 23, 2017 at 4:17 AM, Maciej Sumiński <
> maciej.suminski@xxxxxxx>
> >> wrote:
> >>
> >>> Hi Jon,
> >>>
> >>> I really like the generic approach in the zoom methods. This part I
> >>> would merge instantly, but there is an issue with caching the board
> >>> bounding box. It does not take into account that items already added to
> >>> board may change their position and affect the bounding box. I would
> >>> remove caching completely, what do you think?
> >>>
> >>> If you are going to modify the patch, would you rename boardBBox in
> >>> COMMON_TOOLS::ZoomFitScreen() to bbox or anything that is not related
> to
> >>> board? Thank you in advance.
> >>>
> >>> Regards,
> >>> Orson
> >>>
> >>> On 02/23/2017 05:34 AM, Jon Evans wrote:
> >>>> Hi all,
> >>>>
> >>>> This patch finishes off the refactor I did a few days ago, by enabling
> >>>> ZoomFitScreen to work without knowledge of the BOARD class.
> >>>>
> >>>> In order to make this work, I changed the way GetBoundingBox and
> >>>> ComputeBoundingBox work so that the call sites of GetBoundingBox don't
> >>> have
> >>>> to also call ComputeBoundingBox if they don't need to use the
> >>>> aBoardEdgesOnly flag.
> >>>>
> >>>> Those with good knowledge of BOARD should review this to make sure I
> >>> caught
> >>>> all the instances where we should mark the bounding box as needing to
> be
> >>>> re-computed.
> >>>>
> >>>> Best,
> >>>> 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
>