kicad-developers team mailing list archive

[Wayne Stambaugh <stambaughw@xxxxxxxxx>]

Every BOARD_ITEM has a parent/child relationship or at least it did if
someone did not break it.  You can always find the parent BOARD object
by walking up the BOARD_ITEM parent list.  There is already a
BOARD_ITEM::GetBoard() call that should return the parent board.  You
could use that to notify the BOARD object when a child BOARD_ITEM change
effects the bounding box.

On 2/24/2017 9:27 AM, Jon Evans wrote:
> 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
> <mailto: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 <mailto: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
>     <https://launchpad.net/~kicad-developers>
>     >>>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>     <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
>     >>>> Unsubscribe : https://launchpad.net/~kicad-developers
>     <https://launchpad.net/~kicad-developers>
>     >>>> More help   : https://help.launchpad.net/ListHelp
>     <https://help.launchpad.net/ListHelp>
>     >>>>
>     >>>
>     >>>
>     >>>
>     >>> _______________________________________________
>     >>> Mailing list: https://launchpad.net/~kicad-developers
>     <https://launchpad.net/~kicad-developers>
>     >>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>     <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
>     >>> Unsubscribe : https://launchpad.net/~kicad-developers
>     <https://launchpad.net/~kicad-developers>
>     >>> More help   : https://help.launchpad.net/ListHelp
>     <https://help.launchpad.net/ListHelp>
>     >>>
>     >>>
>     >>
>     >
>     >
>     >
>     > _______________________________________________
>     > Mailing list: https://launchpad.net/~kicad-developers
>     <https://launchpad.net/~kicad-developers>
>     > Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>     <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
>     > Unsubscribe : https://launchpad.net/~kicad-developers
>     <https://launchpad.net/~kicad-developers>
>     > More help   : https://help.launchpad.net/ListHelp
>     <https://help.launchpad.net/ListHelp>
>     >
> 
> 
>     _______________________________________________
>     Mailing list: https://launchpad.net/~kicad-developers
>     <https://launchpad.net/~kicad-developers>
>     Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>     <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
>     Unsubscribe : https://launchpad.net/~kicad-developers
>     <https://launchpad.net/~kicad-developers>
>     More help   : https://help.launchpad.net/ListHelp
>     <https://help.launchpad.net/ListHelp>
> 
>