Ciao Adrian,
please read below...
On Feb 9, 2008 12:27 PM, Adrian Custer <acuster@anonymised.com> wrote:
Hey Daniele and Simone,
Thanks for the update. Below are some suggestions for the documentation
I would hope you could give our users, probably on the module wiki page,
as you land your new unsupported module into geotools.
With this new module, we are going to have a bunch of work to do to
resolve the issues you talk about. Since there's lots of potential for
confusion, I hope we could resolve the project management stuff to help
users and ourselves (me) sort our way through the complexity. Also I
would really like some concrete vision of the work need to resolve the
outstanding issues and how they can be resolved.
1. Code duplication: the new code does the same thing as old code
so we need (a) to be really clear to users about the deprecation
of the old code, the transition to the new code, and the
timelines involved, and (b) to be really clear to ourselves that
the cleanup work will happen and give ourselves systematic
updates on where we are headed. For example, if the only
requirement to swing from one set of plugins to another will be
to refactor Abstract*... could that work not be scheduled early
in the evolution of the module so we know how the module will
plug into the current system?
The new code will replace the old one when will be ready. Since people
are already using it we cannot just wipe it out.
Anyway it will take a few weeks at least, before we will do that,
because once this new structure will be ready, we'll have to test it
with geoserver very carefully.
For the moment there is no time to refactor/replace the Abstract*
classes, but we simply trying to minimize the dependency.
I am sparing some time to elaborate more on jody and aaime's proposal
for this but I don't have much spare time.
2. Dependency and build fragility: your mail confirms that you have
seen the issues involved with all this. I hope you could lay out
the issues in the module wiki page and present the plans you
have for resolving them. Last night I started looking at the ECW
code which appears to be: Take ER mapper's sdk and wrap that
with GDAL, take GDAL and wrap that with SWIG, take that and wrap
that with image-io.ext, take that and plug that into geotools
possibly compling with a profile. Perhaps we have no other way
to work around this chain but because it's so convoluted we will
need clarity.
I am sure people can find enough information on this on the net if
they are interested in ECW or something like that, otherwise they can
just ignore it. That's why whe are using aplugin system I think.
I am wondering if you know the details of VMAP or MIF or ARCSDE or H2
since you never asked clarification for them :-)!
For example, I presume you are tracing and
profiling across that chain, and other developers building on
geotools are going to need to know how to do that as well so
this info needs to be in the module page. Also, Jody talks about
a maven profile for compilation, does that mean that the code
will be out of the build, absent this profile? And will the
profile pull out the old modules at the same time? Also, does
all of image-io.ext use GDAL or only the newer code?
We are open to suggestions on how to handle the native code dependency
for geotools.
Imageio-ext proposes a solution but, of course, if something better
comes up we can use it.
3. Build instructions: Given it's such a long dependency chain,
geotools users (me) are going to need early access to your setup
document.
The document has been available for a few months now and it is open to
improvements since imageio-ext is open source project.
(It took me a long time to find the 'guest' account
with the empty password used to get access to the svn in order
to get access to the doc).
Blame SUN for this.
This doc also needs polish;
You might want to subscribe as a developer to the project and send
suggestions , as I said this is an open source project.
it would
be really good if you were not suggesting running make as a
superuser! (use --prefix=/some/dir/ and LD_LIBRARY_PATH instead
as needed)
See above.
So, Simone, your answer starts the process but ideally (1) you would
flesh this out for us and (2) you would get that info on the wiki rather
than on the mailing list. It will keep me from bugging you next time I
wander back into your coverage world, maybe when I get to write up the
coverage core explanation.
It would be great to have some more documentation on coverage.
CODE:
----
I took an initial glance at the code (yeah! Java is so much more fun
than project management) but (boo!) I'm not yet at the substance of the
code just on the periphery :-(.
GDALUtilities:
-------------
IllegalArgException or NullPointer but not both.
You mix and match your response to getting a null argument where an
object is needed. I'd hope we could reach a decision geotools-wide but,
absent that, let's at least each make a consistent decision in our
modules. Actually, perhaps I am wrong and you are using NullPointer in
general but merely have an incorrect use of IllegalArg for a null along
the way.
Why the check on the LOGGER being able to log?
Lots of your geosolutions code has a check in the form of: (from memory)
if ( LOGGER.isLoggable(Level.WARNING) )
LOGGER.log(Level.WARNING, ...
which seems redundant with the work of the logging system. Why are you
doing this? Is there some complex combination of different possible
logging systems that makes us want to be redundantly defensive? If so,
can we get comments around these blocks explaining their necesity?
Catching 'Exception' needs explanation.
If you are going to silently catch all exceptions, it would be good if
you explained why you are allowed to do so. The following is SCARY:
} catch (Exception e) {
// do nothing simply proceed.
}
since I don't understand why you think that the only exceptions you may
catch are unimportant so I have no way to evaluate what may happen here.
(See also next.)
GDALImageReader:
---------------
Explanation for the structure of Exceptions:
Taking a look at the setInput(.) method it seems that you are doing
really wierd things with exceptions. E.g.:
} catch (IOException e) {
throw new RuntimeException("Not a Valid Input", e);
}
this makes no sense to me since IOEx can happen if I have valid input
but just ripped out my usb cable to my external drive. Why catch the
real exception to hide it with another? Also, at the end of the method,
you've had an argument good enough to work with the whole way but it
turns out you don't support the format...seems like it's not an
IllegalArg exception we should be throwing. The exception stuff goes on
being confusing elsewhere in the class as well where you have empty
catch blocks and such which need explanation for me to understand why
they are valid rather than sloppy.
OKay, enough of this. You get the idea and it's not terribly interesting
before I wrap my head around the structure of what you are trying to do.
That will have to wait for my brain to wrap itself around Martin's code
which will have to wait for ...
All these are valuable suggestions, but as I said above, you might
want to subscribe as a developer to the project and send suggestions
there, if you want we can give you commit privileges
so that we can incorporate them.
Ciao,
Simone.
Looking forward to being more useful once I sort all this out this
spring.
cheers,
adrian
On Fri, 2008-02-08 at 19:44 +0100, Simone Giannecchini wrote:
> On Feb 8, 2008 6:06 PM, Adrian Custer <acuster@anonymised.com> wrote:
> > Hey Daniele,
> >
> > Sounds like lots of cool work.
> >
> > To propose a new module, you will have to work against the wiki and add
> > a new module page...see the developer's guide for the details, I expect.
> > Of course you will have to sacrifice a small animal to the confluence
> > gods merely to get to read that document.
> >
>
> Sounds good...
>
>
> >
> > Also, before you start this work, could you flesh out the details of the
> > dependency? I understand a bit of the attraction but little of the
> > consequences. Why another external dependency for a new format? How does
> > this compare to the other dependencies in the coverageio module? What
> > are the licensing issues? Why GDAL for two formats? How does this work
> > for gnu/mac/ms; how do users get the required dependencies? what's the
> > long term vision? How does this affect the build and the release of 2.5
> > and beyond? Others have been really concerned with external dependencies
> > even when they were in the Java VM itself, so I expect we will all want
> > to digest what depending on GDAL or GDAL bindings or whatever, means.
> >
> > Also, what does it mean that you will have other code that is not
> > modified. Are we adding two new modules to do the same thing than two
> > old modules? Why leave a mess rather than cleanup what is there? When
> > will the old modules be deprecated/removed? Are the old modules an
> > earlier attempt of yours or is that someone else's work? Either way, why
> > didn't they work? I'd really like to keep geotools clean so we don't end
> > up with a gazillion unsupported modules.
>
>
> I hope you don't mind if I enthusiastically provide a unified answer
> to all these interesting questions 
> We have dedicated part of our time during the last year and a half to
> put together this imageio-ext (https://imageio-ext.dev.java.net/)
> experiment which would like to become the home for extensions to
> imageio as well as experiment around it.
> Between the other things a lot of work has been done to for trying to
> adapt gdal to the imageio model in order to be able to leverage on the
> N where N > 40 ( I lost the count, check here
> http://www.gdal.org/formats_list.html if interested)
> raster formats that gdal can already read.
> A few months ago we started to put some plugins based on this work
> inside unsupported since there are some people already using for udig
> as well as for geoserver. These plugins where ECW and MRSID (I am sure
> you can find details on the internet for these formats if you need
> them). Now that the work on imageio-ext has reached a decent stage
> daniele is spending a bit of time on creating a simple structure to
> flesh out a structure that would reduce code duplication and which
> would reduce the dependency on the AbstractGridCoverage2D class which
> sooner or later will be replaced. When this will be ready the two old
> plugins will be removed and the new ones should be gradually moved to
> trunk.
> We are concerned as well about the native lib dependencies but for
> some formats there is no other option available. For the moment we
> support them on linux and windows but we have just bought a mac and
> we'll start playing with it soon. The goal is to build a unit tests
> that don't bother if the libs are not avalaible but also put libs
> where they can be downloaded. But this discussion is still a bit
> premature, when things will be in place we'll decide if we want to use
> these plugins as extensions to minimize the impact of the native libs
> or as real plugins (i.e. under the plugins directory). For the moment
> our goal is to have what daniele is doing in a place where people can
> have a look and possible contribute with suggestions/ideas/fixes
> etc... since we believe that collaboration is something different from
> "this is what I've done you can use it if you want".
>
>
> Regards,
> Simone.
>
> >
> > Thanks,
> > --Adrian
> >
> > On Fri, 2008-02-08 at 16:35 +0100, Daniele Romagnoli wrote:
> > > Hi list,
> > > I would like to add a new gt2-imageio-ext unsupported module on
> > > geotools to improve coverage access capabilities based on GDAL.
> > >
> > > Some notes to introduce the aim of this module:
> > > - Actually, we (GeoSolutions) have improved the Image I/O project
> > > which mainly provides raster data access capabilities using GDAL java
> > > bindings.
> > > - The unsupported section already contains a MrSID and an ECW modules
> > > which leverage on such a library to provide coverage access to these
> > > formats. However, both modules contains a lot of shared/duplicated
> > > code.
> > > - The aim of the proposed module would be defining a "generic"
> > > gdalbased reader which extends the AbstractGridCoverage2DReader,
> > > containing all the common code. Moreover, initially, it will contain a
> > > set of format specific plugin (ECW/MrSID) which leverage on the
> > > "generic" reader. The specific plugins simply need to define some
> > > format specific properties to be used by the generic one. Moreover
> > > they will contain additional code for metadata management when
> > > available.
> > > - During an initial incubation/developing/testing phase, the actual
> > > standalone MrSID and ECW modules on the actual unsupported zone, will
> > > not be modified.
> > >
> > > Can I obtain the "GO" to start with this work on unsupported?
> > >
> > > Best Regards,
> > > Daniele Romagnoli
> > >
> > > PS: let me know if I need to specify further details or additional
> > > info. I apologize in case I forgotten something 
> > >
> > > --
> > > -------------------------------------------------------
> > > Eng. Daniele Romagnoli
> > > Software Engineer
> > >
> > > GeoSolutions S.A.S.
> > > Via Carignoni 51
> > > 55041 Camaiore (LU)
> > > Italy
> > >
> > > phone: +39 0584983027
> > > fax: +39 0584983027
> > > mob: +39 328 0559267
> > >
> > >
> > > http://www.geo-solutions.it
> > >
> > > -------------------------------------------------------
> > > -------------------------------------------------------------------------
> > > This SF.net email is sponsored by: Microsoft
> > > Defy all challenges. Microsoft(R) Visual Studio 2008.
> > > http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
> > > _______________________________________________ Geotools-devel mailing list Geotools-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/geotools-devel
> >
> >
> > -------------------------------------------------------------------------
> > This SF.net email is sponsored by: Microsoft
> > Defy all challenges. Microsoft(R) Visual Studio 2008.
> > http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
> > _______________________________________________
> > Geotools-devel mailing list
> > Geotools-devel@lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/geotools-devel
> >
>
>
>
--
-------------------------------------------------------
Eng. Simone Giannecchini
President /CEO GeoSolutions S.A.S.
Via Carignoni 51
55041 Camaiore (LU)
Italy
phone: +39 0584983027
fax: +39 0584983027
mob: +39 333 8128928
http://www.geo-solutions.it
-------------------------------------------------------