Hi Miles, I wasn’t around for the first discussion on the topic, but one of the reasons (1) doesn’t work terribly well is that it’s not enough to know what changed; one also has to know how to group the changes into individual undo/redo steps. It sounds like the plugin infrastructure accomplishes this by the over-simplification that a plugin does discrete operations (ie: one plugin call == one undo step).
Why does the plugin mechanism need to do a diff? Is that to apply the changes, or just to know what to commit? If it’s the latter, just stop doing that. Memory is cheap; commit everything. So what if undo replaces a bunch of stuff that didn’t actually change. Cheers, Jeff. > On 6 Mar 2018, at 09:02, miles mccoo <m...@mmccoo.com> wrote: > > Thanks all for your replies. > > I like the plugin mechanism. It does some nice things for python folks. > Refresh, undo, garbage collection (I think). I want to see it succeed. > > From reading Orson's mail, I think I wasn't entirely clear, so let me try to > fix that first. > > > The plugin mechanism tries to track the changes of a python plugin. In the > c++ world, developers are expected to inform BOARD_COMMIT that something has > changed. The plugin infrastructure does this for you. One could compare it to > garbage collection. My interpretation of how this is done is simple: before a > plugin runs, make a list of all design objects. zones, tracks, modules, > nets... after the plugin completes, do a "diff". > > This is mostly fine with the exception of removed items. > > The problem with removed items is how do you do a diff on a deleted, memory > managed, cleaned up object? Perhaps that memory now contains a different kind > of object. Perhaps it's been reallocated to a similar object as before. > > > > as it stands now, any plugin that removes items from a board item container > will likely fail. It's not quite a crash, but it has a similar feel to the > user. > > > > Solutions. I can think of four. > > solution 1. Why is the plugin mechanism in the business of tracking changes? > Shouldn't BOARD_COMMIT just magically happen whenever a c++ instance is > modified. I brought this up in a previous thread[1] and various reasons were > given why this isn't desirable. > > solution 2. Same as #1 except BC magic only happens on python API calls. The > plugin mechanism would no longer do diffs. Just add BC checkpoints. This is > probably a lot of work. I might be willing to do this work but it would take > time. [2] > > solution 3. easy to implement. turn off deletion on calls to remove if > currently running a plugin > plugin gets a boolean: isPluginActive. set/unset around the call to actual > python run code. > add pcbnew.isPluginActive() to python api. (I could imagine this could have > other uses) > the remove code looks at isPluginActive to decide whether to set isown. (that > code is actually python, not swig) > I have #3 implemented and my plugins no longer crash. See attached patch for > if folks agree it is an acceptable stopgap. > > solution 4. probably not the direction I would go but a way to enable python > memory management and do the plugin diff thing. Basically, give each object > some sort of unique id. (could be useful for other things). In addition to > holding a list of object pointers, plugin infrastructure would hold a list of > object ids. > > > Given the ease with which a plugin can hit this issue and given how long it > would take to get #2 right, I guess I'm recommending the changes of that > attached patch for #3. > > Miles > > [1] https://lists.launchpad.net/kicad-developers/msg32063.html > <https://lists.launchpad.net/kicad-developers/msg32063.html> > [2] I think my personal opinion is that #1 is superior over #2. Python people > shouldn't have to care but why is it that c++ should or want to? Yes, I read > the arguments from the previous thread but I'm not convinced. I'm also just a > kicad spectator, so there's a lot I don't know. > > > > > > On Fri, Mar 2, 2018 at 2:47 PM, Wayne Stambaugh <stambau...@gmail.com > <mailto:stambau...@gmail.com>> wrote: > LOL, I just replied to Miles. Thanks Orson for helping out! > > On 3/2/2018 8:36 AM, Maciej Sumiński wrote: > > Hi Miles, > > > > I suppose the silence in the thread indicates there are not many > > developers knowing the Python scripting interface inside out. Since you > > are both studying the scripting interface and developing own scripts, it > > is quite possible you are the most competent person to give us an advice > > on how to proceed. See some comments below, but I am neither a Python > > script developer nor a scripting interface maintainer, so I might be > > lacking some basic knowledge here. > > > > On 02/28/2018 05:12 PM, miles mccoo wrote: > >> So I'm plugin-ifying my python scripts (the mechanism is awesome). One of > >> the plugins deletes some stuff and that is causing trouble. > >> > >> > >> > >> I'm not sure how to fix the root cause. Hence this mail. > >> > >> > >> > >> The plugin just deletes Edge.Cuts[1]: > >> for d in board.GetDrawings(): > >> if (d.GetLayerName() == 'Edge.Cuts'): > >> board.Remove(d) > >> > >> > >> > >> in board_item_container.i, I see this (with stuff deleted): > >> %rename(RemoveNative) BOARD_ITEM_CONTAINER::Remove; > >> def Remove(self,item): > >> self.RemoveNative(item) > >> item.thisown=1 > >> > >> > >> Setting thisown tells, python "you own it". Delete it when you're done. > >> Which it does. > >> > >> > >> The problem this causes is that the plugin mechanism saves a list of all > >> objects before running the plugin and then checks if any of them has a null > >> list after (ie is it still in the design). > > > > Is this mechanism implemented to handle memory leaks? If so, would not > > it be sufficient to stick to 'thisown' flag on Remove() calls or is > > there another way objects might be destroyed using Python scripts? > > > >> Since the object has been deleted by python, the plugin stuff gets > >> confused. > >> > >> > >> *So, the question is how to fix this?* > >> > >> > >> It appears that the plugin infrastructure will delete for you (that's what > >> I'm guessing), so the thisown setting shouldn't be done. > >> > >> > >> On the other hand, if running code from within a standalone script (ie from > >> regular python shell), now thisown'ing it would yield a memory leak. > >> > >> > >> > >> Perhaps the plugin stuff should have some sort of flag indicating "you're > >> in a plugin". Then the thisown setting could be conditional. > > > > If the object listing mechanism is required for other reasons, then I > > suppose it is second best idea. Generally speaking, I would like to make > > the scripting interface convenient for the users, so they do not need to > > worry about whether their scripts are run standalone or as a plugin. > > Let's hide the dirty magic from them and make the coding process enjoyable. > > > > Regards, > > Orson > > > >> But I'm just a spectator. *I'm happy to put in the time to fix this but > >> need guidance on what approach to take.* > >> > >> > >> Miles > >> > >> > >> > >> [1] full plugin text > >> import pcbnew > >> > >> class RemoveBoundaryPlugin(pcbnew.ActionPlugin): > >> def defaults(self): > >> self.name <http://self.name/> = "Remove boundary" > >> self.category = "A descriptive category name" > >> self.description = "This plugin reads a dxf file and converts it to > >> a zone" > >> > >> def Run(self): > >> board = pcbnew.GetBoard() > >> > >> for d in board.GetDrawings(): > >> print("{}".format(str(d))) > >> #print("on layer {} {} {}".format(d.GetLayerName(), > >> # str(d.GetStart()), > >> # str(d.GetEnd()))) > >> if (d.GetLayerName() == 'Edge.Cuts'): > >> board.Remove(d) > >> > >> RemoveBoundaryPlugin().register() > >> > >> > >> > >> _______________________________________________ > >> 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> > > <0001-Fix-for-crash-due-to-pcbnew_action_plugin-object-tra.patch>_______________________________________________ > 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
_______________________________________________ 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
