| Thread Previous • Date Previous • Date Next • Thread Next |
It was in this thread; I've attached them both again for convenience. The "0001-Remove-BOARD-SetBoundingBox.patch" is applied after applying the other patch. On Wed, Mar 1, 2017 at 8:59 AM, Wayne Stambaugh <stambaughw@xxxxxxxxx> wrote: > 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 <stambaughw@xxxxxxxxx > > <mailto:stambaughw@xxxxxxxxx>> 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 <jon@xxxxxxxxxxxxx > > <mailto:jon@xxxxxxxxxxxxx>> 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 > > <stambaughw@xxxxxxxxx <mailto:stambaughw@xxxxxxxxx>> > > >>> 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 < > > >>>> stambaughw@xxxxxxxxx <mailto:stambaughw@xxxxxxxxx> > > >>>>>>> <mailto: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> <mailto: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> > > >>>>>>> <https://launchpad.net/~kicad-developers > > <https://launchpad.net/~kicad-developers>> > > >>>>>>> >>>> Post to : kicad-developers@xxxxxxxxxxxxxxxxxxx > > <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx> > > >>>>>>> <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx > > <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>> > > >>>>>>> >>>> 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@xxxxxxxxxxxxxxxxxxx > > <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx> > > >>>>>>> <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx > > <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>> > > >>>>>>> >>> 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@xxxxxxxxxxxxxxxxxxx > > <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx> > > >>>>>>> <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx > > <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>> > > >>>>>>> > 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@xxxxxxxxxxxxxxxxxxx > > <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx> > > >>>>>>> <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx > > <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>> > > >>>>>>> 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@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 > > <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> > > > > >
Attachment:
0001-Move-ZoomFitScreen-and-ZoomPreset-from-PCBNEW_CONTRO.patch
Description: Binary data
Attachment:
0001-Remove-BOARD-SetBoundingBox.patch
Description: Binary data
| Thread Previous • Date Previous • Date Next • Thread Next |
