Repository navigation
Conversation
|
@Mbucari - this is a big PR and it is going to take time to review and because of the scale of the ABI changes we are probably going to have to discuss when to bring it in. That is not to say tthat an overhaul like this is not overdue! As a starting point - you have a very nice list of the new methods (thank you). Is it possible to add a list of the obsoleted/changed methods (or method signatures). That will review a lot easier - having a list to tick off. |
@runette - Completely understandable, and I'd like to be a part of those discussions. Is there a formal process needed to kick-off a community review? Should you (a project Collaborator) or I (the PR author) spearhead the committee discussion/review?
You're very welcome. I've updated the PR comment with 3 new lists. I build a .NET assembly diff tool, so let me know if you want me to make any other queries or comparisons. |
|
Note that the "cs:defaultargs" feature requires SWIG >= v4.2.0. All workflows which build the C# bindings are using SWIG >= v4.2.0, but GDAL only requires SWIG >= 4.0.0. If SWIG < v4.2.0 then automatic overloads will be created instead (like Java). This could be problematic for multi-platform devs (of whom I am one). I checked @MaxRev-Dev's build systems and found that Windows uses v4.4.1, Mac uses v4.5.0, and Linux uses v4.1.0. The result is that the same package version would have a different ABI surface on Linux than it does on Windows and Mac. |
|
@szekerest , by the documentation https://gdal.org/en/stable/api/csharp/index.html you have been maintaining these things. |
Formerly yes, but feel free to update the document as needed. |
How about your gisinternals builds, would they, and you, be happy with these SWIG changes? |
@Mbucari Sorry if I worded it badly - I definitely meant "have a conversation in this PR " and you need to be involved and driving it - as you have. Even is usually the keeper of releases - I will start the process below. |
|
@rouault - a heads up that we have this PR that involves changes to the ABI. I have not started reviewing it but I am aware we will have to be carefull about how we fit this into the relese schedule. Note for all - I am about to spend the nexf five days working in a field! So, do not be concerned if I seem to go quiet for periods. |
That is going to be an issue. It is not just the people who distribute binary versions - people might be building themeselves and the publsihed requirements say SWIG>4 and not 4.2. To change that will require an RFC and has dependencies across the Python, Java and C# interfaces (which I am sure you know). @MaxRev-Dev - is there a reason why you are using SWIG 4.1 for Linux. That could shape the conversation. |
The SWIG interfaces conditionally enable that feature #if SWIG_VERSION >= 0x040200, so it does not change the build requirement. |
Good to know it will not crash, however we should take a deep breath before deliberately introducing ABI differences that depends on the SWIG version. I think that you might be about to say that we have them already - but that is not the same as intentionality. I guess not impossible but not a simple yes. I will have to pick this up after the weekend |
3d77651 to
43e8e55
Compare
|
Changes from SWIG 4.0.0 -> 4.2.0
|
I assume the methods that are no longer available were broken or barely usable because of missing typemaps ? That seem fine to me. That can be documented in doc/source/user/migration_guide.rst |
Correct.
Done. |
Those SWIG binaries are just installed from package managers (here and here). I didn't bother with versioning as it didn't cause any issues using different minor versions. I can pin them respectfully to 4.2.0+. |
|
Thank you, @MaxRev-Dev. @szekerest I see your build systems are all using SWIG 4.0.2. are you willing to upgrade to 4.2.0? |
34d8be7 to
1c81e93
Compare
- Add ArrayHelper class to help marshal arrays of Gdal objects and primitives to/from GDAL - Expose additional functions for C# which had previously only been exposed for Java/python. - Add multi-argument typemaps to map C# arrays to/from GDAL functions which accept list size and list pointer arguments, which already exist in Java and python. This causes the method signatures to changes for several methods which accept array arguments. - Fix `CTYPE *argout[ANY]` typemaps not working correctly, which fixes `SpatialReference` methods `ExportToPCI` and `ExportToUSGS`. - Fix incorret typemap applied to `panSuccess` argument in `GDAL.Transformer.TransformPoints`. - Fix GUIntBig arrays incorrectly being typemapped to C# uint arrays, now mapped to long arrays. - Fix incomplete typemap definitions for `GDALProgressFunc`. - Improve `GDALRasterIOExtraArg` - Expose existing `GDALRasterIOExtraArg` struct in SWIG instead of re-defining it. - Add support for progress delegates with string messages and C# object data. - OGRLayerShadow **ppoBelongingLayer typemap is now `out Layer` instead of `ref IntPtr` - Expose GDALGetOpenDatasets for C#
Several method signatures were changed due to changes in typemaps. Re-add all methods that were removed and mark them as obsolete. For removed methods whose arguments are easily mapped onto the new methods, create an overload accessing the new method. For all other removed methods, mark the obsolete function as an error.
- consolidated all SWIGTYPE typedefs - Remove SWIG_VERSION <= 0x020000 code since SWIG 4.0 or greater is required. - SWIGTYPEs throw exception if accessed after disposed instead of causing a crash
- Add multi-argument byte array support for (int nList, char* pList) - Add backwards compatibility overload for old Geometry constructor
Disable the swig feature "compactdefaultargs" for C# so that SWIG generates overloads for all methods which have default arguments.
- Use the Java renamings for "wrapper_*" method names. - Add overloads for "wrapper_*" methods which point to the new methods names and are tagged obsolete.
- Add overloads to get/set values by field name - Add C# support for int64 list
- Use GDALRATDateTime typedef from gdal.h and extend it with constructors and destructor - Add C# Feature overloads to get/set date fields as System.DateTime - Add C# functions to convert RATDateTime to/from DateTimeOffset
- Use RasterIOEx for all raster IO operations - Add support for more C# raster IO data types - Add C# default parameters
Extend StringListMarshal to support collections of KeyValuePair<string,string>
- Array out typemap was incorrect. It didn't free the array and didn't correctly marshal it. - Add GetDefaultHistogramEx with ulong array - Fix AdviseRead C# - buf_xsize and buf_ysize were incorrectly mapped to type 'ref int' which can never be null in C#
- Allow passing multiple layers - Add GDALProgressFunc typemap
Adjust GetNextFeature() definition to allow default arguments
- Use multivalue typemaps - Add GetHistogramEx support
Also fix Checksum default argument
- Simplified and consolidated C# typemaps - Expand TransformPoints methods to accept optional arrays - Enable Geometry.ExportToWkb and Osr. CreateGeometryFromWkb to for geometries > 2GB (unmanaged data only) - Expose Geometry.ExportToIsoWkb - Expose SetDefaultHistogramEx - Add more obsolete messages
Updated migration guide to reflect changes impacting C# users, including removed call sites and parameter type corrections.
1c81e93 to
eb94da6
Compare
|
@rouault I fixed the typo causing the check to fail. The Ubuntu ASAN test always fails, but that doesn't appear to be caused by this PR. |
|
The GDAL project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 28 days and is being automatically marked as "stale". If you think this pull request should be merged, please check
|
|
This PR is ready for review. @runette |
What does this PR do?
Overhauls the C# SWIG bindings by:
GDALProgressFuncDelegate'argout') and adding new ones (eg.CTYPE **array_argout_free)SetFieldAsDateTimeandGetFieldAsDateTimeusingSystem.DateTime)New Methods exposed to C#
This PR exposes several new GDAL functions. See the list below, and use checkboxes to track their review.
List of new GDAL methods exposed to C#
OSGeo.OSR.CoordinateTransformation.TransformPointsWithErrorCodesSetFieldInteger64ListGetFieldAsInteger64ListSetFieldBinaryGetFieldAsBinaryOSGeo.OGR.FldDomain.GetEnumerationWkbLongSize(property)ExportToIsoWkbGetDataset(via an OSGeo.GDAL extension method)GetExtent(overload which acceptsint geom_field)GetExtent3DCreateCodedFieldDomainCreateGeometryFromEnvelopeGetDefaultAsIntegerListGetDefaultAsDoubleListGetAsIntegerListGetAsDoubleListSetDatasetsGetHistogramExGetDefaultHistogramExSetDefaultHistogramExSetGCPs2(accepts a SpatialReference object)CreateLayerCreateLayerFromGeomFieldDefnCopyLayerExecuteSQLReleaseResultSetGetStyleTableSetStyleTableOSGeo.GDAL.ExtendedDataType.CreateCompoundOSGeo.GDAL.Gdal.GetOpenDatasetsOSGeo.GDAL.RATDateTime.ctorOSGeo.GDAL.RATDateTime..ctorNew Helper Methods
I added some new helper methods which are implemented purely in .NET
List of new helper C# methods
byte[] ExportToWkb(wkbByteOrder byte_order)byte[] ExportToIsoWkb(wkbByteOrder byte_order)GetExtent3D(out double x, out double y)GetExtent3D(out double x, out double y, out double z)GetExtent3D(out double x, out double y, out double z, out double m)GetFieldAsDateTime(int id)GetFieldAsDateTime(string field_name)FromDateTimeOffsetToDateTimeOffsetSetProgressDelegateSetProgressDelegate<TData>Method Signature Changes
For existing C# methods whose signature changed due to the new typemaps, I added those methods back and marked them as obsolete with messages directing users to the replacement method. Most of those obsolete methods are forwarded to the new method signature, but some will be marked as an error and will throw a NotSupportedException. See the list below, and use checkboxes to track their review.
List of existing C# methods which this PR marks as obsolete
Methods marked with ❌ are marked as compiler errors and trow a
NotSupportedException.TransformPoints(int, double[], double[], double[])ExportToPCI(out string, out string)(fixed typemap to enableout double[] params_)ExportToUSGS(out int, out int, out int)(fixed typemap to enableout double[] params_)SetDataAxisToSRSAxisMapping(int, int[])FindMatches(string[], out int, out int[])SetFieldDoubleList(int, int, double[])SetFieldIntegerList(int, int, int[])SetFromWithMap(Feature, int, int, int[])ExportToWkb(int, IntPtr, wkbByteOrder).ctor(wkbGeometryType, string, int, IntPtr, string)ReorderFields(int, int[])UpdateFeature(Feature, int, int[], int, int[], bool)CreateGeometryFromWkb(uint, IntPtr, SpatialReference)SetAsIntegerList(int, int[])SetAsDoubleList(int, double[])MaximumOfNBands(int, Band[])MeanOfNBands(int, Band[])MinimumOfNBands(int, Band[])GetHistogram(double, double, int, int[], int, int, GDALProgressFuncDelegate, string)AdviseRead(int, int, int, int, ref int, ref int, ref int, string[])Checksum(int, int, ref int, ref int)SetDefaultHistogram(double, double, int, int[])GetDefaultHistogram(out double, out double, out int, out int[], int, GDALProgressFuncDelegate, string)AdviseRead(int, int, int, int, ref int, ref int, ref int, int, int[], string[])BuildOverviews(string, int, IntPtr, GDALProgressFuncDelegate, string, string[])GetNextFeature(ref IntPtr, ref double, GDALProgressFuncDelegate, string)CreateAttribute(string, int, uint[], ExtendedDataType, string[])CreateAttribute(string, int, uint[], ExtendedDataType, string[])Resize(int, uint[], string[])Transpose(int, int[])TransformPoints(int, int, double[], double[], double[], double[])VSIFWriteL(string, int, int, IntPtr)GCPsToHomography(int, IntPtr, double[])RasterizeLayer(Dataset, int, int[], Layer, IntPtr, IntPtr, int, double[], string[], GDALProgressFuncDelegate, string)RegenerateOverviews(Band, int, Band[], string, GDALProgressFuncDelegate, string)ContourGenerate(Band, double, double, int, double[], int, double, Layer, int, int, GDALProgressFuncDelegate, string)CreatePansharpenedVRT(string, Band, int, Band[])wrapper_GDALBuildVRT_objects(string, int, IntPtr, GDALBuildVRTOptions, GDALProgressFuncDelegate, string)wrapper_GDALMultiDimTranslateDestName(string, int, IntPtr, GDALMultiDimTranslateOptions, GDALProgressFuncDelegate, string)wrapper_GDALContourDestDS(Dataset, Dataset, GDALContourOptions, GDALProgressFuncDelegate, string)wrapper_GDALContourDestName(string, Dataset, GDALContourOptions, GDALProgressFuncDelegate, string)wrapper_GDALDEMProcessing(string, Dataset, string, string, GDALDEMProcessingOptions, GDALProgressFuncDelegate, string)wrapper_GDALFootprintDestDS(Dataset, Dataset, GDALFootprintOptions, GDALProgressFuncDelegate, string)wrapper_GDALFootprintDestName(string, Dataset, GDALFootprintOptions, GDALProgressFuncDelegate, string)wrapper_GDALGrid(string, Dataset, GDALGridOptions, GDALProgressFuncDelegate, string)wrapper_GDALNearblackDestDS(Dataset, Dataset, GDALNearblackOptions, GDALProgressFuncDelegate, string)wrapper_GDALNearblackDestName(string, Dataset, GDALNearblackOptions, GDALProgressFuncDelegate, string)wrapper_GDALRasterizeDestDS(Dataset, Dataset, GDALRasterizeOptions, GDALProgressFuncDelegate, string)wrapper_GDALRasterizeDestName(string, Dataset, GDALRasterizeOptions, GDALProgressFuncDelegate, string)wrapper_GDALTranslate(string, Dataset, GDALTranslateOptions, GDALProgressFuncDelegate, string)wrapper_GDALVectorTranslateDestDS(Dataset, Dataset, GDALVectorTranslateOptions, GDALProgressFuncDelegate, string)wrapper_GDALVectorTranslateDestName(string, Dataset, GDALVectorTranslateOptions, GDALProgressFuncDelegate, string)wrapper_TileIndex_names(string, string[], GDALTileIndexOptions, GDALProgressFuncDelegate, string)GDAL_GCP_*property getters/settersMethod Changes Which Should Be Transparent to the Caller
Some methods were changed in ways that shouldn't require any changes by users (e.g. changing a parameter type from
inttolongor adding a new parameter that has a default argument). These changes have the potential to be breaking if these methods are cast to delegates, but they should not cause problems if called normally. See the list below, and use checkboxes to track their review.List of transparent changes
OSGeo.GDAL.Band.ExportToWkb(byte[] buffer)(new method has second argument with a default value)OSGeo.OGR.Geometry..ctor(wkbGeometryType type)(new method has additional argument with a default value)OSGeo.GDAL.Band.ReadRaster,OSGeo.GDAL.Band.WriteRaster,OSGeo.GDAL.Dataset.ReadRaster, andOSGeo.GDAL.Band.WriteRastermethods (changedpixelSpaceandlineSpacetypes tolongandextraArghas default valuenull)Dataset.BuildOverviews(string, int[])andDataset.BuildOverviews(string, int[], GDALProgressFuncDelegate, string)(new method has additional argument with a default value)Methods Signatures Changed With No Replacement (breaking)
There is only one method that was changed without adding replacements or obsolete messages:
OSGeo.GDAL.MDArray.BuildOverviews(string, int, SWIGTYPE_p_int, GDALProgressFuncDelegate callback, string, string[]). This method was properly type mapped a to accept anint[].Types Removed
Due to typemap fixes in this PR,
OSGeo.GDAL.SWIGTYPE_p_GDALProgressFuncandOSGeo.GDAL.SWIGTYPE_p_intare no longer in the Assembly. This is a breaking change, but I doubt anyone was using them.What are related issues/pull requests?
#3041
#10534
AI tool usage
Tasklist
Environment
Provide environment details, if relevant: