[prev in list] [next in list] [prev in thread] [next in thread]
List: mapguide-internals
Subject: Re: [mapguide-internals] rfc60 ready for review
From: UV <uvwild () googlemail ! com>
Date: 2009-12-21 21:07:05
Message-ID: 4B2FE379.40709 () gmail ! com
[Download RAW message or body]
thanks for the review!
comments inline
Trevor Wekel wrote:
> Hi everyone,
>
> I finally found some time to review the latest incarnation of RFC60. I have tried \
> to be as thorough as possible but I would welcome additional reviews from others \
> like Walt, Traian, and Bruce if they have time.
> Overall this is an excellent and very involved first submission to the MapGuide \
> code base. I have made a number of comments in my review. UV, please do not take \
> this as a negative reflection on your code. You have touched a lot of areas with \
> this submission and I am very pleased with what you have accomplished here.
>
> General comments
> - Tab characters should be replaced by four spaces
> - Please ensure all code changes compile and run correctly on Linux
>
> Map.h
> - STRCOLORLIST should be camel case "StrColorList" to follow convention of typedefs \
> in Map.cpp
> - PSTRCOLORLIST should be removed. Typically we do not declare a second typedef \
> for the pointer of a typedef.
> - Can GetColorPalette() return an StrColorList&
> - AddColors2Palette() should be renamed AddColorsToPalette
> - AddColorsToPalette() should accept a StrColorList&
> - m_colorPalette should be an StrColorList. It will reduce object management, \
> initializers, etc
> Map.cpp
> - change m_colorPalette to an StrColorList.
>
A lazy instantiated container has been chosen here.
This considering the member is only used for some use cases namely the
PNG8/GIF tiles.
Creating the container when its not needed seems unnecessary. And the
DTOR code is already there.....
>
> RendingOptions.cpp
> - Ok
>
> UserInformation.cpp
> - Please revert all changes. They are only tab differences.
>
> MapGuideCommon.vcproj
> - Please revert all changes. AdditionalIncludeDirectories is the same path.
>
done
> AGGImageIO.cpp
> - AGGImageIO::ImageCopyForcePaletteGD should follow existing bracket convention in \
> file.
this has been intentionally copied from source as is. There is a
reference to the source and not reformatting simplifies later comparison.
> AGGImageIO.h
> - Is it possible to move the #include of gd.h and AGGRenderer.h back into the \
> source file?
added forward declarations to RendererStyles.h
> AGGRenderer.cpp
> - Can we use a line of all "/" for the separators?
>
Done. I think such convention should be added to coding style. I only
need separator lines. It doesnt matter which character to use :)
> AGGRenderer.h
> - typedef std::vector<RS_Color> RSCOLORS should be declared in RendererStyles.h
> - PRSCOLORS should be removed
>
thanks for the hint on proper location
> DefaultStylizer.cpp
> - Can we use a line of all "/" for the separators?
>
sure
> ServerManager.vcproj
> - Changes to include path will also need to be made in Makefile.am
>
I used the opportunity to order the includes in Makefile.am alphabetically
>
> PostBuild.mak
> - Has the location of the FDO binaries changed recently? Is this more in line with \
> the FDO SDK?
in RFC62 I tried to point out the discrepancies between FDO and MapGuide
build directories when I encountered them.
I believe those trees to be minimally inconsistent.
A build script in the repository which builds this on a buildserver
would be the best way to document how this is supposed to be
or otherwise the existing problems trying to do that with a script. I
think this needs coordination with the FDO repository.
This does not need to be merged now.
> MappingUtil.cpp
> - Include "Map.h" and drop StrColorList typedef
>
done
> MappingUtil.h
> - Can forward declarations be used instead of including SE_SymbolManager.h, \
> SymbolDefinition.h, CompoundSymbolDefinition.h?
done
> ServerRenderingService.cpp
> - Put back MG_TRY() / MG_CATCH() block in RenderMapInternal, otherwise dr->EndMap() \
> will not be called
done
> ServerRenderingService.h
> - ParseColorStrings method should be moved to StylizationUtil. It is strange \
> sitting in MgServerRenderingService.
I think this is cleaner typing.
RenderingService its taking strings from the XML MdfModel and converts
them into a RS_ColorVector to pass to renderer.
the option is another parameter to StylizeLayers or member to Map
objects to keep a handle on the converte RS_ColorVector.
> ServerTileService.cpp
> ServerTileService.h
> - It looks as though GetResourceServiceForMapDef is only used once. Is a new \
> method really necessary?
The original reason to do the method extraction has disappeared by now.
However, its a meaningful refactoring of some code functionality and as
such provides some way of documentation and improved code readability.
Why remove?
> Great work UV!
>
> Thanks,
> Trevor
>
>
>
> -----Original Message-----
> From: mapguide-internals-bounces@lists.osgeo.org \
> [mailto:mapguide-internals-bounces@lists.osgeo.org] On Behalf Of \
> Tom Fukushima
> Sent: December 11, 2009 10:45 AM
> To: MapGuide Internals Mail List
> Subject: RE: [mapguide-internals] rfc60 ready for review
>
> Trevor,
>
> Will you be reviewing any of this submission?
>
> Tom
>
> -----Original Message-----
> From: mapguide-internals-bounces@lists.osgeo.org \
> [mailto:mapguide-internals-bounces@lists.osgeo.org] On Behalf Of UV
> Sent: December-11-09 9:48 AM
> To: MapGuide Internals Mail List
> Subject: Re: [mapguide-internals] rfc60 ready for review
>
> Hi Tom,
>
> 1) We are using the patched 2.1 on our client website since may 2009 .
> So there are no more deadlines. However, right now the merging
> effort is the smallest as I already merged the branch with trunk.
>
> In this context a comment to the process:
> I still believe it to be wrong process for an open source project to
> have blocked this useful, transparent and working feature for 2.1 based
> on incompleteness of support of new stylization which
> a) nobody needed at that time which means no complex test cases
> to support development did exist
> b) might not even be so important as area styles are the only
> obvious visible features for color quantization errors
> (lets wait for a big map which is telling us otherwise)
> c) is not particular well documented/specified which in
> combination with missing complex test cases significantly increased the
> coding effort (color quantization occurs on a complexity level which is
> very difficult to reach with unit level test cases)
> d) has no funding and little motivation to support it because we
> never needed it ourselves.
>
> Therefore adapting the RFC60 as it was and later create tickets for
> it after somebody exposed something missing with his maps (this way
> creating useful test cases) would be a much more appropriate and
> productive process to deal with such a feature which has been partially
> funded by a client as an external contribution in an open source
> project. Keywords: completeness, funding, motivation & test cases
> I very much suggest to review your external contribution process
> again. At that time it was discouraging as the additional time spent
> dealing with extending the feature has artificially increased the effort
> to a multiple of the time solving our clients probem.
> Normally coding stops when the client problem is solved and nobody
> else pays for it.
> Dealing with external contributions like this might discourage
> people from contributing their patches to the shared code base, contrary
> to the open source idea. In this case we all were lucky because I had
> some spare time to invest.
> ------------------------------------------------------------------------------------------------
> 2) The revision merged from trunk is 4410 and can also be found in the
> MergeInfo property of http://svn.osgeo.org/mapguide/sandbox/rfc60/MgDev
> The current revision on the rfc60 branch is 4411 - no changes yet!
> ------------------------------------------------------------------------------------------------
> 3) In the Common code base I changed:
> Map: add a member containing the string
> color list
> RenderingOptions: move formatting of property to setter method
> (CTOR)
> AGGImageIO: added color quantization code
> AGGRenderer: pass the color palette through to AGGImageIo
> method
> DefaultStylizer: added accessor and member for Symbolmanager
>
> In the Server code base I changed:
> ServerManager.vcproj: adding a number of include dirs
> MappingUtil: add code to extract strings
> from XML color fields into the map color table
> ServerRenderingService: add parameter to control exection of
> color extraction code
> parse the string list to
> extract the colors (here any further expresssion evaluation can be
> easily added)
> (commented out: some
> code to let the complete tile fail when exception is caught in a layer
> which makes sense for
> tiling, see rfc64)
> ServerTileService: comments and method extraction to
> help understand whats going on
> (copy & paste of code
> seems quite popular in same places which does not help)
>
> This is all related to rfc60.
>
>
>
> Tom Fukushima wrote:
>
> > Hi UV,
> >
> > Since this is a large submission, please answer the following questions and \
> > requests for info, so we can figure out the logistics of getting the review done. \
> > 1) When do you need the review done by, do you have any pending hard deadlines? \
> > MGOS 2.2 doesn't have a cut off that I know of yet so if all you want is for this \
> > to go into 2.2 we do have some time.
> > 2) I noticed that you did a sync of your sandbox with trunk. I think that the \
> > reviewers will just diff the sandbox with trunk to see what the changes are. \
> > Which revision of trunk did you sync your sandbox with?
> > 3) You say below that there were other changes that are not related to RFC 60. \
> > There will be different reviewers for the RFC 60 part and the non-RFC 60 parts. \
> > Please specify the list of files that were changed but were not related to RFC \
> > 60.
> > Thanks
> > Tom
> >
> >
> >
> > -----Original Message-----
> > From: mapguide-internals-bounces@lists.osgeo.org \
> > [mailto:mapguide-internals-bounces@lists.osgeo.org] On Behalf Of \
> > UV
> > Sent: December-09-09 5:57 PM
> > To: MapGuide Internals Mail List
> > Subject: [mapguide-internals] rfc60 ready for review
> >
> > In the sandbox on
> > http://svn.osgeo.org/mapguide/sandbox/rfc60/MgDev
> > you can find a feature branch which is ready to be merged with trunk.
> > This codetree includes code to deal with new stylization and lookup of
> > symboldefinitions in addition to the color lookup in the previous
> > stylization.
> >
> > *in MappingUtil (ExtractColors, FindColorInSymDefHelper)
> > strings are collected from the xml definition of the scaleRanges
> > which are describing stylization colors.
> > *in ServerRenderingService (RenderMapInternal:777, ParseColorStrings)
> > after the Stylization the found strings are converted into colors
> > and passed on to the AGGRenderer
> > *in AGGImageIo (ImageCopyForcePaletteGD)
> > a provided colorpalette is used to optimize the colorquantization
> > algorithm for the tile
> >
> > this code is only executed for PNG8 and GIF tiles.
> >
> > I further added comments for some existing methods and did some method
> > extraction while trying to understand the code.
> >
> > The interesting question is now if the colors found in the new
> > stylization can overlap tile boundaries in a visible way and therefore
> > need to be included into the color quantization optimization?
> >
> > In our map the long edges of the ocean areas with slightly different
> > colors where the only ones we could spot,
> > whereas a slight color change of a line crossing a tile boundary is
> > quite hard to spot.
> >
> >
> > Other than that, this feature is a MUST for everyone using colormapped
> > tiles as otherwise such map simply looks ugly .
> > _______________________________________________
> > mapguide-internals mailing list
> > mapguide-internals@lists.osgeo.org
> > http://lists.osgeo.org/mailman/listinfo/mapguide-internals
> > _______________________________________________
> > mapguide-internals mailing list
> > mapguide-internals@lists.osgeo.org
> > http://lists.osgeo.org/mailman/listinfo/mapguide-internals
> >
> >
> >
>
> _______________________________________________
> mapguide-internals mailing list
> mapguide-internals@lists.osgeo.org
> http://lists.osgeo.org/mailman/listinfo/mapguide-internals
> _______________________________________________
> mapguide-internals mailing list
> mapguide-internals@lists.osgeo.org
> http://lists.osgeo.org/mailman/listinfo/mapguide-internals
>
>
> ------------------------------------------------------------------------
>
> _______________________________________________
> mapguide-internals mailing list
> mapguide-internals@lists.osgeo.org
> http://lists.osgeo.org/mailman/listinfo/mapguide-internals
>
_______________________________________________
mapguide-internals mailing list
mapguide-internals@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/mapguide-internals
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic