kicad-developers team mailing list archive

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

Most of the operations one can do on a BOARD_ITEM potentially affects
the board bounding box. It means there might be many places where the
notifications should be added.

Some time ago, Michael Steinberg has put in place OBSERVABLE class
(include/observable.h) that could facilitate the task.

With the current approach, developers have to guess whether to update
the bounding box with ComputeBoundingBox() or take the cached value.
Update is called in very few places, so I really wonder what kind of
magic makes it work, but I might have missed something here.

Regards,
Orson

On 02/24/2017 04:06 PM, Wayne Stambaugh wrote:
> 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>
>>
>>
> 
> 
> _______________________________________________
> 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
>

Attachment: signature.asc
Description: OpenPGP digital signature