Well, here is the problem that smart pointers won’t resolve. If I draw
an atom, then draw a bonded atom off of that the bond has two options,
remember the pointer to the second atom drawn, or remember the index
numbers for each atom it is connected to. In the case of remembering
pointers, on an undo, the atom and bond get deleted. on redo when I
recreate the bond based on a pointer to the atom, that pointer is
invalid and I can never find the atom I am supposed to bond with.
On the other hand, if I use indexes with auto-add hydrogen this always
screws up the induces. For example: Draw two connected, then draw a
third connected to the second. the third atom you’ve drawn will be 6.
Then draw a bond between the third and the first, your third atom
becomes index 3.
Now the main problem I see with a unique index is aren’t we going to run
out of numbers sooner or later? Especially if auto-add hydrogens is
creating and deleting a bunch of hydrogens over and over again. The max
that any QList can hold (bad design by TT) is the max int which on my
systems is 2147483647. Although, a std::vector of pointers can hold up
to 2305843009213693951 which is longer than I expect any instance of
Avogadro will run.
Another idea I was bouncing around was if we have the draw tool hang on
to the atom it is using and not actually let Molecule delete, but rather
let it remove that atom from the molecule. I don’t know that OB
supports this. Anyways, the idea is that when an atom drawing gets
undone, it just hangs on to that atom, then when it gets redone it just
re-adds it rather than creating a new one, then the bond would be able
to hang on to it’s pointers. However, this would be problematic when we
start doing molecule duplications. Like “RemoveBond” does a complete
backup of the molecule and so if you drew two bonded molecules, then
deleted the bond, then undid that the pointers that your AddAtom/AddBond
commands had would not be valid to perform an undo because by undoing
the RemoveBond command you are replacing the Molecule with new atoms and
bonds… old pointers are invalid.
I apologies for my kinda rampant emails. I have not had a lot of time
to better think through a solution or how to explain the problem.
Hopefully some feedback can be given. I am working on just implementing
unique IDs in Avogadro.
I do agree that we should try to use more “smart pointers” in our code
at least where we have extensions/tools/engines using pointers.
I will be trying to resolve more crashes in the upcoming week. Is there
a better tool for debugging than GDB? It is so goddamn slow for startup
especially having to load all the plugins I just get tired of having to
restart after a crash too.
–
Donald
(Tue, May 06, 2008 at 03:43:47PM -0400) Geoffrey Hutchison geoff.hutchison@gmail.com:
On May 6, 2008, at 6:13 AM, Donald Ephraim Curtis wrote:
I gotta call timeout for a big suggestion and/or change.
Suggestions and/or changes are welcome. However, I don’t think it’s a
good idea for Open Babel to make such a large change in the timeframe
of 2.2 (which is intended to release by the end of the May). As is,
I’m worried we haven’t provided our usual level of testing for 2.2.0.
Certainly Open Babel will add this at some future time, but I would
say that Avogadro shouldn’t count on it ASAP.
Thus, I have to suggest that we either disable undo/redo for the next
release OR we add a layer of identifiers on top of OpenBabel. This is
what I propose:
There is, also, no reason that Avogadro can’t use smart pointers
internally.
http://doc.trolltech.com/4.4/qpointer.html
shared_ptr in GCC or Boost
That said, I don’t think there’s any reason Avogadro can’t include
permanent object IDs like you mention. This would likely remove many
undo/redo bugs.
OTOH, while there are bugs, I don’t come across them on a day-to-day
basis. I think we do have to be pragmatic sometimes. Is the current
SVN trunk better than the last release? Do we have regressions (i.e.,
new bugs that didn’t exist in 0.6.x?)?
These are questions we should answer first IMHO.
Cheers,
-Geoff