Add Geometry::set_points, change Geometry::get_points to use OGR_G_GetPoints

[JmsPae]

• I agree to follow the project's code of conduct.
• I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

---

The PR has since expanded in scope, check the edit history and earlier discussion for additional context/scheming.

This PR adds Geometry::set_points, changes Geometry::get_points to use OGR_G_GetPoints and creates the CoordinateLayout to describe the layout of the in/output data for both of these methods.

This also removes Geometry::get_points_zm since it has been made redundant by a generalized coordinate layout enum, and could make implementing conversions to/from geo_traits somewhat easier.

[JmsPae]

This is also a small bit of groundwork to make harmonizing the index types (as discussed in #600 ) a little less annoying.

[JmsPae]

thoughts? @lnicola

[lnicola]

'do what the library does' and overwrite

I'm not sure I follow. GDAL doesn't know (or care) if you have a Vec-like growable buffer or not. You can pass in a pointer to the middle of a buffer and it will just write from that point. So technically, GDAL doesn't resize or clear the destination.

The vague warning in the GDAL docs does not spark confidence.

Slices are aligned, but the bigger issue is that (f32, f32, f32) is not #[repr(C)] so we can't pass a &mut [(f32, f32, f32] to GDAL. We could do the following and use &mut [PointXyz]:

#[repr(C)]
struct PointXyz(f32, f32, f32);

(&mut [[f32; 3]] might work too, but I'm actually not sure)

So the simplest would probably be to pass in a &mut Vec<f32> and document that the output will be laid out as XYZXYZXYZXYZ. But then some user will want XXXXYYYYZZZZ, which is only possible if we expose the stride parameters.

Can of 🪱s indeed 😀.

[JmsPae]

I'm not sure I follow. GDAL doesn't know (or care) if you have a Vec-like growable buffer or not. You can pass in a pointer to the middle of a buffer and it will just write from that point. So technically, GDAL doesn't resize or clear the destination.

Fair enough.

Slices are aligned, but the bigger issue is that (f32, f32, f32) is not #[repr(C)] so we can't pass a &mut [(f32, f32, f32] to GDAL.

For example, if we were to implement OGR_G_SetPoints? (Though that exposes stride etc so maybe not...?)

Nvm, i get what you mean... hmmm

[JmsPae]

We could start adding geometry types (shame geo is 2D-only) but that's... A lot 😄

[lnicola]

For example, if we were to implement OGR_G_SetPoints?

I think both OGR_G_GetPoints and OGR_G_SetPoints wrappers would look the same, there's no difference between them.

Maybe the simplest is to offer something like:

geom.get_points(&mut Vec::<f32>::new(), CoordinateLayout::XyzXyz); // there's probably a better name
geom.get_points(&mut Vec::<f32>::new(), CoordinateLayout::XxYyZz);

It doesn't offer the full power of the C API, but it's still useful. We could even fit Xyzm in the same API.

[JmsPae]

This might actually be a neat way to go about it if we want to avoid adding our own geometry types.

Since OGR_G_GetPoints and OGR_G_GetPointsZM both simply ignore null data, we could use this enum to limit which dims we want to extract and skip a separate zm impl.

It would, however, still be quite annoying for the end-user to chunk the output to extract individual points

[lnicola]

this enum to limit which dims we want to extract and skip a separate zm impl

Yeah, I updated my comment about this exactly when you posted yours. We could even do ZyxZyx 😬.

It would, however, still be quite annoying for the end-user to chunk the output to extract individual points

Only a little, right? And there's also Itertools::tuples if you don't mind an extra depedency.

[JmsPae]

Only a little, right?

It certainly beats having to cook up our own geo_types, so I guess I'm sold.

For all the XyzXyz, XxYyZz, ZzXxYy etc combinations, a macro might be warranted, but I'll have a gander with some basic variations for now.

[lnicola]

In any case, I propose we postpone (no need to close) this for a later time. I'd like to get a new release out of the door soon (maybe after 3.10.1 comes out in the next couple of days).

[JmsPae]

Very well, I'll be cooking with gas in the meantime.

[JmsPae]

@lnicola reasonably happy with this as a first go at it. Not sure how I'll go about strangely ordered coordinates (if that's even necessary) but feel free to take a peek when you want to.

Edit: Not my proudest git history, but I'll stop touching things for now.

[JmsPae]

Reasonably sure I'm thorough enough with these test-cases? The pointer manipulation does get a little scary, so idk if more are warranted.

[JmsPae]

Pretty sure this selection of layouts is sufficient? The bitmask was simple enough, though it isn't terribly flexible in case we want to add a Custom variant and would likely need an overhaul.

[JmsPae]

Perhaps take &mut [f64] instead? Would require additional checks to avoid segv, but I think being more general might be better. Eg. allowing for fixed slices as opposed to just Vecs (though perhaps that isn't especially useful?).

[JmsPae]

I went ahead and implemented this change, though dissenting opinions are welcome and I wouldn't mind reverting.

[JmsPae]

Ok now I'm done fiddling (for real this time) so feel free to review @lnicola @weiznich whenever you have time / are feeling bored / 0.18 gets kicked through the door.

I'll update the readme after 0.18 is released.