Hi all, Jody, pull requests updated according to feedback \ last changes.
Thank you ! Nuno Oliveira On 08/15/2018 01:18 AM, Jody Garnett wrote:
Thanks Nuno, comments/discussion inline: First comment, the current API breaks backward compatibility by requiring that a measure should always be provided when building a coordinate sequence, IMHO it should have a fallback method that assumes zero measures if none if provided ... otherwise changes in jaitools will also be required.The CoordinateSequenceFactory provides both create( dimension ) and create( dimension, measure ). Your link [1] is to a specific implementation, PackedCoordinateSequence.java <https://github.com/locationtech/jts/blob/master/modules/core/src/main/java/org/locationtech/jts/geom/impl/PackedCoordinateSequence.java#L48>, it would be wise to use the factory if you can?When investigating a failing test I step on a visibility change [1] that doesn't allow us to change LiteCoordinateSequence coordinates\dimension in place anymore.If I understand correctly LiteCoordinateSequence is willing to swap out its array on the fly right, and as part of that the dimensions and measures used to navigate the array.This is not a use-case I have inside JTS, but it is one that we can support ... will mark the field "protected" rather than "final protected".Another issues, the following test [3] is failing when invoking the normalize function [4] on polygon: Caused by: java.lang.IllegalArgumentException: Invalid ordinate index: 2 at org.locationtech.jts.geom.CoordinateXY.getOrdinate(CoordinateXY.java:77) at org.locationtech.jts.geom.impl.CoordinateArraySequence.getOrdinate(CoordinateArraySequence.java:260) at org.locationtech.jts.geom.CoordinateSequences.swap(CoordinateSequences.java:47) at org.locationtech.jts.geom.CoordinateSequences.reverse(CoordinateSequences.java:32) at org.locationtech.jts.geom.Polygon.normalize(Polygon.java:429) at org.locationtech.jts.geom.Polygon.normalized(Polygon.java:416) at org.locationtech.jts.geom.Polygon.normalize(Polygon.java:374) at it.geosolutions.jaiext.vectorbin.ROIGeometry.<init>(ROIGeometry.java:269) at it.geosolutions.jaiext.vectorbin.ROIGeometry.<init>(ROIGeometry.java:195) at it.geosolutions.jaiext.vectorbin.ROIGeometry.<init>(ROIGeometry.java:150) at org.geotools.coverage.grid.io.footprint.MultiLevelROIGeometry$FastClipROIGeometry.<init>(MultiLevelROIGeometry.java:162) at org.geotools.coverage.grid.io.footprint.MultiLevelROIGeometry.getTransformedROI(MultiLevelROIGeometry.java:128) at org.geotools.coverage.grid.io.footprint.MultiLevelROIGeometry.getTransformedROI(MultiLevelROIGeometry.java:45) at org.geotools.gce.imagemosaic.GranuleDescriptor.loadRaster(GranuleDescriptor.java:1348) at org.geotools.gce.imagemosaic.GranuleLoader.call(GranuleLoader.java:108) ... 44 more I'm not familiar with JTS code, so I'm clueless in find out what may be causing this :(This is the kind of feedback I needed thank you. Looking at the swap method it seems fine:public static void swap(CoordinateSequence seq, int i, int j) { if (i == j) return; for (int dim = 0; dim < seq.getDimension(); dim++) { double tmp = seq.getOrdinate(i, dim); seq.setOrdinate(i, dim, seq.getOrdinate(j, dim)); seq.setOrdinate(j, dim, tmp); } }Something has gotten in consistent, the dimension of the coordinate sequence must have changed from 2 (producing a cache of CoordinateXY) to 3 (producing a reference to getOrdinate( x, 2 ).I will try and reproduce the failure locally... Last finding, the previous coordinate object, even if only a dimension of 2 was provided, was always allowing to access or set a Z value, this is not the case anymore in the new API [5]. I agree that the behavior of the new API is the correct behavior, but unfortunately it breaks existing expectations. The issue here is that again this will prevent us to change geometries in place, which means that some not so trivial re-factor will need to be done in a few places.Let's figure out what is needed so you can change geometries in place and avoid a refactor unless there is a benefit.This are the issues I found when using the new JTS API, all of the found issues (at exception the polygon normalize) are the consequence of the new JTS API breaking backward compatibility.I am going to make an issue for the polygon normalize now (https://github.com/locationtech/jts/issues/296) and pull request (https://github.com/locationtech/jts/pull/297).
-- Regards, Nuno Oliveira == GeoServer Professional Services from the experts! Visit http://goo.gl/it488V for more information. == Nuno Miguel Carvalho Oliveira @nmcoliveira Software Engineer GeoSolutions S.A.S. Via di Montramito 3/A 55054 Massarosa (LU) Italy phone: +39 0584 962313 fax: +39 0584 1660272 http://www.geo-solutions.it http://twitter.com/geosolutions_it ------------------------------------------------------- Con riferimento alla normativa sul trattamento dei dati personali (Reg. UE 2016/679 - Regolamento generale sulla protezione dei dati “GDPR”), si precisa che ogni circostanza inerente alla presente email (il suo contenuto, gli eventuali allegati, etc.) è un dato la cui conoscenza è riservata al/i solo/i destinatario/i indicati dallo scrivente. Se il messaggio Le è giunto per errore, è tenuta/o a cancellarlo, ogni altra operazione è illecita. Le sarei comunque grato se potesse darmene notizia. This email is intended only for the person or entity to which it is addressed and may contain information that is privileged, confidential or otherwise protected from disclosure. We remind that - as provided by European Regulation 2016/679 “GDPR” - copying, dissemination or use of this e-mail or the information herein by anyone other than the intended recipient is prohibited. If you have received this email by mistake, please notify us immediately by telephone or e-mail.
------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________ Geoserver-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/geoserver-devel
