[mapguide-internals] RFC 60 Status
Traian Stanev
traian.stanev at autodesk.com
Wed Jul 29 10:05:33 EDT 2009
Hi Zac,
OK thanks. I have no other objections.
Traian
> -----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 10:44 PM
> To: MapGuide Internals Mail List
> Subject: Re: [mapguide-internals] RFC 60 Status
>
> Hi Traian,
>
> the stylistic coding stuff that has been taken on board and will be
> addressed,
> I was only referring to the additional expression stuff
>
> z
>
>
> On Wed, Jul 29, 2009 at 1:55 AM, Traian
> Stanev<traian.stanev at autodesk.com> wrote:
> > 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
> > _______________________________________________
> > mapguide-internals mailing list
> > mapguide-internals at lists.osgeo.org
> > http://lists.osgeo.org/mailman/listinfo/mapguide-internals
> >
>
>
>
> --
> 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
More information about the mapguide-internals
mailing list