[mapguide-internals] RFC60 finalization

Mon Oct 26 14:13:30 EDT 2009

BTW, UV, please understand that my feedback is just part of the normal review process.  Your submission will be fairly big and touches some sensitive areas of the code, so you should expect this level of feedback.

For now I think just doing the refactoring would be sufficient so we can move towards the final / formal code review.  Enhanced stylization support can be added separately.

Walt

-----Original Message-----
From: mapguide-internals-bounces at lists.osgeo.org [mailto:mapguide-internals-bounces at lists.osgeo.org] On Behalf Of Walt Welton-Lair
Sent: Friday, October 23, 2009 12:32 PM
To: MapGuide Internals Mail List
Subject: RE: [mapguide-internals] RFC60 finalization

Yes, my main concern is not with attribute-based stylization.  I want the current code to be updated so that it can properly accommodate enhanced stylization going forward.

The refactoring work:
* Move changes that were done in MdfModel to collect colors into a method in MgMappingUtil.  For now that method will only collect colors for the legacy stylization.

To add support for enhanced stylization (inlined symbols):
* add code to the above method which iterates over the symbol definition object model and collects colors
* not a huge task - roughly 1-2 days work is my estimate.

To add support for enhanced stylization (referenced symbols):
* Assuming inlined symbol support is present, update the method to make use of the SEMgSymbolManager class (in MappingService) to load referenced symbols.  Once loaded, pass the symbol to the code which extracts colors for a symbol definition (added during the previous task).
* relatively straightforward task - 1 day is sufficient

BTW, even if we completely ignore enhanced stylization the current RFC 60 implementation has the same problem when it comes to point layers styled using a symbol library.  The layer definition references a DWF symbol library resource and the name of the symbol to use.  The current RFC 60 code will be unable to load the DWF symbol and take into account any colors defined in the DWF symbol.  But after the refactoring, this would now be possible.  Similar to the enhanced stylization, the color collecting code could be updated to use of the RSMgSymbolManager class to load referenced DWF symbols.  And of course there would need to be a method to parse a DWF symbol and collect its colors.

For both of these, if we do the refactoring then the color collecting code is ready for future enhancements to support missing cases like the above.

Walt

-----Original Message-----
From: mapguide-internals-bounces at lists.osgeo.org [mailto:mapguide-internals-bounces at lists.osgeo.org] On Behalf Of Tom Fukushima
Sent: Friday, October 23, 2009 11:56 AM
To: MapGuide Internals Mail List
Subject: RE: [mapguide-internals] RFC60 finalization

I'm trying to understand this...so correct me if I'm wrong.  But it seems that the problem is not with attribute-based stylization.  It's that if someone uses referenced symbols (that is, the LayerDefinition specifies a resource using a resource ID such as Library://mysymbols/a.SymbolDefinition instead of embedded XML for a SymbolDefinition in the LayerDefinition) then a large part of the code will need to be rewritten to support it.

If the above is the case, I understand that we need to make tradeoffs all the time, and this may be where we need to make one.  I think though that it would be advantageous for the current developer to do the requested refactoring now while it is fresh.  Later, if defects are found or refinements (e.g., attribute-based stylization) are done, they could be addressed at that time---and this would be manageable because wouldn't require refactoring large parts of the code.

Could we get some idea of the work that would be required to get support for the referenced symbol definitions?

Note that I'm not saying it has to be done (but of course I would prefer it to be done), I'm trying to get more information on this sticking point (I believe it is the only one right?) to try to make a judgment.

Tom

-----Original Message-----
From: mapguide-internals-bounces at lists.osgeo.org [mailto:mapguide-internals-bounces at lists.osgeo.org] On Behalf Of Jason Birch
Sent: Friday, October 23, 2009 9:24 AM
To: MapGuide Internals Mail List
Subject: Re: [mapguide-internals] RFC60 finalization

It seems to me that we have frequently (too often perhaps?) favoured
expediency over correctness when faced with resourcing or timeline
issues.

In this case, I'm in favour of placing the burden of change on the
whoever deems that it is critical to support pallette reservation for
attribute-based stylization.

Jason

On 2009-10-23, Walt Welton-Lair <walt.welton-lair at autodesk.com> wrote:
>> Secondly, it think it is wrong to request the implementation of the new
>> stylization for such transparent feature addition.
>> RFC60 works fine for our case, why should we implement something that
>> does not harm anything but what we do not need?
>
> The way your code is designed - where it does the work of grabbing the
> colors inside the MdfModel project - WILL NOT WORK for enhanced stylization
> in which there are referenced symbol definitions.  It's not possible to load
> referenced symbols from the resource service from inside the MdfModel code.
> This is not a minor defect.  It requires that a lot of your code be
> refactored.  I already explained the problem last March.  Here's the
> excerpt:
>
> "With the new enhanced stylization (RFC 14) the layer definition can
> reference symbol definitions (in addition to inlining them).  The symbols,
> of course, define colors which need to be accounted for.  Accessing
> referenced symbol definitions requires the resource service - we need to get
> the symbol definition resource from the service.  The MdfModel project
> (where VectorLayerDefinition is stored) does not have access (nor want
> access) to the resource service.  So if you want to properly support the new
> enhanced stylization with your RFC (you should) then we'll probably have to
> move this ComputeUsedColors method somewhere else.  There's a method -
> MgMappingUtil::ComputeStylizationOffset - which does something analogous to
> ComputeUsedColors, so possibly we can add ComputeUsedColors to
> MgMappingUtil."
>
> So even if you don't add code to actually parse colors for enhanced style
> layers, you need to keep the bigger picture in mind and design your new code
> so it will be easy to add this support.
>
> All along that's the real reason why I wanted you to take a look at this.
> So that you would come to realize that your current design doesn't fully
> work as is.
>
>
>> Open source is more like: Who needs it should code it!
>
> True, but it's not an excuse for not doing the right thing.  People can't
> just add anything they want without taking account the bigger picture.
> That's my take on this.
>
>
> Walt
>
> -----Original Message-----
> From: mapguide-internals-bounces at lists.osgeo.org
> [mailto:mapguide-internals-bounces at lists.osgeo.org] On Behalf Of UV
> Sent: Friday, October 23, 2009 9:03 AM
> To: MapGuide Internals Mail List
> Subject: [mapguide-internals] RFC60 finalization
>
> Hi all,
>
> I looked into the RFC60 code again and added some more features to it
> e.g. finding label colors.
>
> However, a redirection of referenced featureIds which would cause a FDO
> lookup seems prohibitive expensive in this context.
> I would not like to add this into the code simply as it could increase
> delay tile computation a lot.
> So the interesting test here are performance related. Simple unit-tests
> are not sufficient for this.
> Creating complex test data to test performance issues is tedious and
> very badly motivated!!! (specification vs. implementation from same hand)
>
> Secondly, it think it is wrong to request the implementation of the new
> stylization for such transparent feature addition.
> RFC60 works fine for our case, why should we implement something that
> does not harm anything but what we do not need?
> This is not thought along open source paradigms.  Open source is more
> like: Who needs it should code it!
> So we should leave this open and the next person that has real test data
> which doesn't work can post a defect!
> Very easy process!
>
>
> RFC60 is not trivial at all but it solves a real problem in real maps.
> To block the inclusion of RFC60 into the code base on those matters is
> simply wrong in open source terms as it keeps others from using a new
> working feature.
> _______________________________________________
> 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