Hi Yuv, > > Even deactivated it crashes on Windows. I can not load projects, add > > images -> crash. > > sorry for that. Are you sure this change is the culprit? There have been > crash reports from right after the merge of the two GSoC2010 branches [0]
Yes, I'm sure, your changeset is the culprit. Changeset 22828bb9df09 works ok, but changeset d14b0775fbfa crashes. > > > I'm in favour to revert this changes. Let me explain why: > > I am working on a fix right now, please excuse me if I am slow, > I am building on a 1.6GHz Atom. > > While I agree with you that the changes are half-baked and need to be > reverted, I do not agree with you on all points: > > > 1.) First you did not consider the points in the other thread which > > were mentioned for bigger projects. > > Yes I did. This is why the user can enable/disable the behavior via > preferences. Other kind of considerations would have taken more time than I > had to spare. You did not consider the critics. The changeset is the same as your patch from last year. You added only a preference, but did not consider the critics points. Your code can go into an endless loop (as James already mentioned http://groups.google.com/group/hugin-ptx/msg/6752a969d852a97a ). Only when somebody can deactivate it by select an option, which would otherwise make it unusable, this a would *not* consider mature. Such "hidden" option gives hugin the reputation of an expert only program. If you can't fix it, than leave it in the current state, which works, instead of adding a half baked solution, which will cause more problems and trouble, as already mentioned last year. If it works for you, you can leave it in your personal repository, but don't push it the the public one. > In terms of maturity criteria [1] > a. Since it worked for my bigger projects, it was good enough for me > b. It had been tested back then (my mistake not to have it retested today) > c. it does not unintentionally break existing functionality (read: the > behavior is off by default and users have to deliberately activate it). Sorry, but this not correct. After compiling the changeset d14b0775fbfa it crashes when loading a project or add a images - and your option is *off*, so it breaks existing functionality. > > 3.) You are using ImageOptions. This structure is deprecated since > > merging of the layout mode and should not used any more. Exisiting > > parts, which still use it, needs to be change. > > Is this documented somewhere? If you want to deprecate structures, and > motivate contributors to use some structures and not others, we need an easy > to find documentation of this. It is in the documentation: look at SrcPanoImage::getOptions(), the function you are using: http://hugin.sourceforge.net/docs/html/classHuginBase_1_1SrcPanoImage.html#a19 There stands: "Do not use: " > > 4.) You are again modifing the panorama object outside the > > CommandHistory. If you undo, this will interfere with the your OnIdle > > code. > > Have not considered that. I start to question the CommandHistory structure > all together. What is the purpose of undo/redo? Have we done enough > abstraction, or are we recording irrelevant details in there? There are more points, which you don't consider, otherwise we would not have this discussion. The list, which images are cached, should stored inside the image cache and not in the panorama. > > 5.) Even if the user deactivates the option, it will produce disc load > > (on unix) / registry load (on windows), because you are reading and > > reading again the config. > > How can you tell if your build crashes? I would expect wxConfigBase [2] to > take care of the detail / cache the config in memory. My expectation is > confirmed by what I observe here (no disk access when idling). > But not on Windows, the OnIdle event does access the registry the whole time (your option off, and before the crash). > I could get around this by adding a boolean variable to > MainFrame.cpp, read only once, and test on it, but then enabling/disabling the > behavior will require a new start unless I also access that variable from the > PreferencesDialog.cpp. This will solve this issue, but there are more general problems with your patch. > > I think, this problem should be solved in a more general approach, as > > already discussed in the other thread. Otherwise it creates more > > problems/confusion. > > If somebody has the time and skill to solve it in a more general approach, go > for it. In the meantime, like one year ago, this patch makes *my* life easier > and I wanted to share it with others. > Nice, that is makes *your* life easier, but it produces more trouble for me. Thomas -- You received this message because you are subscribed to the Google Groups "Hugin and other free panoramic software" group. A list of frequently asked questions is available at: http://wiki.panotools.org/Hugin_FAQ To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected] For more options, visit this group at http://groups.google.com/group/hugin-ptx
