On Mar 10, 2015, at 9:21 AM, Phil Rosenberg <p.d.rosenb...@gmail.com> wrote:
> Hi Jim > Unfortunately after applying your patch sequence I am having build > problems. I am building the static libs with Visual; Studio on Windows > and when I try to build the examples I get > > plplot.lib(plmeta.obj) : error LNK2019: unresolved external symbol > handle_fsetpos_failed referenced in function UpdatePrevPagehdr Strange, I see a difference between my repository and what is on sourceforge. The handle_setpos_failed() (line 590 in my repository, line 485 on the version currently in sourceforge) should be handle_fsetpos_failed(). Somehow that change was lost somewhere. > > I have made a couple of modifications on top of your patch sequence > (see below) so please could you have a look at fixing the problem, > then create a patch containing ONLY THE MODIFICATIONS AFTER THE PATCH > SEQUENCE YOU SENT. I think you can do this by creating a patch only > containing specific commits or by creating a new branch from your > current working branch. > Which patch sequence? The one sent 3/7 or 3/2? I have no changes after 3/7. There is a third patch that I sent to update the build instructions for Mac OS X that does not seem to have been applied. > As I said I have made a couple of other mods and have some questions > about the changes in the patch, I've listed these below: > > You seem to have removed the check for a NULL pointer in > PLESC_HAS_TEXT option of plbuf_esc(). However, there was a comment > next to it saying Check required by GCW driver, please do not remove. > I haven't checked to see why it was required, but just wondered why > you removed it? Is it essential it is removed? have you checked it is > safe to remove? > I moved it from the plbuf_esc() function into the plbuf_text(). I wanted to have parameter validation in the function that implements the operation rather than the dispatch function. Actually, I should amend the comment because that check is now required by all drivers that use PLESC_HAS_TEXT. > On the face of it the patch makes it look like you have made some > major changes to the text code, however I suspect you have made just > enough minor changes that the diff algorithm saw fit to replace a very > large hunk with another almost identical hunk. I don't suppose you > could just give a rundown of the changes you made to the text code and > why? > Actually, I did do some significant code reorganization. The plP_text function had two unicode processing pathways in it and I found it to be very confusing what it was doing because the function was very long. When I worked through the paths, I noticed that unicode strings were being processed twice and rendered twice on xcairo. I pulled out the unicode processing pathways into two functions that plP_text call, which makes the code more readable. The other modification, which is relatively minor, was to always include the string passed by the caller in args.string so that the plmeta driver can store the original string in the metafile. That enables better support for devices that can render text. > I haven't looked in much detail at the changes to plmeta.c, as far as > I am concerned this is your domain. However it looks like you have not > gone down the route of using the buffer as a source of data for the > device. I think post release this should be looked at for the reasons > we discussed previously (guarentees the driver is kept up to date, > useful code reuse, avoids introducing new code to maintain) although I > am sure you have reasons for using the method you did and > fundamentally the plmeta device will certainly be much improved. > The changes required to use plbuf at this point in time was too much this close to release. > I personally am a bit uncomfortable with the way plbuf_bop() calls > wr_command(). Although I can see the code reuse benefits somehow it > feels wrong that we start generating sub commands within commands and > I can't quite put my finger on why it feels wrong. Perhaps someone > else might have comments. If I'm honest I would much rather see a call > to c_plscmap() in plP_bop() which gives the code reuse, but without > the uncomfortable feeling. I'm confused by this comment. The wr_command( pls, (U_CHAR) BOP ) indicates the BOP command when the buffer is executed (e.g. by a resize). The key thing that it does is restore the state at BOP. Do you mean the the two commands for saving the colormaps? A call to c_plscmap() would not work in plbuf_bop() because the colormap needs to be written to the buffer. I could easily remove the plbuf_control(…) and substitute the two wr_data() commands. I did PLSTATE_CMAP# way originally because I was relying the colormap being restored as a PLSTATE_CMAP# operation when the buffer was executed. However, to fix the bug you identified the original approach does not work. > > I have restored the static status of plhrsh as it seems to not be used > in plmeta after all.plhrsh > Yep, based on Alan's suggestion I went the string approach and exposing plhrsh is not needed. > Hope this all sounds okay to you? I'm open to discussion on any of > these points so if I have missed any reasoning anywhere let me know. > > Phil > > > On 9 March 2015 at 12:30, Phil Rosenberg <p.d.rosenb...@gmail.com> wrote: >> Just saw your other email to the list so don't worry :-) I will try to >> look at the patch series tonight. >> >> Phil >> >> On 9 March 2015 at 12:27, Phil Rosenberg <p.d.rosenb...@gmail.com> wrote: >>> Hi Jim (back on list) >>> Is this fix committed to the online repo or just in your local repo? >>> >>> Phil >>> >>> On 8 March 2015 at 03:47, Jim Dishaw <j...@dishaw.org> wrote: >>>> I fixed the bug and it was a lot simpler than I thought. I was concerned >>>> that the xwin driver was changing the colormap, which would cause the pen >>>> color to change. Fortunately the xwin driver allocates pen colors >>>> independently of the plplot colormaps. >>>> >>>> I fixed the state saving that plbuf_bop does so that rdbuf_bop restores the >>>> correct colors. >>>> >>>> On Mar 5, 2015, at 3:58 AM, Phil Rosenberg <p.d.rosenb...@gmail.com> wrote: >>>> >>>> Hi Jim. >>>> It is perhaps both more and less straightforward than you consider in your >>>> email. Plplot honours changes in colour maps midway through a plot. So for >>>> example I could plot one dataset, change the colour map and then plot a >>>> different dataset. This would be not only possible, but very likely for >>>> colour map 1. >>>> >>>> Currently in this situation a replot would result in both datasets using >>>> the >>>> second colourmap. >>>> >>>> The fix, however, is not so complex. Either pl_bop, must call plcol0, >>>> plwidth, set the colour maps etc, using the current values which will >>>> result >>>> in them ending up in the buffer, or a bop event in the buffer must store >>>> all >>>> these values, then on replot it must reinstate them and call the >>>> appropriate >>>> functions so the state gets passed to the driver. >>>> >>>> The only alternative is to reset all state parameters to their defaults on >>>> bop. This would break most of our examples and most user code I imagine (it >>>> would certainly break all mine). >>>> >>>> So in summary, this bug has even more complex and likely repercussions than >>>> you mention in your message, but the fix is pretty straightforward. >>>> >>>> Phil >>>> ________________________________ >>>> From: Jim Dishaw >>>> Sent: 05/03/2015 04:20 >>>> To: Phil Rosenberg >>>> Subject: Re: [Plplot-devel] buffer state parameters >>>> >>>> I think I tracked down the bug and it might be tricky. I noticed that when >>>> I put a pladv(0) before changing the colormap (my first guess at the >>>> problem) the plot turned all yellow (the last color used) on a resize. So >>>> I >>>> think there are two issues here. >>>> >>>> If it is just the wrong pen color is selected, then restoring the correct >>>> state is a simple fix. If it is a colormap changing issue, then the bug is >>>> more difficult to fix. >>>> >>>> The EOP does not get generated until the next map call, thus all the >>>> colormap setup happens on the end of page #4. When the plot buffer is >>>> executed on a resize, the colormap is changed at the end and that causes >>>> the >>>> problem. >>>> >>>> The only two solutions I see are: >>>> >>>> 1) Require a pladv() prior to a colormap change that is for a new page. >>>> 2) Have plbuf ignore colormap changes that occur at the end of the page >>>> when >>>> replaying the plot buffer and then executing them at the next BOP. >>>> >>>> I will work on this tomorrow, but I wanted to give you an update. >>>> >>>> On Mar 4, 2015, at 4:03 PM, Phil Rosenberg <p.d.rosenb...@gmail.com> wrote: >>>> >>>>> Okay >>>>> I will leave that with you then >>>>> >>>>> Thanks >>>>> Phil >>>>> >>>>> On 4 March 2015 at 15:07, Jim Dishaw <j...@dishaw.org> wrote: >>>>>> >>>>>>> On Mar 4, 2015, at 9:13 AM, Phil Rosenberg <p.d.rosenb...@gmail.com> >>>>>>> wrote: >>>>>>> >>>>>>> Hi Jim >>>>>>> There is still a bug in the buffer with respect to the correct state >>>>>>> parameters. I think we discussed this in the past regarding plplot >>>>>>> initialisation, but this particular bug occurs at page change. >>>>>>> >>>>>>> Basically at the beginning of a render, plplot inherits it's state >>>>>>> from the previous render. Mostly when we simply move from one page to >>>>>>> the next this behaviour gives expected results. However if we rerender >>>>>>> a page twice, then the second render inherits the state from the first >>>>>>> render which is wrong. >>>>>>> >>>>>>> A good example is x19.4. At the end of this page the colour table is >>>>>>> changed (including the background colour) in preparation for page 5. >>>>>>> If you resize page 4 under the X driver you will see that the >>>>>>> background and lines change colour because this new colour table is >>>>>>> being incorrectly used. >>>>>>> >>>>>>> I have seen a similar thing happen with the line width and presumably >>>>>>> other state variables behave similarly too. >>>>>>> >>>>>>> Basically I wanted to check if this is something that is on your >>>>>>> radar? I am happy to fix the problem, but I don't want to create a >>>>>>> clash with your current work. >>>>>>> >>>>>> >>>>>> I will fix and patch. I will also make a test case. >>>>>> >>>>>>> Phil >>>>>>> >>>>>>> >>>>>>> ------------------------------------------------------------------------------ >>>>>>> Dive into the World of Parallel Programming The Go Parallel Website, >>>>>>> sponsored >>>>>>> by Intel and developed in partnership with Slashdot Media, is your hub >>>>>>> for all >>>>>>> things parallel software development, from weekly thought leadership >>>>>>> blogs to >>>>>>> news, videos, case studies, tutorials and more. Take a look and join the >>>>>>> conversation now. http://goparallel.sourceforge.net/ >>>>>>> _______________________________________________ >>>>>>> Plplot-devel mailing list >>>>>>> Plplot-devel@lists.sourceforge.net >>>>>>> https://lists.sourceforge.net/lists/listinfo/plplot-devel >>>> >>>> ------------------------------------------------------------------------------ Dive into the World of Parallel Programming The Go Parallel Website, sponsored by Intel and developed in partnership with Slashdot Media, is your hub for all things parallel software development, from weekly thought leadership blogs to news, videos, case studies, tutorials and more. Take a look and join the conversation now. http://goparallel.sourceforge.net/ _______________________________________________ Plplot-devel mailing list Plplot-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/plplot-devel
