kicad-developers team mailing list archive

[Wayne Stambaugh <stambaughw@xxxxxxxxx>]

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
>