On 3/1/2017 8:54 AM, Jon Evans wrote: > Wayne, Orson's latest patch is basically Option 2 but implemented > another way. Can you check it out and let us know if you are OK merging > it as-is, or if we should change the implementation to be more like what > I described?
I don't remember seeing this patch. Was it posted to the dev list? > > Thanks, > Jon > > On Wed, Mar 1, 2017 at 8:43 AM, Wayne Stambaugh <stambau...@gmail.com > <mailto:stambau...@gmail.com>> wrote: > > Option 2 seems fine. It may be easier to just add an option argument to > the existing GetBoundingBox() function to either return the cached > bounding box or recalculate the bounding box on demand rather than > create a different function for the cached vs. uncached version. > > On 3/1/2017 2:51 AM, Maciej Sumiński wrote: > > Wayne, what do you think? If it is acceptable, then I would like to > > merge the patches. I should not judge my code, as I might be > tempted to > > consider it a good solution. > > > > In case my proposal to cache the bounding box in autorouter code > is not > > approved, then I would say #2 would do the job. > > > > Cheers, > > Orson > > > > On 03/01/2017 01:23 AM, Jon Evans wrote: > >> Update: after some more testing, I think Orson's patch on top of > mine is > >> the way to go. It pulls the computation out of the loops for > autorouter, > >> and that's the only place where the board's bounding box is used > >> repetitively. Since the other call sites are only going to get the > >> bounding box intermittently, I don't think it's worth any effort > doing > >> complicated things to track and recompute the bounding box when > things are > >> changed. > >> > >> So, I think my patch and Orson's should be merged. > >> > >> -Jon > >> > >> On Tue, Feb 28, 2017 at 6:51 PM, Jon Evans <j...@craftyjon.com > <mailto:j...@craftyjon.com>> wrote: > >> > >>> I'm finally back to this problem now that I have some other WIP > committed. > >>> > >>> As far as I can tell, there two paths to go down for this problem: > >>> > >>> 1) Implement auto-caching of the bounding box. This requires some > >>> mechanism to tell BOARD that it needs to update the cache, be it > OBSERVABLE > >>> or some other update function added to BOARD_ITEM. No matter > how it's > >>> done, this method seems not great to me because it requires > touching a ton > >>> of code to provide coverage for every possible entry point that > could end > >>> up having an effect on the bounding box. > >>> > >>> 2) Have two different calls to get the bounding box for an > EDA_ITEM, one > >>> that always re-generates the cache and one that just returns a > cached > >>> value. Update the call sites that hit the bounding box frequently > >>> (autorouter, etc) to make use of the cached value; keep the call > sites that > >>> always re-generate the bounding box in today's code working that > way. > >>> Performance shouldn't be any better or worse than it is now. > >>> > >>> 3) Track whether a board is clean/dirty in some other way > besides looking > >>> at BOARD_ITEMs. This might not be practical while we still > support legacy, > >>> but once there is only GAL, I could see this maybe being > possible through > >>> the tool framework. For example, actions could be marked as > possibly > >>> impacting the board or not (i.e. move item does, zoom does not) > >>> > >>> So, unless someone suggests otherwise, I'm going to try > implementing #2, > >>> and suggest #3 as a future improvement that would improve > performance for > >>> some edge case operations (i.e. "zoom to fit screen" would be > instantaneous > >>> if nothing has modified the board since the last time the bbox > was cached, > >>> even on large boards) > >>> > >>> -Jon > >>> > >>> On Fri, Feb 24, 2017 at 11:01 AM, Wayne Stambaugh > <stambau...@gmail.com <mailto:stambau...@gmail.com>> > >>> wrote: > >>> > >>>> On 2/24/2017 10:30 AM, Maciej Sumiński wrote: > >>>>> 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. > >>>> > >>>> It could be useful but I'm not sure it would be a better design > than the > >>>> child object informing it's parent that it's bounding box has > changed. > >>>> I'm not sure the additional code would make the code any more > readable. > >>>> The only benefit would be if other observers were required. > >>>> > >>>>> > >>>>> 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. > >>>> > >>>> This solution would require limiting the entry points in the BOARD > >>>> object that allow changes to the BOARD's child items. > Conceptually this > >>>> is probably easier to understand but requires developers to be > diligent > >>>> when making changes to the BOARD object or not trying to bypass > these > >>>> entry points. > >>>> > >>>>> > >>>>> 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 < > >>>> stambau...@gmail.com <mailto:stambau...@gmail.com> > >>>>>>> <mailto:stambau...@gmail.com <mailto:stambau...@gmail.com>>> > 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.sumin...@cern.ch > <mailto:maciej.sumin...@cern.ch> <mailto:maciej.sumin...@cern.ch > <mailto:maciej.sumin...@cern.ch>>> > >>>>>>> >> 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> > >>>>>>> <https://launchpad.net/~kicad-developers > <https://launchpad.net/~kicad-developers>> > >>>>>>> >>>> Post to : kicad-developers@lists.launchpad.net > <mailto:kicad-developers@lists.launchpad.net> > >>>>>>> <mailto:kicad-developers@lists.launchpad.net > <mailto:kicad-developers@lists.launchpad.net>> > >>>>>>> >>>> Unsubscribe : > https://launchpad.net/~kicad-developers > <https://launchpad.net/~kicad-developers> > >>>>>>> <https://launchpad.net/~kicad-developers > <https://launchpad.net/~kicad-developers>> > >>>>>>> >>>> More help : https://help.launchpad.net/ListHelp > <https://help.launchpad.net/ListHelp> > >>>>>>> <https://help.launchpad.net/ListHelp > <https://help.launchpad.net/ListHelp>> > >>>>>>> >>>> > >>>>>>> >>> > >>>>>>> >>> > >>>>>>> >>> > >>>>>>> >>> _______________________________________________ > >>>>>>> >>> Mailing list: > https://launchpad.net/~kicad-developers > <https://launchpad.net/~kicad-developers> > >>>>>>> <https://launchpad.net/~kicad-developers > <https://launchpad.net/~kicad-developers>> > >>>>>>> >>> Post to : kicad-developers@lists.launchpad.net > <mailto:kicad-developers@lists.launchpad.net> > >>>>>>> <mailto:kicad-developers@lists.launchpad.net > <mailto:kicad-developers@lists.launchpad.net>> > >>>>>>> >>> Unsubscribe : > https://launchpad.net/~kicad-developers > <https://launchpad.net/~kicad-developers> > >>>>>>> <https://launchpad.net/~kicad-developers > <https://launchpad.net/~kicad-developers>> > >>>>>>> >>> More help : https://help.launchpad.net/ListHelp > <https://help.launchpad.net/ListHelp> > >>>>>>> <https://help.launchpad.net/ListHelp > <https://help.launchpad.net/ListHelp>> > >>>>>>> >>> > >>>>>>> >>> > >>>>>>> >> > >>>>>>> > > >>>>>>> > > >>>>>>> > > >>>>>>> > _______________________________________________ > >>>>>>> > Mailing list: https://launchpad.net/~kicad-developers > <https://launchpad.net/~kicad-developers> > >>>>>>> <https://launchpad.net/~kicad-developers > <https://launchpad.net/~kicad-developers>> > >>>>>>> > Post to : kicad-developers@lists.launchpad.net > <mailto:kicad-developers@lists.launchpad.net> > >>>>>>> <mailto:kicad-developers@lists.launchpad.net > <mailto:kicad-developers@lists.launchpad.net>> > >>>>>>> > Unsubscribe : https://launchpad.net/~kicad-developers > <https://launchpad.net/~kicad-developers> > >>>>>>> <https://launchpad.net/~kicad-developers > <https://launchpad.net/~kicad-developers>> > >>>>>>> > More help : https://help.launchpad.net/ListHelp > <https://help.launchpad.net/ListHelp> > >>>>>>> <https://help.launchpad.net/ListHelp > <https://help.launchpad.net/ListHelp>> > >>>>>>> > > >>>>>>> > >>>>>>> > >>>>>>> _______________________________________________ > >>>>>>> Mailing list: https://launchpad.net/~kicad-developers > <https://launchpad.net/~kicad-developers> > >>>>>>> <https://launchpad.net/~kicad-developers > <https://launchpad.net/~kicad-developers>> > >>>>>>> Post to : kicad-developers@lists.launchpad.net > <mailto:kicad-developers@lists.launchpad.net> > >>>>>>> <mailto:kicad-developers@lists.launchpad.net > <mailto:kicad-developers@lists.launchpad.net>> > >>>>>>> Unsubscribe : https://launchpad.net/~kicad-developers > <https://launchpad.net/~kicad-developers> > >>>>>>> <https://launchpad.net/~kicad-developers > <https://launchpad.net/~kicad-developers>> > >>>>>>> More help : https://help.launchpad.net/ListHelp > <https://help.launchpad.net/ListHelp> > >>>>>>> <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@lists.launchpad.net > <mailto:kicad-developers@lists.launchpad.net> > >>>>>> 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@lists.launchpad.net > <mailto:kicad-developers@lists.launchpad.net> > >>>>> 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@lists.launchpad.net > <mailto:kicad-developers@lists.launchpad.net> > >>>> 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@lists.launchpad.net > <mailto:kicad-developers@lists.launchpad.net> > >> 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@lists.launchpad.net > <mailto:kicad-developers@lists.launchpad.net> > > 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@lists.launchpad.net > <mailto:kicad-developers@lists.launchpad.net> > 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@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
