Use MapContent in GTRenderer and StreamingRenderer

Andrea Aime added a comment - 05/May/11 12:51 PM 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.

Michael Bedward added a comment - 05/May/11 9:39 PM - 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.

Andrea Aime added a comment - 06/May/11 1:58 AM 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.

Andrea Aime added a comment - 06/May/11 2:31 AM Just sent a mail to geotools-users that the shapefile renderer is about to get killed. Let's see if there is any reaction :-)

Andrea Aime added a comment - 07/May/11 9:25 AM 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)

Michael Bedward added a comment - 08/May/11 11:19 PM 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.

Andrea Aime added a comment - 10/May/11 6:41 AM 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)

Michael Bedward added a comment - 10/May/11 7:02 AM 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.

Michael Bedward added a comment - 03/Jun/11 11:05 PM 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.

Andrea Aime added a comment - 04/Jun/11 11:54 AM 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)

Jody Garnett added a comment - 19/Jun/11 2:15 AM 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.

Jody Garnett added a comment - 19/Jun/11 9:30 AM 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!

Jody Garnett added a comment - 19/Jun/11 10:27 AM This patch rolls together all the others; however svg tests in geoserver are now failing

Andrea Aime added a comment - 25/Jun/11 1:52 AM 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.

Andrea Aime added a comment - 25/Jun/11 2:21 AM 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

Andrea Aime added a comment - 02/Jul/11 5:25 AM Replacing the combined mega patch with a cleaned up one

Andrea Aime added a comment - 02/Jul/11 6:06 AM 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

Andrea Aime added a comment - 02/Jul/11 1:01 PM Patch finally applied, whew