[mapguide-internals] RFC 60 Status

Tue Jul 28 11:55:48 EDT 2009

Hi Zac,

I believe that my question about the implementation of RFC 60 from May 12 was not answered yet.

After all the criticism of our code by UV, it's only fair that my single specific concern about what I believe to be sub-par code of his to be answered.

I am pasting the relevant thread here:
====================================================================================
====================================================================================
Hi UV,

I was specifically referring to the RSCOLORLIST in the rendering service. It's not a list of strings. It's a list of parsed colors. Specifically this section of code:

       945         PRSCOLORLIST tileColorPalette = new RSCOLORLIST(); 
 	946	        /// the map object has a list from all color entries found in the most recent Layerstylization 
 	947	    /// TODO currently they are interpreted as ffffffff 32 bit RGBA string values. 
 	948	    /// adding expresssions and other interpretations could be done here.  
 	949	        try { 
 	950	        ParseColorStrings (tileColorPalette, map); 
 	951	                #ifdef _DEBUG_PNG8 
 	952	                printf("<<<<<<<<<<<<<<<<<<<<< MgServerRenderingService::ColorPalette->size(): %d\n", mapColorPalette->size()); 
 	953	                #endif 
 	954	                //  call the image renderer to create the image ---------------------------------------- 
 	955	                if (wcscmp(m_rendererName.c_str(), L"AGG") == 0) 
 	956	                        data.reset(((AGGRenderer*)dr)->Save(format, saveWidth, saveHeight, tileColorPalette)); 
 	957	                else 
 	958	                        data.reset(((GDRenderer*)dr)->Save(format, saveWidth, saveHeight)); 
 	959	        } catch (exception e) { 
 	960	        if (tileColorPalette)    // cleanup the bare pointer (its a STL list so we do it by hand 
 	961	        { 
 	962	            RSCOLORLIST::iterator it; 
 	963	            for (it = tileColorPalette->begin();it != tileColorPalette->end(); it++)  
 	964	                delete *it; // the RS_Color is on the heap and needs destruction 
 	965	            delete tileColorPalette; 
 	966	        } 
 	967	        ACE_DEBUG((LM_DEBUG, L"(%t) %w caught in RenderingService ColorPaletteGeneration\n", e.what())); 
 	968	                throw e; 
 	969	        }

Thanks,
Traian

-----Original Message-----
From: mapguide-internals-bounces at lists.osgeo.org [mailto:mapguide-internals-bounces at lists.osgeo.org] On Behalf Of UV
Sent: Saturday, May 09, 2009 9:25 AM
To: MapGuide Internals Mail List
Subject: Re: [mapguide-internals] Some code fixes for RFC 60

Hi Traian,

thanks for the comments.

*TYPEDEF
The allcaps typedef followed a coding style I was used to and saw in the Oem code base to differentiate the TYPEDEFS from the real type names. 
Sorry for this.  I did not see typedefs in the Mapguide code so I missed that bit.

std::list<const MdfString>
You are right that the stringlist is not an efficient way of storing this generally.

*Why Strings?
The reason for using the strings is that if the values in the XML are not RRGGBBAA hex the color collecting code would still work.
Only the rendering service needs to reinterpret the stringlist.
This is implementing separation of concerns between the collaborating parts of the code. Since requests for extending this code have been stated already this seemes like the way to simplify the implementation and being able to use a simple adapter design to reinterpret the XML strings found in the map.

*Why List?
I was originally considering the vector as opposed to the list container but the beauty of the sort and unique methods made me use the list.
My reasoning is that I can use the sort/unique functionality of the std::list at the cost of less efficient storage of some small data structures.

*Extra Processing*
My assessment of the extra cost for the additional processing time once per tile was that it was not relevant in comparison to the complexity of the mapping algorithms. So if we are looking at an roughly estimated ratio of 1000: 1 computations the  cleanness of the code would justify this approach.
If this assessment is wrong then this needs changing..... however, the costly sort / unique algorithms implemented with a vector might chew up the savings made by choosing the vector as a container.

Thanks for pointing those bits out to me as this reasoning should have been written in the code where its used or at least in the RFC.
My fault.

Traian Stanev wrote:
> I am looking at the RSCOLORLIST structure in the RenderingService part of the RFC (v8 of the patch file) and I have some comments.
>
> Using pointers allocated with new to point to something as small as a color is not "more flexible". It is:
>
> * A lot slower. Probably at least 10x slower, due to all the calls to new().
> * Uses a lot more memory. A color can be represented in 4 bytes, using RS_Color::argb(). You are using 4 bytes for the pointer, 16 bytes for the color, plus the malloc() overhead per color, which is at least 8 more bytes. 
> * A lot more complicated and leak-prone, in that it requires you to write more code to free the colors when you are done with them.
>
> Here is what you could have done:
>
> std::vector<unsigned> colorList;
>
> This way:
>
> * You don't need to allocate the list using new(). 
> * You don't need to allocate list members using new(). 
> * You don't have to worry about catching exceptions in order to delete the list pointer you allocated using new(), since the destructor of the vector will automatically get called when it goes out of scope.
> * You use far less memory and your code is a lot simpler and a lot faster.
> * Since the type is not typedef-ed by an obscure name, another programmer would know immediately what data structure he is dealing with (a vector of integers).
>
> Also, you did not follow the style of the rest of the code by declaring all-caps data types, in defiance of pretty much every single type in the rest of the project...
>
> Thanks,
> Traian
>
>
> -----Original Message-----
> From: mapguide-internals-bounces at lists.osgeo.org [mailto:mapguide-internals-bounces at lists.osgeo.org] On Behalf Of UV
> Sent: Friday, May 08, 2009 8:29 AM
> To: MapGuide Internals Mail List
> Subject: Re: [mapguide-internals] Some code fixes for RFC 60
>
> Thanks for the input. Much appreciated.
>
> Added the delete.
>
> Why did I use pointers?
> I did use pointers as this leaves more flexibility for later improvements.
> Also In the beginning I was not sure if the map object might be shared 
> between threads or sessions.
> I can imagine some use cases where this might be an issue.... at that 
> stage different colorpalettes need to be handled and pointers would be 
> more flexible.
>
> To notice an eventual leak in the assignment I added the assert 
> statement....
> so its an odd coding but a destructive assignment does not leak it 
> breaks ;) 
> This is like a runtime check to understand whats happening in a 
> multithreaded server.
> So its the same  for now.
>
> I added those things now.... the Coordsys error message (#980) is also 
> included.
> I  commented out the experimental exception throw in StylizeLayers...
>
> Cheers,
> UV
>   
_______________________________________________
mapguide-internals mailing list
mapguide-internals at lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/mapguide-internals

====================================================================================
====================================================================================
====================================================================================

> -----Original Message-----
> From: mapguide-internals-bounces at lists.osgeo.org [mailto:mapguide-internals-
> bounces at lists.osgeo.org] On Behalf Of Zac Spitzer
> Sent: Tuesday, July 28, 2009 2:22 AM
> To: MapGuide Internals Mail List
> Subject: [mapguide-internals] RFC 60 Status
> 
> I'm really happy with the state of RFC 60 as it stands at the moment,
> maps look much cleaner and nicer.
> 
> As a question for the PSC, currently the fix only covers static colors
> within
> a map definition, which fixes all the problems I was experiencing.
> 
> It seems that it's ok for Autodesk staff to do partial implementations
> of functionality within a RFC, ie create feature source only doing files
> for now, citing financial limits. which is a completely valid reason.
> 
> I have hit the same issue with rfc60 and the expression stuff.  I have no
> money left to implement support for them and no need from my clients
> for it.
> 
> As i see it, like the autodesk RFC's, the project is better off with
> this support in place as it stands than without it.
> 
> If someone really needs that support they can add it,
> 
> correct me if I'm wrong, but not having expression support for the
> palette stuff means that colors expressions are just rendered
> the same as before, just with a slightly smaller palette to dither
> with
> 
> z
> 
> 
> --
> Zac Spitzer -
> http://zacster.blogspot.com
> +61 405 847 168
> _______________________________________________
> mapguide-internals mailing list
> mapguide-internals at lists.osgeo.org
> http://lists.osgeo.org/mailman/listinfo/mapguide-internals