[mapguide-internals] rfc60 ready for review
UV
uvwild at googlemail.com
Mon Dec 21 16:07:05 EST 2009
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 at lists.osgeo.org [mailto:mapguide-internals-bounces at 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 at lists.osgeo.org [mailto:mapguide-internals-bounces at 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 at lists.osgeo.org [mailto:mapguide-internals-bounces at 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 at lists.osgeo.org
>> http://lists.osgeo.org/mailman/listinfo/mapguide-internals
>> _______________________________________________
>> mapguide-internals mailing list
>> mapguide-internals at lists.osgeo.org
>> http://lists.osgeo.org/mailman/listinfo/mapguide-internals
>>
>>
>>
>
> _______________________________________________
> mapguide-internals mailing list
> mapguide-internals at lists.osgeo.org
> http://lists.osgeo.org/mailman/listinfo/mapguide-internals
> _______________________________________________
> mapguide-internals mailing list
> mapguide-internals at lists.osgeo.org
> http://lists.osgeo.org/mailman/listinfo/mapguide-internals
>
>
> ------------------------------------------------------------------------
>
> _______________________________________________
> mapguide-internals mailing list
> mapguide-internals at lists.osgeo.org
> http://lists.osgeo.org/mailman/listinfo/mapguide-internals
>
More information about the mapguide-internals
mailing list