Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A Consistent and Principled Approach to GDAL C Pointers #445

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .cargo/config.toml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
[alias]
# Run doctests, displaying compiler output
# doc-test-output: Run doctests, displaying compiler output
dto = "test --doc -- --show-output"
# Run clippy, raising warnings to errors
nowarn = "clippy --all-targets -- -D warnings"
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ ndarray = { version = "0.15", optional = true }
chrono = { version = "0.4.26", default-features = false }
bitflags = "2.4"
once_cell = "1.18"
foreign-types = "0.5.0"

[build-dependencies]
semver = "1.0"
Expand Down
3 changes: 1 addition & 2 deletions examples/rasterband.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
use gdal::raster::RasterBand;
use gdal::{Dataset, Metadata};
use std::path::Path;

Expand All @@ -7,7 +6,7 @@ fn main() {
let dataset = Dataset::open(path).unwrap();
println!("dataset description: {:?}", dataset.description());

let rasterband: RasterBand = dataset.rasterband(1).unwrap();
let rasterband = dataset.rasterband(1).unwrap();
println!("rasterband description: {:?}", rasterband.description());
println!("rasterband no_data_value: {:?}", rasterband.no_data_value());
println!("rasterband type: {:?}", rasterband.band_type());
Expand Down
2 changes: 1 addition & 1 deletion examples/read_write_ogr_datetime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ fn run() -> gdal::errors::Result<()> {
for feature_a in layer_a.features() {
let mut ft = Feature::new(&defn)?;
if let Some(geom) = feature_a.geometry() {
ft.set_geometry(geom.clone())?;
ft.set_geometry(geom.to_owned())?;
}
// copy each field value of the feature:
for field in defn.fields() {
Expand Down
9 changes: 9 additions & 0 deletions fixtures/tinymarble.tif.aux.xml
Original file line number Diff line number Diff line change
@@ -1,3 +1,12 @@
<PAMDataset>
<SRS dataAxisToSRSAxisMapping="2,1">GEOGCS["WGS 84",DATUM["WGS_1984",SPHEROID["WGS 84",6378137,298.257223563,AUTHORITY["EPSG","7030"]],AUTHORITY["EPSG","6326"]],PRIMEM["Greenwich",0,AUTHORITY["EPSG","8901"]],UNIT["degree",0.0174532925199433,AUTHORITY["EPSG","9122"]],AXIS["Latitude",NORTH],AXIS["Longitude",EAST],AUTHORITY["EPSG","4326"]]</SRS>
<PAMRasterBand band="1">
<Metadata>
<MDI key="STATISTICS_MAXIMUM">255</MDI>
<MDI key="STATISTICS_MEAN">68.4716</MDI>
<MDI key="STATISTICS_MINIMUM">0</MDI>
<MDI key="STATISTICS_STDDEV">83.68444773935</MDI>
<MDI key="STATISTICS_VALID_PERCENT">100</MDI>
</Metadata>
</PAMRasterBand>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll revert this.

</PAMDataset>
9 changes: 7 additions & 2 deletions src/dataset.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use foreign_types::ForeignTypeRef;
use std::{ffi::CString, ffi::NulError, path::Path, ptr};

use gdal_sys::{self, CPLErr, GDALDatasetH, GDALMajorObjectH};
Expand All @@ -6,6 +7,7 @@ use crate::cpl::CslStringList;
use crate::errors::*;
use crate::options::DatasetOptions;
use crate::raster::RasterCreationOption;
use crate::spatial_ref::SpatialRefRef;
use crate::utils::{_last_cpl_err, _last_null_pointer_err, _path_to_c_string, _string};
use crate::{
gdal_major_object::MajorObject, spatial_ref::SpatialRef, Driver, GeoTransform, Metadata,
Expand Down Expand Up @@ -224,13 +226,16 @@ impl Dataset {
#[cfg(major_ge_3)]
/// Get the spatial reference system for this dataset.
pub fn spatial_ref(&self) -> Result<SpatialRef> {
unsafe { SpatialRef::from_c_obj(gdal_sys::GDALGetSpatialRef(self.c_dataset)) }
Ok(
unsafe { SpatialRefRef::from_ptr(gdal_sys::GDALGetSpatialRef(self.c_dataset)) }
.to_owned(),
)
}

#[cfg(major_ge_3)]
/// Set the spatial reference system for this dataset.
pub fn set_spatial_ref(&mut self, spatial_ref: &SpatialRef) -> Result<()> {
let rv = unsafe { gdal_sys::GDALSetSpatialRef(self.c_dataset, spatial_ref.to_c_hsrs()) };
let rv = unsafe { gdal_sys::GDALSetSpatialRef(self.c_dataset, spatial_ref.as_ptr()) };
if rv != CPLErr::CE_None {
return Err(_last_cpl_err(rv));
}
Expand Down
10 changes: 6 additions & 4 deletions src/gcp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,10 @@ use std::marker::PhantomData;
use gdal_sys::CPLErr;

use crate::errors::Result;
use crate::spatial_ref::SpatialRef;
use crate::spatial_ref::{SpatialRef, SpatialRefRef};
use crate::utils::{_last_cpl_err, _string};
use crate::Dataset;
use foreign_types::{ForeignType, ForeignTypeRef};

/// An owned Ground Control Point.
#[derive(Clone, Debug, PartialEq, PartialOrd)]
Expand Down Expand Up @@ -120,8 +121,9 @@ impl Dataset {
if c_ptr.is_null() {
return None;
}

unsafe { SpatialRef::from_c_obj(c_ptr) }.ok()
// NB: Creating a `SpatialRefRef` from a pointer acknowledges that GDAL is giving us
// a shared reference. We then call `to_owned` to appropriately get a clone.
Some(unsafe { SpatialRefRef::from_ptr(c_ptr) }.to_owned())
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lnicola said in 7db2868#r128939164

We could return a SpatialRefRef here, right?
One more reason to rename these to SpatialReference.

}

/// Get the projection definition string for the GCPs in this dataset.
Expand Down Expand Up @@ -207,7 +209,7 @@ impl Dataset {
self.c_dataset(),
len,
gdal_gcps.as_ptr(),
spatial_ref.to_c_hsrs() as *mut _,
spatial_ref.as_ptr() as *mut _,
)
};

Expand Down
2 changes: 1 addition & 1 deletion src/metadata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -382,7 +382,7 @@ mod tests {
fn test_set_description() {
let driver = DriverManager::get_driver_by_name("MEM").unwrap();
let dataset = driver.create("", 1, 1, 1).unwrap();
let mut band = dataset.rasterband(1).unwrap();
let band = dataset.rasterband(1).unwrap();

let description = "A merry and cheerful band description";
assert_eq!(band.description().unwrap(), "");
Expand Down
38 changes: 38 additions & 0 deletions src/raster/gcp.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
//! Raster ground control point support

use foreign_types::ForeignTypeRef;
use crate::spatial_ref::SpatialRefRef;
use crate::Dataset;

impl Dataset {
/// Get output spatial reference system for GCPs.
///
/// # Notes
/// * This is separate and distinct from [`Dataset::spatial_ref`], and only applies to
/// the representation of ground control points, when embedded.
///
/// See: [`GDALGetGCPSpatialRef`](https://gdal.org/api/raster_c_api.html#_CPPv420GDALGetGCPSpatialRef12GDALDatasetH)
pub fn gcp_spatial_ref(&self) -> Option<&SpatialRefRef> {
let c_ptr = unsafe { gdal_sys::GDALGetGCPSpatialRef(self.c_dataset()) };

if c_ptr.is_null() {
return None;
}

Some(unsafe { SpatialRefRef::from_ptr(c_ptr) })
}
}

#[cfg(test)]
mod tests {
use crate::test_utils::fixture;
use crate::Dataset;

#[test]
fn test_gcp_spatial_ref() {
let dataset = Dataset::open(fixture("gcp.tif")).unwrap();
let gcp_srs = dataset.gcp_spatial_ref();
let auth = gcp_srs.and_then(|s| s.authority().ok());
assert_eq!(auth.unwrap(), "EPSG:4326");
}
}
17 changes: 6 additions & 11 deletions src/raster/mdarray.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use super::GdalType;
use crate::errors::*;
use crate::spatial_ref::SpatialRef;
use crate::spatial_ref::{SpatialRef, SpatialRefRef};
use crate::utils::{_last_cpl_err, _last_null_pointer_err, _string, _string_array};
use crate::{cpl::CslStringList, Dataset};
use gdal_sys::{
Expand All @@ -16,12 +16,13 @@ use gdal_sys::{
GDALGroupOpenGroup, GDALGroupOpenMDArray, GDALGroupRelease, GDALMDArrayGetAttribute,
GDALMDArrayGetDataType, GDALMDArrayGetDimensionCount, GDALMDArrayGetDimensions,
GDALMDArrayGetNoDataValueAsDouble, GDALMDArrayGetSpatialRef, GDALMDArrayGetTotalElementsCount,
GDALMDArrayGetUnit, GDALMDArrayH, GDALMDArrayRelease, OSRDestroySpatialReference, VSIFree,
GDALMDArrayGetUnit, GDALMDArrayH, GDALMDArrayRelease, VSIFree,
};
use libc::c_void;
use std::ffi::CString;
use std::os::raw::c_char;

use foreign_types::ForeignTypeRef;
#[cfg(feature = "ndarray")]
use ndarray::{ArrayD, IxDyn};
use std::fmt::{Debug, Display};
Expand Down Expand Up @@ -324,15 +325,9 @@ impl<'a> MDArray<'a> {
}

pub fn spatial_reference(&self) -> Result<SpatialRef> {
unsafe {
let c_gdal_spatial_ref = GDALMDArrayGetSpatialRef(self.c_mdarray);

let gdal_spatial_ref = SpatialRef::from_c_obj(c_gdal_spatial_ref);

OSRDestroySpatialReference(c_gdal_spatial_ref);

gdal_spatial_ref
}
// NB: Creating a `SpatialRefRef` from a pointer acknowledges that GDAL is giving us
// a shared reference. We then call `to_owned` to appropriately get a clone.
Ok(unsafe { SpatialRefRef::from_ptr(GDALMDArrayGetSpatialRef(self.c_mdarray)) }.to_owned())
}

pub fn no_data_value_as_double(&self) -> Option<f64> {
Expand Down
Loading