I forgot to CC the list... On Tue, Mar 18, 2008 at 4:32 PM, Alan W. Irwin <[EMAIL PROTECTED]> wrote: > Thanks Hez. > > I am not competent enough with driver/core C code to comment on the > specifics of your patch, but I would like to make some general comments.
Thanks for the feedback Alan. I am learning more and more about the PLplot internals as I go along, whether I want to or not :-) The changes I have submitted so far are important for my plotting needs, and I hope that they will be useful to others as well once they are complete. > (1) sys/win32/msdev/src/win3.cpp is no longer maintained or used so you > don't have to worry about it. To give you the historical background, it is > part of svn for now so that people can study that historical windows driver > code, but we no longer use it. In fact all of sys/win32 is excluded from > our release tarballs since the new CMake build system (which works well on > Windows) supersedes that old hand-crafted Windows-only build system, and > there are a number of devices that work on Windows now, including the > windows-only drivers/wingcc.c device driver which supersedes > sys/win32/msdev/src/win3.cpp. Sounds good - I will ignore that portion of the source tree for the purposes of this patch. > (2) It's a fact (and not criticism) that your patch hits a substantial > fraction of the areas of PLplot (e.g., drivers/xwin.c, src/plimage.c, > examples/python/qplplot.py) which currently do not have much core developer > support for a variety of reasons. Developers have lost some interest in > drivers/xwin.c because the fonts currently look bad, and nobody has cared > enough to fix that so far. src/plimage.c was donated by core developer Joao > Cardoso who has not been heard from for several years now. > examples/python/qplplot.py was donated by an external developer who did not > maintain it afterwards. So I think we all appreciate your interest in > plimage.c and want to encourage it. > > That said, I think you should not remove the dev_fastimg rendering path > unless we find that rendering path really does not provide much of a speed > increase. The reason I am concerned is Joao Cardoso introduced that > rendering path (IIRC) because he was not satisfied with the speed of the > example 20 results for our premier device at that time (early 2000's), > xwin.c. Now, computers are much more powerful so speed is not as important > an issue, but nevertheless, fast results are probably something we should > not give up lightly and which we should positively encourage for the new > devices which in other respects (such as font handling) are superseding > xwin.c. Currently, you have stated above that that the dev_fastimg > rendering path does not work well with the custom coordinate transform > support added to plimagefr. I encourage you to fix that issue (assuming > dev_fastimg rendering really is faster for xwin.c). I realize that is > probably a lot of work, but it does make your patch much less obtrusive and > prepares necessary infrastructure to propagate the dev_fastimg rendering to > other devices. dev_fastimg is definitely faster than the slow rendering path for the xwin driver. I tested example 20 with both 5.9.0 and plplot-svn+my patch, and (using xwin) 5.9.0 is certainly less CPU intensive. That said, from what I understand reading the xwin.c code, dev_fastimg assumes that the image is made up of homogeneously sized unrotated rectangular boxes. So while I think it could be adapted for non-transformed images, it would not work for images with most coordinate transforms. The patch I submitted does all of the drawing with plfill, which makes plimage much simpler and more flexible, and unfortunately noticeably slower. For the time being, would you accept a patch with the dev_fastimg rendering path code left in place, but unused? I would comment out the code in a few places, and leave it untouched but unused in others. The changes required to keep it in use for cases where it will be of use will take me a while as I will have to get to know the PLplot internals better. My initial move to use dev_fastimg would be for non-transformed images only. So calls to plimage would use the dev_fastimg path when available, but calling plimagefr directly would only use dev_fastimg if the pltr callback is NULL. Otherwise plfill would still be used for each transformed pixel. To sum up, I would like to submit patches in the follow steps: (1) Add coordinate transform to plimagefr and disable the dev_fastimg rendering path, but without removing the dev_fastimg code. (2) Update dev_fastimg to work with the updated plimagefr, but only use it for non-transformed images. (3) Update example 20 with some examples of what plimagefr can do, with pages to illustrate both fixed color ranges and coordinate transformations. Does this sound like a reasonable compromise? Taking this on over a few steps would be much easier for me since it breaks the work up in to smaller chunks, which in turn makes tracking plplot-svn simpler. Step (3) could come before step (2), and perhaps should for testing and verification purposes to make sure dev_fastimg works as expected. > (3) I suggest you add a page or more to x20c.c to show an example (or > examples) of how to use the new features you have added for plimagefr. The > reason I suggest that change is new PLplot API that is not illustrated with > one of the examples tends to go unused and untested by our developers and > users. Of course, keep the old x20c.c pages as well which show plimage > examples. I will certainly add one or more pages to example 20, though I would again prefer this as a separate step once at least the initial patch is in PLplot. I have some very simple test code that I use now, but it is mostly in OCaml to make sure I am keeping the interfaces in sync. > Thanks again for your interest in improving PLplot. > > Alan I am very happy to be able to be able to provide improvements where I am able. PLplot has been a pleasure to work with, particularly how open you (the core devs) have been to working with submitted patches and improvements. Hez -- Hezekiah M. Carty Graduate Research Assistant University of Maryland Department of Atmospheric and Oceanic Science ------------------------------------------------------------------------- This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008. http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/ _______________________________________________ Plplot-devel mailing list Plplot-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/plplot-devel
