No problem! Responses are inline below.
Andrew
On Thu, Oct 11, 2012 at 03:22:34AM -0700, phil rosenberg wrote:
> Hi Andrew
> ?
> Thanks for your comments, massively useful. I have added some further
> comments/questions below.
> ?
> Cheers
> ?
> Phil
>
>
> ________________________________
> From: Andrew Ross <andrewr...@users.sourceforge.net>
> To: phil rosenberg <philip_rosenb...@yahoo.com>
> Cc: "plplot-devel@lists.sourceforge.net" <plplot-devel@lists.sourceforge.net>
> Sent: Wednesday, 10 October 2012, 21:38
> Subject: Re: [Plplot-devel] map resolution
>
>
>
> Phil,
>
> Thanks for your patch. A great start. I've got a few comments.
>
> To get it to compile on Linux required a couple of changes:
>
> 1) Debian installs the include file shapefil.h in /usr/include not in a
> shapelib subdirectory. Changing the #include statement in plmap.c fixed
> this. Phil, where is yours installed? Is the subdirectory a standard
> thing, or something you created? Making use of the ${SHAPELIB_INCLUDE_DIR}
> variable should allow you to search in the correct place.
>
> The joy of windows - there is no standard directory, in the source tree the
> shapefil.h header is in the top level. I always create a
> librarytopleveldir/include/libraryname folder for headers and add the include
> MAKE at all so is there any chance you could create a #define
> SHAPELIB_INCLUDE_DIR based on the variable.
The way to do this is to use the cmake variable to set the include path on
the command line. My latest commit to src/CMakeLists.txt does this, so you
should now be able to set CMAKE_INCLUDE_DIR and then just have
"#include <shapefil.h>"
in src/plmap.c.
> 2) plplotd needs to be linked with the shp library. To achieve this I
> added the following code to src/CMakeLists.txt
>
> if(HAVE_SHAPELIB)
> ? set(
> ? ? libplplot${LIB_TAG}_LINK_LIBRARIES
> ? ? ${libplplot${LIB_TAG}_LINK_LIBRARIES}
> ? ? shp
> ? ? )
> endif(HAVE_SHAPELIB)
>
> Does plplotd link with the other libraries used by plplot? Thisbehaviour
> doesn't occur on my Windows?system and I have to link my final code against
> the other dependancies myself. Do you want to commit this change yourself or
> shall I include it in my patch?
My patch also ensures that plplotd is properly linked to libshp. It's a
bit more comprehensive than the above. At least on Linux this means that
you don't need to explicitly link to the shapelib library when using
plplot.
>
> 3) The code generated the following gcc warning
> /home/andrew/software/plplot/plplot/src/plmap.c:48:1: warning: 'visibility'
> attribute ignored [-Wattributes]
>
> The problem here is the static attribute. This makes the scope of the
> variable local to this file, and hence visibility attributes don't make
> sense. I'm guessing this is not what you wanted. Looks like this bit of
> code was just copied from src/plctrl.c. The plplotLibDir variable there
> is only actually used by the tcl code to pass the tcl library directory.
> I would just remove all references to this here.
>
> Good spot, I didn't get the warning. I wasn't sure about the use of
> plplotLibDir so was just trying to preserve it. I will remove references to
> it.
>
> Other comments:
>
> - Having made these changes the code compiled and example 19 ran. I tried
> this _before_ downloading any shapefiles. As it stands your patch doesn't
> fall back to using the old map code. I'd suggest a fallback would be
> sensible for compatibility. One might want to use shapefiles and
> old plplot maps (at least for now). At least it needs an error message if
> the file can't be found!
>
> My fallback strategy will be to create a globe.shp, etc based on the contents
> of globe.map I was just waiting for the code to be tested before I did it. I
> think this is the "method of least surprise." If a user selects to use
> shapefiles then they should use shapefiles easch time.
I don't feel too strongly about this (perhaps others do?)
I do think there should be a warning message if the file is not found
though.
> - The function OpenShapeFile still contains mentions of plLibOpenPdfstr.
>
> I'll make sure this is removed
>
> - As a general coding principle I try to avoid using macros in the
> manner you have done with OpenMap and CloseMap. It's not wrong, it
> just leads to code that can be harder to read. It's generally better to
> include code in #ifdefs as then makes it explicit what functions are
> being called and under what circumstances. Macros can also confuse some
> debuggers.
>
> I'll change this then
>
> - As a further basic principle I'd try to ensure that the different code
> paths (shapelib and plplot format) are as similar as possible, and share
> code where appropriate. As an example, bufx and bufy are statically
> created for the old code path and allocated with malloc in the shapelib
> code path. Some of this code could be shared.
>
> Yeah, I was just trying to keep as much old code as possible, but I agree
> there can be more shared code this way, I'll change it
>
> - I've had a quick look at the transform code. Making multiple copies of
> the data seems like a slightly inefficient way of solving the problem. I
> wonder if there is a better algorithm for fixing the problem? I would
> suggest creating a simple example to illustrate the problem (maybe a
> new page for example 19). This way we would have a test case try out
> new algorithms.
>
> It might be inefficient, but I would guess (and it is a guess without
> profiling) that the time to make multiple copies is less than the time to
> read the data from the hard drive. I could instead reuse the same data, but
> would need to itterate over it 5 times, which I'm not sure would be much
> quicker.
> The method that was there before, plain didn't work, neither did the method
> that it had replaced that was commented out. This method should be as general
> as possible so should work with any shapefile (with units of degrees lat/
> lon) which is important if users might try their own shapefiles. I can
> certainly implement another algorithm if you have any suggestions.
Clearly this is a bug fix, and is somewhat separate from the shapelib
support. Perhaps stick with your method for now. If it proves burdensome
then we can look at alternative ways of solving the problem.
------------------------------------------------------------------------------
Don't let slow site performance ruin your business. Deploy New Relic APM
Deploy New Relic app performance management and know exactly
what is happening inside your Ruby, Python, PHP, Java, and .NET app
Try New Relic at no cost today and get our sweet Data Nerd shirt too!
http://p.sf.net/sfu/newrelic-dev2dev
_______________________________________________
Plplot-devel mailing list
Plplot-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/plplot-devel
