GeoTools
  1. GeoTools
  2. GEOT-3565

Use MapContent in GTRenderer and StreamingRenderer

    Details

    • Type: Improvement Improvement
    • Status: Resolved Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 8.0-M1
    • Component/s: render
    • Labels:
      None

      Description

      MapContent is advertised in the docs as the class to use in preference to MapContext for new development. The attached patch does the following:

      • adds setMapContent and getMapContent methods to GTRenderer and StreamingRenderer
      • replaces the MapContext instance in StreamingRenderer with a MapContent instance
      • deprecates setContext and getContext methods
      1. combined_mega_patch.patch
        67 kB
        Andrea Aime
      2. combined_mega_patch.patch
        88 kB
        Jody Garnett
      3. combined-renderer-mapcontent.patch
        16 kB
        Michael Bedward
      4. GEOT-3565-aaime.patch
        21 kB
        Andrea Aime
      5. GEOT-3565-context-fix.patch
        62 kB
        Andrea Aime
      6. mega_patch.patch
        34 kB
        Jody Garnett
      7. renderer-mapcontent.patch
        14 kB
        Michael Bedward
      8. renderer-mapcontent2.patch
        14 kB
        Michael Bedward
      9. shapefile-renderer-mapcontent.patch
        2 kB
        Michael Bedward

        Issue Links

          Activity

          Hide
          Andrea Aime added a comment -
          So this is an API change. It will break all existing implementations of GTRenderer, in particular afaik the patch does not care for the shapefile renderer.
          I also see that the patch removes some c coding style bits with more java like ones (David B. used to code in java as it that was C in fact), but I'm unsure if all of them are actually equivalent. See this bit:

          {code}
                     // skip layers that do have only one fts
          - if(layer.getStyle().getFeatureTypeStyles().length < 2)
          + if (!(layer instanceof FeatureLayer)) {
                           continue;
          + }
           
          + FeatureLayer featureLayer = (FeatureLayer) layer;
          +
          + if (featureLayer.getStyle().featureTypeStyles().size() < 2) continue;
          +
          {code}

          Maybe it is equivalent but after 10 hours of work I'm too tired to tell :-p
          Allow me some more time to look into the patch. Maybe next time split it into two, code cleanups in one patch, api changes in the other.

          Show
          Andrea Aime added a comment - So this is an API change. It will break all existing implementations of GTRenderer, in particular afaik the patch does not care for the shapefile renderer. I also see that the patch removes some c coding style bits with more java like ones (David B. used to code in java as it that was C in fact), but I'm unsure if all of them are actually equivalent. See this bit: {code}            // skip layers that do have only one fts - if(layer.getStyle().getFeatureTypeStyles().length < 2) + if (!(layer instanceof FeatureLayer)) {                  continue; + }   + FeatureLayer featureLayer = (FeatureLayer) layer; + + if (featureLayer.getStyle().featureTypeStyles().size() < 2) continue; + {code} Maybe it is equivalent but after 10 hours of work I'm too tired to tell :-p Allow me some more time to look into the patch. Maybe next time split it into two, code cleanups in one patch, api changes in the other.
          Hide
          Michael Bedward added a comment - - edited
          My apologies - ShapefileRenderer is in unsupported and I had the idea, probably wrongly, that it was being phased out. Attached is a patch which adds setMapContent and getMapContent methods.
          Show
          Michael Bedward added a comment - - edited My apologies - ShapefileRenderer is in unsupported and I had the idea, probably wrongly, that it was being phased out. Attached is a patch which adds setMapContent and getMapContent methods.
          Hide
          Andrea Aime added a comment -
          The idea that it's being phased out is correct, it's however still there until we remove it for good.
          GeoServer still has an option to enable it (though it's not used by default anymore), not sure if uDig got rid of it for good. AtlasStyler is also still using it afaik.

          I certainly want to kill it dead in the 8.x series, just being cautious about how I move since there is people out there using it.
          Show
          Andrea Aime added a comment - The idea that it's being phased out is correct, it's however still there until we remove it for good. GeoServer still has an option to enable it (though it's not used by default anymore), not sure if uDig got rid of it for good. AtlasStyler is also still using it afaik. I certainly want to kill it dead in the 8.x series, just being cautious about how I move since there is people out there using it.
          Hide
          Andrea Aime added a comment -
          Just sent a mail to geotools-users that the shapefile renderer is about to get killed. Let's see if there is any reaction :-)
          Show
          Andrea Aime added a comment - Just sent a mail to geotools-users that the shapefile renderer is about to get killed. Let's see if there is any reaction :-)
          Hide
          Andrea Aime added a comment -
          Hi Michael, looked at the patch more in detail, everything seems to be in order. +1 on committing, thought I'd also have an eye on the ongoing discussion in the devel list about the topic of doing the next step (switching the renderer to use the new layer classes and so on)
          Show
          Andrea Aime added a comment - Hi Michael, looked at the patch more in detail, everything seems to be in order. +1 on committing, thought I'd also have an eye on the ongoing discussion in the devel list about the topic of doing the next step (switching the renderer to use the new layer classes and so on)
          Hide
          Michael Bedward added a comment -
          Thanks very much Andrea.

          I haven't committed anything yet because, when double checking the patch, I thought the "instanceof FeatureLayer" check which it puts into StreamingRenderer.getMaxBackBufferMemory is a stupid idea because it ignores grid coverage / reader layers. To fix this I have used the new StyleLayer class in the instanceof check, but then have to resort to creating a MapLayer instance to act as a helper for the next step (checking features to decide if the style is active).

          I've attached an updated patch for you and Jody to consider. I want to make sure that these changes are not just creating more work in the long run, ie. whether it would be better to hold off until MapContent etc is stable and then do StreamingRenderer in one go.
          Show
          Michael Bedward added a comment - Thanks very much Andrea. I haven't committed anything yet because, when double checking the patch, I thought the "instanceof FeatureLayer" check which it puts into StreamingRenderer.getMaxBackBufferMemory is a stupid idea because it ignores grid coverage / reader layers. To fix this I have used the new StyleLayer class in the instanceof check, but then have to resort to creating a MapLayer instance to act as a helper for the next step (checking features to decide if the style is active). I've attached an updated patch for you and Jody to consider. I want to make sure that these changes are not just creating more work in the long run, ie. whether it would be better to hold off until MapContent etc is stable and then do StreamingRenderer in one go.
          Hide
          Andrea Aime added a comment -
          The "stupid idea" made GS WMS much more resistant to large request attacks... maybe there was something good in it, people wanted the same for WCS and won't use WPS until we also get something there:
          http://docs.geoserver.org/latest/en/user/services/wms/configuration.html#request-limits

          Readers are not taken into consideration because a good one does use very little memory, and all of it in the tile cache, but I admit have forgotten about layers based on a in memory coverage.
          I0ll have a look at the patch soon (hopefuly)
          Show
          Andrea Aime added a comment - The "stupid idea" made GS WMS much more resistant to large request attacks... maybe there was something good in it, people wanted the same for WCS and won't use WPS until we also get something there: http://docs.geoserver.org/latest/en/user/services/wms/configuration.html#request-limits Readers are not taken into consideration because a good one does use very little memory, and all of it in the tile cache, but I admit have forgotten about layers based on a in memory coverage. I0ll have a look at the patch soon (hopefuly)
          Hide
          Michael Bedward added a comment -
          Thanks Andrea. The stupid idea reference was aimed at my code because I was worried it could be creating a gap for grid coverages to fall through.

          I've plenty to do with other parts of the swing module so no rush.
          Show
          Michael Bedward added a comment - Thanks Andrea. The stupid idea reference was aimed at my code because I was worried it could be creating a gap for grid coverages to fall through. I've plenty to do with other parts of the swing module so no rush.
          Hide
          Michael Bedward added a comment -
          New patch - combined-renderer-mapcontent.patch

          No additional changes in this patch - it just freshens the previous patch to take account of the recent work on label obstacles.

          Andrea, as per the previous comments, this patch just differs from the original one in the getMaxBackBufferMemory method which now queries a StyleLayer to get the number of feature types being used. I know that you're super busy, so if you can't review this for some time I'll create a branch to get moving with the swing module work.
          Show
          Michael Bedward added a comment - New patch - combined-renderer-mapcontent.patch No additional changes in this patch - it just freshens the previous patch to take account of the recent work on label obstacles. Andrea, as per the previous comments, this patch just differs from the original one in the getMaxBackBufferMemory method which now queries a StyleLayer to get the number of feature types being used. I know that you're super busy, so if you can't review this for some time I'll create a branch to get moving with the swing module work.
          Hide
          Andrea Aime added a comment -
          New new patch :-p
          Deprecates MapLayer too (since once switched to MapContext there is no way to use it anyways) and the two subclasses of MapContext.
          Also fixes FeatureLayer.getQuery() to return Query.ALL in case of no query, just like the javadoc said (but the implementation didn't)
          Show
          Andrea Aime added a comment - New new patch :-p Deprecates MapLayer too (since once switched to MapContext there is no way to use it anyways) and the two subclasses of MapContext. Also fixes FeatureLayer.getQuery() to return Query.ALL in case of no query, just like the javadoc said (but the implementation didn't)
          Hide
          Jody Garnett added a comment -
          mega patch resolves a couple of the sub issues:

          - GEOT-3628 Handling of getBounds() and AreaOfInterest()
          - GEOT-3658 Layer to support methods expected by Renderers

          It is provided as a single patch for aaime to review. Next step is to update mbedward's patch to renderer and aaime's patch to geoserver.
          Show
          Jody Garnett added a comment - mega patch resolves a couple of the sub issues: - GEOT-3628 Handling of getBounds() and AreaOfInterest() - GEOT-3658 Layer to support methods expected by Renderers It is provided as a single patch for aaime to review. Next step is to update mbedward's patch to renderer and aaime's patch to geoserver.
          Hide
          Jody Garnett added a comment -
          Okay testing a super combined mega patch now (gathering up the patches to Layer hierarchy; and andrea's patch to StreamingRenderer and ShapefileRenderer).

          It went mostly smoothly; the classes StyleLayer and RasterLayer no longer really hold their weight. I went through and removed all references in the code to MapLayer and for the most part it was fine.

          There were two sections worthy of comment:

          1) Code supporting ContentSource was commented out; this was for an intern experiment at refractions, where they made a ContentSource backed by some kind of OR mapper. While I like the idea of the rendering system accepting POJOs life is too short ... besides it would be better handled by an explicit CollectionLayer.
          2) In a similar fashion the code to determine the CRS for a layer had some special hacks for when a ContentSource was available

          But yeah all the internal methods and data structures accepted the revised Layer as a drop in replacement for MapLayer - win!
          Show
          Jody Garnett added a comment - Okay testing a super combined mega patch now (gathering up the patches to Layer hierarchy; and andrea's patch to StreamingRenderer and ShapefileRenderer). It went mostly smoothly; the classes StyleLayer and RasterLayer no longer really hold their weight. I went through and removed all references in the code to MapLayer and for the most part it was fine. There were two sections worthy of comment: 1) Code supporting ContentSource was commented out; this was for an intern experiment at refractions, where they made a ContentSource backed by some kind of OR mapper. While I like the idea of the rendering system accepting POJOs life is too short ... besides it would be better handled by an explicit CollectionLayer. 2) In a similar fashion the code to determine the CRS for a layer had some special hacks for when a ContentSource was available But yeah all the internal methods and data structures accepted the revised Layer as a drop in replacement for MapLayer - win!
          Hide
          Jody Garnett added a comment -
          This patch rolls together all the others; however svg tests in geoserver are now failing
          Show
          Jody Garnett added a comment - This patch rolls together all the others; however svg tests in geoserver are now failing
          Hide
          Andrea Aime added a comment -
          Hi Jody, did you also prepare an equivalent patch for GeoServer?
          To evaluate the updated set of classes for good the only realistic way is to switch something that uses them heavily, you have seen the amount of chaos the previous set of classes caused.
          Btw, the patch you attached has a ton of commented out blocks and a number of unrelated changes that are probably due to encoding issues, those need cleaning up.
          Show
          Andrea Aime added a comment - Hi Jody, did you also prepare an equivalent patch for GeoServer? To evaluate the updated set of classes for good the only realistic way is to switch something that uses them heavily, you have seen the amount of chaos the previous set of classes caused. Btw, the patch you attached has a ton of commented out blocks and a number of unrelated changes that are probably due to encoding issues, those need cleaning up.
          Hide
          Andrea Aime added a comment -
          Btw, I can re-try doing the geoserver patch, just want to make sure I'm not throwing away a day duplicating work you might have already done
          Show
          Andrea Aime added a comment - Btw, I can re-try doing the geoserver patch, just want to make sure I'm not throwing away a day duplicating work you might have already done
          Hide
          Andrea Aime added a comment -
          Replacing the combined mega patch with a cleaned up one
          Show
          Andrea Aime added a comment - Replacing the combined mega patch with a cleaned up one
          Hide
          Andrea Aime added a comment -
          This one fixes a mistake in the previous patches: if one sets a map context using setContext(), getContext() must return the same object, there is no guarantee it's goind go be a plain MapContext(), in the case of GEoServer it's a subclass with extra infos
          Show
          Andrea Aime added a comment - This one fixes a mistake in the previous patches: if one sets a map context using setContext(), getContext() must return the same object, there is no guarantee it's goind go be a plain MapContext(), in the case of GEoServer it's a subclass with extra infos
          Hide
          Andrea Aime added a comment -
          Patch finally applied, whew
          Show
          Andrea Aime added a comment - Patch finally applied, whew

            People

            • Assignee:
              Andrea Aime
              Reporter:
              Michael Bedward
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: