On Mon, Mar 31, 2008 at 3:52 PM, Hezekiah M. Carty <[EMAIL PROTECTED]> wrote:
>
> On Wed, Mar 19, 2008 at 2:31 PM, Andrew Ross
> <[EMAIL PROTECTED]> wrote:
> >
> > On Wed, Mar 19, 2008 at 01:12:24PM -0500, Maurice LeBrun wrote:
> > > On Wednesday, March 19, 2008 at 10:14:30 (-0700) Alan W. Irwin writes:
> > > > On 2008-03-19 10:59-0400 Hezekiah M. Carty wrote:
> > > >
> > > > > 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?
> > > >
> > > > Hi Hez:
> > > >
> > > > Actually after sleeping on it, I am leaning toward saying do (1)
> (with code
> > > > commentary where you do the disabling in plimage.c, xwin.c, etc.,
> about why
> > > > it was necessary) and leave (2) as a would-be-nice rather than a
> requirement
> > > > since it sounds like it might be a lot of work which you could more
> > > > productively spend on the OCaml bindings, for example.
> > > >
> > > > However, I don't feel right making this decision alone because I
> haven't
> > > > used -dev xwin or the plimage capability for my own PLplot needs, and
> > > > somebody who has more of a vested interest in those parts of PLplot
> may feel
> > > > a lot stronger about their speed than I do. Thus, I am going to need
> > > > advice/help from the other PLplot core developers on the decision
> about (1)
> > > > and (2) so please step forward, guys, and comment.
> > >
> > > I use -dev xwin extensively (and its client, plframe) but not plimage.
> That
> > > said, doing (1) and leaving/documenting (2) as a nice-to-have sounds
> fine to
> > > me, unless someone can make a case otherwise.
> >
> > Ditto.
>
> Thank you for all of the feedback, and sorry for the delayed response.
> As I am sure is the case for many of you, using PLplot for research
> got in the way of developing PLplot...
>
> The attached patch does the following:
> - Disables the dev_fastimg rendering path. This is done in a
> (hopefully) minimally invasive manner. No dev_fastimg code is
> deleted, just commented out when needed with a comment explaining why
> and that this is intended to be temporary.
> - Adds coordinate transformation (pltr callback) support to plimagefr
> - Removes the Dxmin, Dxmax, Dymin, Dymax arguments from plimagefr as
> they do not make sense when using a coordinate transform. The
> associated code/logic is moved in to plimage to keep its interface and
> function consistent with how it has worked in the past.
> - Should fix the (occasional) off-by-one bug in the number of image
> pixel rows/columns rendered when selecting a sub-image in plimage.
> - Updates api.xml to reflect the changes to plimagefr.
>
> Example 20 updates are not in place yet, but I will add them once we
> get this patch sorted out.
>
> This version of the patch works well for me.
>
> Some questions:
> - Should the name stay as plimagefr? I do not have any better ideas
> at this time, but with the addition of the coordinate transform
> support someone may have other naming suggestions.
> - What is the proper way to restore the initial drawing color when
> these functions end? Currently a call to plimage or plimagefr will
> leave the color set to whichever color palette 1 color was used for
> the last pixel. Ideally the color at entry would be saved at the
> start of the function and set again at the end of the function. I see
> examples elsewhere in the code which are specific to color palette 0
> or color palette 1, but I would like to do this in a generic fashion,
> regardless of the color palette in use before the call to
> plimage/plimagefr.
>
>
> > > Ideally someone would play with dev_fastimg vs !dev_fastimg and see if
> there's
> > > a noticeable difference on mondern hardware. I'm sure many here have
> seen
> > > hardware advances make irrelevant some "optimizations" done previously,
> or at
> > > least mitigate performance concerns.
> >
> > I agree. If it turns out that dev_fastimg really is useful then I would
> > encourage you to think about (2), i.e. still using dev_fastimg for the
> > no-transform case at least. Without the tests it is hard to comment
> > though.
>
> There is a speed difference between the two - running example 20 using
> the xwin driver under PLplot 5.9.0 is quicker than with the attached
> patch. This only applies for the xwin driver though, as none of the
> other output drivers have dev_fastimg support. Adding dev_fastimg
> support back in should probably involve input from the output driver
> maintainers so that the interface is reasonably easily implemented and
> kept in sync with the currently popular and maintained drivers.
>
> That said, the speed difference is negligible for smaller images. The
> speed of example 20 is different but not very noticeable on my laptop
> (Pentium-M 1.7ghz) though this may add up to be significant if many
> images were plotted. I do not know what the speed difference would be
> for 1000x1000 and larger image arrays.
>
> I think the speed issue comes from using plfill to draw every pixel of
> the image. This is how the previous implementation of the slow
> rendering path works as well. The xwin dev_fastimg approach uses a
> custom rasterizer in xwin.c which this patch comments out.
>
>
> > > For example, the X driver was first developed on 8-bit r/w color
> displays and
> > > sharing a single r/w colormap was the norm. If this didn't suffice for
> the
> > > application, a custom colormap could be used (which I never liked very
> much).
> > > Seems so quaint now. :) But when I went to 16 or 24 bit r/o colormaps
> on a
> > > Linux box years later, the performance degradation of swapping out
> colors
> > > really didn't seem to matter much. One of these days I'd like to give
> the
> > > xwin driver a bit of housecleaning, starting with chopping out the
> custom
> > > colormap support that was never really used anyway.
> >
> > If you can find the time, it sounds like a good idea! I still
> > extensively use the xwin driver. I prefer it over the more sophisticated
> > gnome driver for example for many purposes because it is quick and
> > clean.
>
> I use the xwin driver quite often as well, largely because of the
> threading support. This makes it easier to quickly see results in an
> interactive environment.
>
>
> > P.S. Hez, is the bug fix easy to tease out from the rest of your patch?
> > We should at least apply that as we think about the rest it. Committing
> > this separately also makes it clear what is bug fix and what is new
> > feature. This can be useful when looking back through the commits. If
> > the bug is only fixed because the broken code is replaced, then don't
> > worry about it.
>
> The bug fix is something that I tackled during the rewrite, rather
> than as a separate issue. I think that the bug would be fixed by
> changing the following lines in plimage.c, function c_plimagefr, line
> 205:
>
> dx = (xmax - xmin) / (nx - 1);
> dy = (ymax - ymin) / (ny - 1);
> nnx = (Dxmax-Dxmin)/dx + 1;
> nny = (Dymax-Dymin)/dy + 1;
>
> to:
>
> dx = (xmax - xmin) / (PLFLT)nx;
> dy = (ymax - ymin) / (PLFLT)ny;
> nnx = ceil((Dxmax - Dxmin) / dx);
> nny = ceil((Dymax - Dymin) / dy);
>
> I have only tested this briefly, but I think it should do the trick.
> My apologies for not providing a separate patch - I am not sure that
> this will fix the issue, but I think it should.
>
> Thanks again for the feedback,
Here is a small follow-up patch to properly handle the case when all
values in an image are the same - this would lead to division by 0,
nan being passed to plcol1 and segfaults under the xwin driver.
This patch applies on top of plimagefr_pltr_support2.diff
Hez
--
Hezekiah M. Carty
Graduate Research Assistant
University of Maryland
Department of Atmospheric and Oceanic Science
diff --git a/src/plimage.c b/src/plimage.c
index 9d15200..a868878 100644
--- a/src/plimage.c
+++ b/src/plimage.c
@@ -223,20 +223,27 @@ c_plimagefr(PLFLT **idata, PLINT nx, PLINT ny,
// are not plotted.
for (ix = 0; ix < nx; ix++) {
for (iy = 0; iy < ny; iy++) {
- datum = idata[ix][iy];
- if (datum < zmin || datum > zmax) {
- // Set to a guaranteed-not-to-plot value
- z[ix * ny + iy] = COLOR_NO_PLOT;
+ if (valuemin == valuemax) {
+ // If valuemin == valuemax, avoid dividing by zero.
+ z[ix * ny + iy] = (COLOR_MAX + COLOR_MIN) / 2.0;
}
else {
- if (datum < valuemin) {
- datum = valuemin;
+ datum = idata[ix][iy];
+ if (datum < zmin || datum > zmax) {
+ // Set to a guaranteed-not-to-plot value
+ z[ix * ny + iy] = COLOR_NO_PLOT;
}
- else if (datum > valuemax) {
- datum = valuemax;
+ else {
+ if (datum < valuemin) {
+ datum = valuemin;
+ }
+ else if (datum > valuemax) {
+ datum = valuemax;
+ }
+ // Set to a value scaled between COLOR_MIN and COLOR_MAX.
+ z[ix * ny + iy] =
+ (datum - valuemin + COLOR_MIN) / (valuemax - valuemin) * COLOR_MAX;
}
- // Set to a value scaled between COLOR_MIN and COLOR_MAX
- z[ix * ny + iy] = (datum - valuemin) / (valuemax - valuemin);
}
}
}
-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
Plplot-devel mailing list
Plplot-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/plplot-devel
