Menu
▾
▴
gnuplot-beta
|
From: <mi...@ph...> - 2004-08-12 07:01:00
|
Daniel has made a great work on implementing support for (binary) images into gnuplot. The result is a patch implementing two new styles "with image" and "with rgbimage", and extending support for reading binary files. I propose the patch gets committed into cvs. --- PM |
|
From: Hans-Bernhard B. <br...@ph...> - 2004-08-12 09:42:31
|
On Thu, 12 Aug 2004 mi...@ph... wrote: > Daniel has made a great work on implementing support for (binary) images > into gnuplot. The result is a patch implementing two new styles "with > image" and "with rgbimage", and extending support for reading binary > files. > I propose the patch gets committed into cvs. I must admit I haven't quite kept up with its development recently, but from what I remember off-hand, there was quite some discussion about the way it integrated its binary datafile reading with datafile.c. It essentially blows up the largest routine in all of datafile.c by yet another factor of two, but with rather little actual connection between the new and old portions. I would have liked to have seen that cleaned up before it goes into CVS, but it can be delayed until later. -- Hans-Bernhard Broeker (br...@ph...) Even if all the snow were burnt, ashes would remain. |
|
From: <mi...@ph...> - 2004-08-12 11:32:18
|
>> I propose the patch gets committed into cvs. > > I must admit I haven't quite kept up with its development recently, but > from what I remember off-hand, there was quite some discussion about > the way it integrated its binary datafile reading with datafile.c. It > essentially blows up the largest routine in all of datafile.c by yet > another factor of two, but with rather little actual connection > between the new and old portions. I would have liked to have seen that > cleaned up before it goes into CVS, but it can be delayed until later. I propose clean up of datafile.c, if you really find this necessary, after the patch is in cvs. The patch is quite big, and its maintenance as a patch needs more effort than necessary. Thus I propose to commit the patch ASAP. I think that Daniel should get rights for cvs. --- PM |
|
From: Daniel J S. <dan...@ie...> - 2004-08-12 12:58:24
|
mi...@ph... wrote: >>>I propose the patch gets committed into cvs. >>> >>> >>I must admit I haven't quite kept up with its development recently, but >>from what I remember off-hand, there was quite some discussion about >>the way it integrated its binary datafile reading with datafile.c. It >>essentially blows up the largest routine in all of datafile.c by yet >>another factor of two, but with rather little actual connection >>between the new and old portions. I would have liked to have seen that >>cleaned up before it goes into CVS, but it can be delayed until later. >> I think there was this issue, and the issue about the way a map3d_xy() routine rounds, especially for PostScript. Ethan said he will be looking at the routine in question some time in the future. But if you want, I can take a look at the rounding issue and see if one is better than the other. My guess is that one case does the rounding directly from a CPU register, the other rounds to return a value on the stack. I could repeat some code to avoid the effect of a double version of map3d_xy(). But I'd rather not introduce superfluous code that someone later could overlook. (It will still remain obvious in the code what the original form of the routine is.) >I propose clean up of datafile.c, if you really find this necessary, after >the patch is in cvs. The patch is quite big, and its maintenance as a >patch needs more effort than necessary. Thus I propose to commit the patch >ASAP. >I think that Daniel should get rights for cvs. > Well, I don't know if that is a good idea. I know the guy. He can run CVS on his own system well enough, where any alterations can be undone by brute force if necessary. But as for expertise of a remote site, that's dicey. Anyway, yes the patch is big and consequently almost any alterations to CVS (the common files that get most development) cause some hunks to fail. But if only one function within datafile.c gets altered, a patch shouldn't go out of date too quickly. I would propose that I build a small patch to clean up that routine in question. But some discussion is required to decide how best to clean up datafile.c, and testing would be required. I think it would be a four to six month time frame for a secondary patch. At that time, if it looks like a little changes are required, which I hope isn't the case, maybe then CVS access. Dan |
|
From: Ethan M. <merritt@u.washington.edu> - 2004-08-12 17:16:09
|
On Thursday 12 August 2004 06:23 am, Daniel J Sebald wrote: > > I think there was this issue, and the issue about the way a map3d_xy() > routine rounds, especially for PostScript. Ethan said he will be > looking at the routine in question some time in the future. My issue with map3d_xy has nothing to do with rounding. The problem is that it returns coordinate values as (unsigned int) without checking for negative numbers. See numerous bug reports about vector and arrow problems whenever one end of a vector is outside of the plot area. When I looked at this before I decided that fixing this properly might require doing away with the routine altogether, and having the callers check for out-of-bound endpoints before converting input coordinates to (unsigned int) screen coordinates. > >I propose clean up of datafile.c, if you really find this necessary, after > >the patch is in cvs. The patch is quite big, and its maintenance as a > >patch needs more effort than necessary. Thus I propose to commit the patch > >ASAP. OK, I guess. But please let's try to get df_readline() cleaned up as the very next thing. And please check one final time that the patched version of datafile.c does not inadvertantly back out other changes made since 4.0. -- Ethan A Merritt merritt@u.washington.edu Biomolecular Structure Center Mailstop 357742 University of Washington, Seattle, WA 98195 |
|
From: Harald H. <h.h...@tu...> - 2004-08-12 17:28:30
|
On Thu, 12 Aug 2004, Ethan Merritt wrote: > On Thursday 12 August 2004 06:23 am, Daniel J Sebald wrote: > > > > I think there was this issue, and the issue about the way a map3d_xy() > > routine rounds, especially for PostScript. Ethan said he will be > > looking at the routine in question some time in the future. > > My issue with map3d_xy has nothing to do with rounding. > The problem is that it returns coordinate values as (unsigned int) > without checking for negative numbers. > See numerous bug reports about vector and arrow problems > whenever one end of a vector is outside of the plot area. > When I looked at this before I decided that fixing this properly > might require doing away with the routine altogether, and > having the callers check for out-of-bound endpoints before > converting input coordinates to (unsigned int) screen coordinates. The problem with negative coordinate values is partly fixed with my relative coordinate patch (#991819). May be applying it could do half the way to fix that problem. By the way: Is there a reason that the group of patches to reach text offsets (#991819, #993411, #993422) neither is applied to cvs nor anybody writes which is wrong with them? Do you think it is unimportant? Yours Harald --=20 Harald Harders Langer Kamp 8 Technische Universit=E4t Braunschweig D-38106 Braunschweig Institut f=FCr Werkstoffe Germany E-Mail: h.h...@tu... Tel: +49 (5 31) 3 91-3062 WWW : http://www.harald-harders.de Fax: +49 (5 31) 3 91-3058 |
|
From: Daniel J S. <dan...@ie...> - 2004-08-13 06:21:36
|
Harald Harders wrote: >On Thu, 12 Aug 2004, Ethan Merritt wrote: > >>My issue with map3d_xy has nothing to do with rounding. >>The problem is that it returns coordinate values as (unsigned int) >>without checking for negative numbers. >>See numerous bug reports about vector and arrow problems >>whenever one end of a vector is outside of the plot area. >>When I looked at this before I decided that fixing this properly >>might require doing away with the routine altogether, and >>having the callers check for out-of-bound endpoints before >>converting input coordinates to (unsigned int) screen coordinates. >> >> > >The problem with negative coordinate values is partly fixed with my >relative coordinate patch (#991819). May be applying it could do half the >way to fix that problem. > > I think this could be a top priority issue. Once in a while I'll zoom and there will be lines going off into seemingly random directions. That kind of thing is unseemly. I could make the binary data file changes Ethan suggested and have a patch ready by monday. This could directly follow. Dan |
|
From: Daniel J S. <dan...@ie...> - 2004-08-12 20:35:40
|
Ethan Merritt wrote: >>>I propose clean up of datafile.c, if you really find this necessary, after >>>the patch is in cvs. The patch is quite big, and its maintenance as a >>>patch needs more effort than necessary. Thus I propose to commit the patch >>>ASAP. >>> >>> > >OK, I guess. >But please let's try to get df_readline() cleaned up as the very next thing. > What are the goals proposed for cleanup? I'd offer up a few intial goals: 1. Clean up the df_readline() routine for neatness. If that means a couple subroutines, that's fine. But even though that function is somewhat big, it's flow of logic is pretty straightforward. 2. Integrate the "gnuplot binary" into the more general binary routine with a few lines of code. Then drop the special routine for reading "gnuplot binary" and if I recall correctly there are a bunch of routines at the end of "datafile.c" that aren't used and could be discarded. (Can always retrieve them from CVS.) 3. Decide a convention of how to properly incorporate info in a datafile into the plot style. There was some discussion about histograms and other plot styles that assume something based upon what is in the data file, i.e., whether that sort of thing should be done, and if so how best to do it. >And please check one final time that the patched version of datafile.c >does not inadvertantly back out other changes made since 4.0. > How does one check that? Compare a diff between current CVS and Gnuplot 4.0 with the patch file? Any mods to CVS not matching what the patch file has for the first few lines of code before and after the change will cause a reject. I've never seen anything in the patched code lost from patching, only what is in the patch file gets rejected. The worst I've seen is a hunk positioned out of order so that it doesn't compile correctly. (Those can be nasty to debug if it's an openning or closing bracket mispositioned.) Dan |
|
From: Ethan M. <merritt@u.washington.edu> - 2004-08-12 22:33:10
|
On Thursday 12 August 2004 02:01 pm, Daniel J Sebald wrote:
> 1. Clean up the df_readline() routine for neatness. If that means a
> couple subroutines, that's fine. But even though that function is
> somewhat big, it's flow of logic is pretty straightforward.
Summarizing my comments of 10-July-2004:
After I apply the with-image patch, df_readline contains 653 lines of
code of which only 303 are shared by the two modes.
The largest block of shared code is the section (lines 3362-3465)
checking for blank lines, skipped lines, and EOF. Is this even relevant
to binary mode?
My view of it is that you basically have two separate routines that are
uneasily co-existing in the same block of code. I think there should be
two routines:
df_readline (existing code = 328 lines)
df_readbinary (code path used by pixel/binary routines = ~450 lines)
By my rough estimate there are at most about 200 lines of code that are
common to both paths. I haven't tried to estimate how much of this may
move easily into a shared subroutine.
There are only 4 callers of df_readline anyway. It is the callers
business to know whether a binary read or a formatted read is
appropriate, and call one of the two alternative input routines.
> >And please check one final time that the patched version of datafile.c
> >does not inadvertantly back out other changes made since 4.0.
>
> How does one check that?
The changes to datafile.c are so extensive that it is very hard
to see exactly what is going on. Can you possibly rearrange
all the brand new routines so that they are together at the end of the file?
In fact, ideally I'd like to see a new file containing all the purely new
routines and also containing the disentangled code from df_readline()
that constitutes the code path for binary files only. The original
df_readline would stay in datafile.c At that point the remaining
changes to datafile.c will probably be small enough to understand,
and much of your new code will be together in one place.
--
Ethan A Merritt merritt@u.washington.edu
Biomolecular Structure Center
Mailstop 357742
University of Washington, Seattle, WA 98195
|
|
From: Daniel J S. <dan...@ie...> - 2004-08-13 00:38:40
|
Ethan Merritt wrote:
>On Thursday 12 August 2004 02:01 pm, Daniel J Sebald wrote:
>
>
>>1. Clean up the df_readline() routine for neatness. If that means a
>>couple subroutines, that's fine. But even though that function is
>>somewhat big, it's flow of logic is pretty straightforward.
>>
>>
>
>Summarizing my comments of 10-July-2004:
>
> After I apply the with-image patch, df_readline contains 653 lines of
> code of which only 303 are shared by the two modes.
>
> The largest block of shared code is the section (lines 3362-3465)
> checking for blank lines, skipped lines, and EOF. Is this even relevant
> to binary mode?
>
>My view of it is that you basically have two separate routines that are
>uneasily co-existing in the same block of code. I think there should be
>two routines:
> df_readline (existing code = 328 lines)
> df_readbinary (code path used by pixel/binary routines = ~450 lines)
>By my rough estimate there are at most about 200 lines of code that are
>common to both paths. I haven't tried to estimate how much of this may
>move easily into a shared subroutine.
>
>There are only 4 callers of df_readline anyway. It is the callers
>business to know whether a binary read or a formatted read is
>appropriate, and call one of the two alternative input routines.
>
>
Well, breaking up the routine into modules is alright. But I don't
agree that a df_readline() and a df_readbinary() is a good idea. That
would require a global variable travelling across datafile.c to
plot2d.c, etc. Inside of plot2d.c is the following construct
while ((j = df_readline(v, max_cols)) != DF_EOF) {
How does one easily reconfigure that to incorporate one of two possible
routines and the global variable to know which is which (df_readline(),
df_readbinary(), df_general_binary)? And why is it important for
plot2d.c to know that it is getting the data from an ASCII or a binary file?
Now, if inside datafile.c you would want something like
int
df_readline(double v[], int max)
{
if (df_general_binary)
return df_read_binary_line(v[], max);
else
return df_read_ascii_line(v[], max);
}
where df_read_ascii_line() is exactly the same as the current
df_readline(), that's easy enough. I would simply copy the existing
code and separate things so that whatever is currently conditional to
df_general_binary gets moved to one function and what is left in the
other. However, there is some code that would be replicated, but not
too bad. Sure, I'm fine with that. That, in fact would weed out some
conditionals in exchange for about ten to twenty lines of replicated
code, so that the total net loss/gain is probably zero. I opt for
keeping static variables local to C files, unless well organized.
So, is everyone alright with changing df_readline() as above and calling
the current df_readline() df_read_ascii_line()?
>
>
>>>And please check one final time that the patched version of datafile.c
>>>does not inadvertantly back out other changes made since 4.0.
>>>
>>>
>>How does one check that?
>>
>>
>
>The changes to datafile.c are so extensive that it is very hard
>to see exactly what is going on. Can you possibly rearrange
>all the brand new routines so that they are together at the end of the file?
>
Sure. Easy enough. Functions down to the bottom of the file... but not
down below the tables of any of the trm files, right? And new tables to
the tops of files... at the bottom of existing tables, or at the top of
existing tables?
>In fact, ideally I'd like to see a new file containing all the purely new
>routines and also containing the disentangled code from df_readline()
>that constitutes the code path for binary files only. The original
>df_readline would stay in datafile.c At that point the remaining
>changes to datafile.c will probably be small enough to understand,
>and much of your new code will be together in one place.
>
What do you want this new file called? binfile.c? databin.c?
So, this will be integrated into CVS then?
Dan
|
|
From:
<br...@ph...> - 2004-08-13 06:55:44
|
Daniel J Sebald wrote: > Well, breaking up the routine into modules is alright. But I don't > agree that a df_readline() and a df_readbinary() is a good idea. That > would require a global variable travelling across datafile.c to > plot2d.c, etc. That cannot possibly be the case, given the fact that your current code manages without such a global, or any other indication of what's going on. So it must obviously be able to figure out whether normal or binary file reading is being called for. At the worst, you would need three routines: df_readbinary() df_readascii() /* holds the bulk of what is now df_readline() */ df_readline() /* very short, just branches into one of the above two */ |
|
From: Daniel J S. <dan...@ie...> - 2004-08-13 13:19:49
|
Hans-Bernhard Br=F6ker wrote: > Daniel J Sebald wrote: > >> Well, breaking up the routine into modules is alright. But I don't=20 >> agree that a df_readline() and a df_readbinary() is a good idea. =20 >> That would require a global variable travelling across datafile.c to=20 >> plot2d.c, etc. > > > That cannot possibly be the case, given the fact that your current=20 > code manages without such a global, or any other indication of what's=20 > going on. So it must obviously be able to figure out whether normal=20 > or binary file reading is being called for. At the worst, you would=20 > need three routines: > > df_readbinary() > df_readascii() /* holds the bulk of what is now df_readline() */ > df_readline() /* very short, just branches into one of the above=20 > two */=20 That is what I'm proposing. Dan |
|
From: Ethan M. <merritt@u.washington.edu> - 2004-08-12 23:30:14
|
On Thursday 12 August 2004 12:00 am, mi...@ph... wrote: > I propose the patch gets committed into cvs. Question: The patch contains a new file gplt_x11.h Why is it marked Copyright 2000 Thomas Williams, Colin Kelley This seems very unlikely to be correct. -- Ethan A Merritt merritt@u.washington.edu Biomolecular Structure Center Mailstop 357742 University of Washington, Seattle, WA 98195 |
|
From: Daniel J S. <dan...@ie...> - 2004-08-13 00:46:27
|
Ethan Merritt wrote: >On Thursday 12 August 2004 12:00 am, mi...@ph... wrote: > > >>I propose the patch gets committed into cvs. >> >> > >Question: > >The patch contains a new file gplt_x11.h >Why is it marked > > Copyright 2000 Thomas Williams, Colin Kelley > >This seems very unlikely to be correct. > I did a block copy from some other header file when that was created and didn't think about it again. Hans or someone probably updated all Copyright notices in all other files before the 4.0 release. That would be my guess. That new header is meant to make common some of the codes and parameters used on both sides of the gnuplot_x11 pipe. (A start anyway.) What should the notice be? In fact, what should the notice be for this new "binary datafile.c" file? Dan |
|
From: Ethan M. <merritt@u.washington.edu> - 2004-08-12 23:37:45
|
On Thursday 12 August 2004 12:00 am, mi...@ph... wrote:
> I propose the patch gets committed into cvs.
I also find this comment in post.trm a little alarming:
+ /*** FIX ME!!! *****
+ *
+ * I had to put this grestore command here, otherwise the
+ * case where multiple images are plotted on the same graph
+ * fails after the first image. I have no idea what it does.
+ * I'm guessing it restores the stack or something and that
+ * it must be paired with gsave. It seems to me there is a
+ * gsave in the palette routine that doesn't have a matching
+ * grestore. Could that be the problem?
+ */
+ fputs("grestore\n", gpoutfile);
--
Ethan A Merritt merritt@u.washington.edu
Biomolecular Structure Center
Mailstop 357742
University of Washington, Seattle, WA 98195
|
|
From: Daniel J S. <dan...@ie...> - 2004-08-13 00:58:33
|
Ethan Merritt wrote:
>On Thursday 12 August 2004 12:00 am, mi...@ph... wrote:
>
>
>>I propose the patch gets committed into cvs.
>>
>>
>
>I also find this comment in post.trm a little alarming:
>
>+ /*** FIX ME!!! *****
>+ *
>+ * I had to put this grestore command here, otherwise the
>+ * case where multiple images are plotted on the same graph
>+ * fails after the first image. I have no idea what it does.
>+ * I'm guessing it restores the stack or something and that
>+ * it must be paired with gsave. It seems to me there is a
>+ * gsave in the palette routine that doesn't have a matching
>+ * grestore. Could that be the problem?
>+ */
>+ fputs("grestore\n", gpoutfile);
>
>
>
Hey, that's one of those "that's the way it works" sort of things. :-)
I noted what at the time probably seemed like the source of problems.
However, a year ago, and even now, if I explained to the list I thought
there was a grestore missing in some PostScript code that seemed to work
fine the way it was, the reaction would have been "don't believe it".
Dan
|
|
From: Daniel J S. <dan...@ie...> - 2004-08-13 05:36:15
|
Daniel J Sebald wrote:
>
>
> Ethan Merritt wrote:
>
>> On Thursday 12 August 2004 12:00 am, mi...@ph... wrote:
>>
>>
>>> I propose the patch gets committed into cvs.
>>>
>>
>>
>> I also find this comment in post.trm a little alarming:
>>
>> + /*** FIX ME!!! *****
>> + *
>> + * I had to put this grestore command here, otherwise the
>> + * case where multiple images are plotted on the same graph
>> + * fails after the first image. I have no idea what it does.
>> + * I'm guessing it restores the stack or something and that
>> + * it must be paired with gsave. It seems to me there is a
>> + * gsave in the palette routine that doesn't have a matching
>> + * grestore. Could that be the problem?
>> + */
>> + fputs("grestore\n", gpoutfile);
>>
>>
>>
>
> Hey, that's one of those "that's the way it works" sort of things.
> :-) I noted what at the time probably seemed like the source of
> problems. However, a year ago, and even now, if I explained to the
> list I thought there was a grestore missing in some PostScript code
> that seemed to work fine the way it was, the reaction would have been
> "don't believe it".
Sorry, my bad. That message is just some bogus note from when I first
did the code. There *should* be a grestore there because some of the
image commands alter the graphics state, probably. The state was
probably at the top of the stack so "grestore" without a matched "gsave"
didn't make a difference. I put a "gsave" near the top of the routine
to ensure a match.
Dan
|
×
Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.
Thanks for helping keep SourceForge clean.
X