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.
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?
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.
- 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.
Once I downloaded the shapefiles following your pointer to
www.naturalearthdata.com (great site by the way!) all worked fine so
these comments are mostly stylistic in nature.
Regards
Andrew
On Mon, Oct 08, 2012 at 03:27:25PM -0700, phil rosenberg wrote:
> Well I've managed to find a little time between changing dirty nappies so
> please find attached my first cut at a patch for shapelib
> support in plmap
> I???ve made a few changes to plmap to do this:
> ??
> First is the simple reading of the data from the shapefiles
> rather than the original files. The .shp and .shx files are needed, none of
> the other shapefile files are used. ??The shapelib files can be placed and
> referenced exactly like the old files, so either in a ???special place???
> (data
> folder, exe folder, etc) and referenced by filename or anywhere else
> referenced
> by full path. Note that the extension can be omitted or included as per the
> shapelib API.
> ??
> Second is the transformation is now performed after the
> wraparound fixing. This is because the transformation may be into metres or
> some other such length unit if a map projection is used (I've just been
> working with data transformed onto a transverse mercator projection which
> does exactly this) making wraparound fixing impossible after the
> transformation has been applied.
> ??
> Third is that wraparound fixing has been rewritten. This was
> because the previous method broke near the equator, particularly in Africa
> where there are a number of long straight political boundaries (so
> point-to-point lon changes can be large and lat can be small). It may be of
> interest that the new method means that if the user specifies lon on 0-360
> scale, but the map is -180-180 (or visa-versa) then the features still plot
> correctly.
> ??
> From the previous discussions I???m sure there will be a
> number of avid testers. Example 19 uses plmap, but to run it??you will need
> to get a replacement
> shapefile map for the globe and usaglobe original maps. For testing I grabbed
> the 110 m resolution countries map from
> http://www.naturalearthdata.com/features/and made two copies of the .shp and
> .shx files, renaming them globe and
> usaglobe. Putting these in the same location as the usual map files will give
> you a functioning example 19.
> ??
> If/when people are happy with the patch I'll generate shapefile equivalents
> of the current maps which will mean that existing code will produce identical
> output whether or not PLplot has been compiled with shapelib support.
> ??
> The code currently plots polygons and ploylines only, and in both cases plots
> these as lines with no fill. I had wondered about adding the ability to plot
> polygons with a fill, but this requires a change or addition to the public
> API and I'm not sure how useful it would be - It would probably need some way
> to link different fill syles to different objects within a file and I think
> we already discussed that this is something it is best to let people code
> themselves rather than try to turn PLplot into GIS software. Although if
> people think that it would be useful to have a fill option it's relatively
> little effort to do - discus!!!
> ??
> Let me know if the patch works for you.
> ??
> Phil
>
>
>
> ________________________________
> From: Alan W. Irwin <ir...@beluga.phys.uvic.ca>
> To: phil rosenberg <philip_rosenb...@yahoo.com>
> Cc: "plplot-devel@lists.sourceforge.net"
> <plplot-devel@lists.sourceforge.net>; Andrew Ross
> <andrewr...@users.sourceforge.net>
> Sent: Friday, 5 October 2012, 21:38
> Subject: Re: [Plplot-devel] map resolution
>
> By the way, I looked for a shapefile viewer on Debian and found
> thuban. I don't know whether that is the best shapefile viewer for
> Linux, but it is based on libgda/ogr.?? When I tried thuban on overall
> shapefile maps for British Columbian (obtained by following links at
> http://downloads.cloudmade.com/) it appeared to work instantaneously
> and well for the size (which totalled 200MB) of the 7 shapefile layers
> making up the map of British Columbia that is provided by
> http://downloads.cloudmade.com/.?? Furthermore, I was much impressed
> with all the high-resolution local detail (e.g., coastlines, natural features,
> political boundaries to name three of the most useful shapefile layers
> available for the British Columbia map) that was available under the
> thuban zoom mode.?? By the way, that zoom mode worked essentially
> instantaneously as well.
>
> I assume shapelib will be as fast or faster than libgda/ogr so I think
> we have a lot to look forward to concerning the speed with which
> shapelib can deliver shapes behind the scenes to plmap for that
> function to plot.
>
> And to anticipate a possible ( :-) ) further question from Phil, no I
> don't think we should get into trying to let plmap deal with several
> shapefile file layers at once. Instead it should be the users'
> responsibility to specify, e.g., "british_columbia_coastline.shp" as a
> filename to plmap if they want the B.C. coastlines on their plot, and
> if a user wants another layer in their plot with natural features on
> top of those coastlines, they can call plmap again with a different
> filename for the same region, e.g., "british_columbia_natural.shp".
>
> Alan
> __________________________
> Alan W. Irwin
>
> Astronomical research affiliation with Department of Physics and Astronomy,
> University of Victoria (astrowww.phys.uvic.ca).
>
> Programming affiliations with the FreeEOS equation-of-state
> implementation for stellar interiors (freeeos.sf.net); the Time
> Ephemerides project (timeephem.sf.net); PLplot scientific plotting
> software package (plplot.sf.net); the libLASi project
> (unifont.org/lasi); the Loads of Linux Links project (loll.sf.net);
> and the Linux Brochure Project (lbproject.sf.net).
> __________________________
>
> Linux-powered Science
> __________________________
> ------------------------------------------------------------------------------
> 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------------------------------------------------------------------------------
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
