Re: [hugin-ptx] Re: A hack?
Hi Thomas,
On September 26, 2010 11:58:11 am T. Modes wrote:
> Yes, I'm sure, your changeset is the culprit.
> Changeset 22828bb9df09 works ok, but changeset d14b0775fbfa crashes.
You may be sure but I am not so sure - I had this kind of symptoms before
applying this changeset. They appeared a few days ago.
> > > 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.
How do you know better than me what I have considered? Please respect that
there can be (and often are) different ways of considering the same critics
and that the result of me considering the feedback can be (and in this case
is) different than yours.
IMO it is enough consideration to activate the feature only on deliberate user
request / preference.
I consider mature something that works as intended. IMHO it does not have to
protect the user from tripping over once he knowingly activated it.
For example, if a user does not know how to drive a car, he should take
driving lessons, not ask for cars to be dumbed down.
> If you can't fix it, than leave it in the current state
done. changeset reverted.
hg backout --merge 4416
> > > 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: "
line 291 of a 632 lines (that's roughly 20 pages) dry and boring html page
that is itself hidden as a link in a long list of cryptic names on the third
or fourth hierarchy level of a documentation that states on its own homepage
[0] that it is an incomplete work in progress and that probably encompasses
hundreds of pages?
we need to do better than this if we want occasional contributors to actually
adopt the conventions.
I never read that documentation. I barely recalled it existed and probably if
I would start by reading it my enthusiasm for hacking on the code would die
instantly.
The way I operate is that once I have an itch I want to scratch I navigate the
code with
find . -exec grep -l "whateverIlookfor" {} \;
Searching for a unique enough GUI term leads me to the XRC file. Searching
for the related term in the XRC file leads me to the parts in the code where
it is used and where it is implemented. I read and try to understand the few
related functions. I do m changes by example, looking at similar things in
neighboring code. I just hack. My changes are small hacks. I am not a
coder. I don't have the necessary skill and time. I learn as I go and become
comfortable at navigating the source code, but I also tend to forget when I am
not in the code regulary, and unfortunately this happens often.
I need the executive summary of what can / can't should / should not be done
with the code - something that does not take more than 15 minutes to read and
understand.
> There are more points, which you don't consider, otherwise we would
> not have this discussion.
Please enlighten me. This is why I usually seek feedback for my contributed
patches, and I am sorry I did not do it this time.
> The list, which images are cached, should stored inside the image
> cache and not in the panorama.
where do I find the image cache?
> > > 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.
can you please make me aware of the trouble it produces for you?
undo/redo as it was until a few days ago produced trouble for me and I solved
that tr
[hugin-ptx] Re: A hack?
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 hugin-ptx@googlegroups.com To unsu
Re: [hugin-ptx] Re: A hack?
Hi Thomas, On September 26, 2010 04:54:50 am T. Modes wrote: > 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] > 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. 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). > 2.) You did not test it again on other systems after the massive > changes to the code (since the last year). Sorry for that. This is point b above. I should have been more patient and restrain my urge to get rid of cruft on my HDD. Testing against *current* codenase and not *past* codebase was needed. > 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. > 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? > 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). 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. > 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. > PS: It seems that you have preferencitis ;-) If you add for each > feature an option on the preference and mark it as experimental, it > will become more confusing and at the end we will have only > experimentals. The "experimental" label is just to prevent consumer whining. We could probably do away with all the "experimental" markings. I strongly believe it is a user's right to adapt and customize the software to his needs. Call it preferencitis if you like. I prefer (pun intended) preferencitis over the dictatorcitis that reigns souvereign in much of the proprietary world and, unfortunately, some of the Open Source world. The most powerful expression of my belief is a full source tree of the whole system+apps and a set of local patches against it. While source-based distros such as Gentoo and FreeBSD enable that, few users have the skills and time to put their hands into every aspect of their system. So they patch only what is critically important to them. Making such patches configurable build-time options is a slightly better option: it reduces redundancies (when many users maintain a set of similar local patches) and makes it easier to maintain such customization "socially". In this specific case, others can pick up my less than perfect code and improve on it; or I will revisit it when I have time. Few users build their systems from source and build-time options are less accessible than preferences. Hence I favor adding preferences to give access to variety and customization to the largest possible user base at the least possible cumulated effort. Looking at it from a another perspective: everybody has the right to change their software. That's locally. When making changes to the public repository, one has to tak
Re: [hugin-ptx] Re: A hack?
Am Sonntag 26 September 2010 schrieb T. Modes: > Hi Yuv, > > > Almost a year ago I proposed a small change to how Hugin handles the > > caching of images [0]. It's a simple and dumb workaround and indeed in > > the ensuing discussion improvements such as the using of wxThreads were > > discussed. > > > > Moreover, the integration of layout mode was of higher priority, so I > > just dumped the patch in the tracker [1] after applying it to my > > production version of Hugin and I forgot about it. > > > > It came to my mind again when after a long time I started doing again > > more than just the occasional stitch. > > > > Even if it is imperfect, I committed it to the default branch. It is > > deactivated by default. A preferences toggle activates it. It does not > > interfere with any other existing behavior/functionality, so if you don't > > like it, ignore it and it will ignore you. > > Even deactivated it crashes on Windows. I can not load projects, add > images -> crash. > It crashes here too, every time I try to save pto. A shortened stacktrace: src/hugin1/hugin/MainFrame.cpp:596 src/hugin_base/algorithms/panorama_makefile/PanoramaMakefilelibExport.cpp:450 src/hugin_base/algorithms/panorama_makefile/PanoramaMakefilelibExport.cpp:1025 src/hugin_base/makefilelib/Variable.cpp:63 src/hugin_base/makefilelib/Makefile.cpp:64 Kornel -- Kornel Benko kornel.be...@berlin.de signature.asc Description: This is a digitally signed message part.
[hugin-ptx] Re: A hack?
Hi Yuv, > Almost a year ago I proposed a small change to how Hugin handles the caching > of images [0]. It's a simple and dumb workaround and indeed in the ensuing > discussion improvements such as the using of wxThreads were discussed. > > Moreover, the integration of layout mode was of higher priority, so I just > dumped the patch in the tracker [1] after applying it to my production version > of Hugin and I forgot about it. > > It came to my mind again when after a long time I started doing again more > than just the occasional stitch. > > Even if it is imperfect, I committed it to the default branch. It is > deactivated by default. A preferences toggle activates it. It does not > interfere with any other existing behavior/functionality, so if you don't like > it, ignore it and it will ignore you. > Even deactivated it crashes on Windows. I can not load projects, add images -> crash. I'm in favour to revert this changes. Let me explain why: 1.) First you did not consider the points in the other thread which were mentioned for bigger projects. 2.) You did not test it again on other systems after the massive changes to the code (since the last year). 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. 4.) You are again modifing the panorama object outside the CommandHistory. If you undo, this will interfere with the your OnIdle code. 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. 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. Thomas PS: It seems that you have preferencitis ;-) If you add for each feature an option on the preference and mark it as experimental, it will become more confusing and at the end we will have only experimentals. -- 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 hugin-ptx@googlegroups.com To unsubscribe from this group, send email to hugin-ptx+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/hugin-ptx
[hugin-ptx] Re: A hack?
Yuval Levy wrote: > Almost a year ago I proposed a small change to how Hugin handles the caching > of images [0]. It's a simple and dumb workaround and indeed in the ensuing > discussion improvements such as the using of wxThreads were discussed. [...] This does not help for the previews on Images, Crop and Mask tabs, does it? The latter two seem to be uncached. cu andreas -- 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 hugin-ptx@googlegroups.com To unsubscribe from this group, send email to hugin-ptx+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/hugin-ptx
