First wave of function bindings update.

[ThomasCartier]

Tests are still missing.

• Not ready right now.
  Confirmation needed:

1. Style ok ?
2. I need some confirmation for io handling (obj, vtk).
   Do we follow the same strategy as wkb and al ?
3. Any further objection before I keep going ?

[mthh]

Thank you for undertaking this work!

Concerning the style, I usually run rustfmt (with cargo fmt) on the code before pushing any changes (I should include this in the continuous integration pipeline to check that the style is OK at the same time as the tests).

Anything that's OK for rustfmt is OK for me (and it avoids discussing a style guide).
... Except for one or two small details: I've seen you introduce a lot of new empty lines (for example, here - click on 'load diff' to go to the excpected line - between the documentation and the start of the function code or there between [#test] and the test code ; ditto for the line breaks introduced between each line in the code of certain functions where the code itself doesn't change). Is there a reason for this?
It adds a lot of noise to the diff and doesn't seem necessary (Rust's own source code doesn't include such new empty lines between a function's documentation and the function itself, for example: https://doc.rust-lang.org/src/alloc/vec/mod.rs.html#397).

[ThomasCartier]

You are right for the formatting, it's because I use a different one from the official to ease the coding.
But I always revert my custom rustfmt.toml before sharing.
Apparently, there were some missed bits! Let me correct that.

I will add some tests today.

Side question: did you create the shapes in the test by hand, or is it an export from Blender or else ?

[mthh]

I need some confirmation for io handling (obj, vtk).
Do we follow the same strategy as wkb and al ?

I'm not sure what information you want. So far WKB wasn't supported (but it seems like a good idea to support it). The same goes for VTK and OBJ (although it's not necessarily a priority).

I guess VTK / OBJ writer could be defined the same way than the WKT writer, with something like the (totally untested) following:

pub fn geometry_as_vtk(&self) ->  -> Result<String> {
    let mut ptr = MaybeUninit::<*mut c_char>::uninit();
    let mut length: usize = 0;
    unsafe {
        sfcgal_geometry_as_vtk(self.c_geom.as_ref(), ptr.as_mut_ptr(), &mut length);
        Ok(_c_string_with_size(ptr.assume_init(), length))
    }
}

[ThomasCartier]

Yep, I will implement that today this way

[mthh]

Side question: did you create the shapes in the test by hand, or is it an export from Blender or else ?

If I remember correctly, some of them are coming from SFCGAL test suite and other may have been created by hand (or reuse examples seen on the Web such as in the PostGIS examples and stuff like that).

[ThomasCartier]

Nice, I will dig into it, it will ease my life :)
I will update the corrected work in a few hours!

BTW, I saw they are about to merge 3D alpha shape. This is a killer feature. I will keep you updated when it's there if you wish. I will pre implement it here and leave it commented. This way, minimal work for you :)

[ThomasCartier]

Last point to discuss: some algorithms need a specific type to work.

I solved this by using this type of matching.

I think I can expand that on most of the functions, because it avoids a ffi roundtrip in case the preconditions don't hold, with a clearer error message.

That's ok for you ?

[mthh]

BTW, I saw they are about to merge 3D alpha shape. This is a killer feature. I will keep you updated when it's there if you wish. I will pre implement it here and leave it commented. This way, minimal work for you :)

OK for me, thanks!

Last point to discuss: some algorithms need a specific type to work.

I solved this by using this type of matching.

I think I can expand that on most of the functions, because it avoids a ffi roundtrip in case the preconditions don't hold, with a clearer error message.

That's ok for you ?

Just had a quick look and I guess it's a good idea for functions that we know will fail on the C API side.
There are already probably some functions (at least one) for which the precondition about the geometry type isn't checked and the C function returns an error :

    let res = some_sfcgeometry_of_type_point.line_substring(12., 20.);
    println!("{:?}", res);

returns Err(Obtained null pointer when creating geometry: line_sub_string(): the first argument must be a lineString) .

While others (such as the orientation or the volume methods) have a precondition like "geom is a Polygon" or "geom is a Volume" (cf. https://sfcgal.gitlab.io/SFCGAL/API/sfcgal__c_8h/#function-sfcgal_geometry_orientation) and still return Ok(some_value) (like Ok(Orientation::undetermined) or Ok(0.0)) which might be OK to keep as it.

[ThomasCartier]

For the latter case, are you against a cfg feature that would enable the users to choose to bail early ? That way, people have the option to choose how "tight" the api is ? I can implement it if you let me.

[mthh]

I deduce that you prefer to fail early (avoiding the FFI round trip)?
Honestly, I'm open to discussion and we can also choose to do this way, to avoid writing extra code with cfg feature flag.

I was just thinking of possible use cases (where a user has - really simple example - a Vec of geometries of mixed types, and accumulates their volumes to calculate the sum, knowing that it returns 0.0 for geometries whose volume can't be calculated would avoid having to deal with Err's that would be returned by the method, allowing you to always unwrap...).
Well, it's a bit contrived.

[mthh]

Let's do without cfg feature and choose to match the preconditions indicated in the documentation on the Rust side, without doing the FFI round trip, even for the few functions (like “orientation” or “volume”) that don't return an error on the C API side even when the precondition doesn't hold.

This will probably lead to a clearer API / better user experience.

[ThomasCartier]

Ok!

[mthh]

Sorry I slightly modified the src/geometry.rs file to make clippy happy after slightly improving the CI workflows, so you'll need to merge the latest master version into your PR branch.

[ThomasCartier]

Hi,
Using the fork, I find some things to change for dev experience:

1. many functions starting with geometry_XXX could be renamed to XXX directly. For example my_sfgeometry.translate3d(...) instead of my_sfgeometry.geometry_translate3d(...)
2. I went too far with tesselate. it returns early when it shouldn't. Surely tired, I didn't spot it. Must correct that tomorrow.

What's your opinion on 1) ?

[mthh]

Sorry for the delay.

Yes, I think it makes sense to remove the “geometry_” part from these method names (keeping of course “triangle_”, “linestring_”, etc. for methods dedicated to triangles, linestrings, etc.).

[mthh]

Hi @ThomasCartier,

Thanks for all the work. Do you want me to take over and finish this PR?

[ThomasCartier]

@mthh : Hi! I can finish it tomorrow if you want; It's minimal.
I don't know why it didn't pass the beta test actually. I use the fork everyday to check if it's ok.

[mthh]

I pushed a few changes to your PR branch so that the tests can pass.

Don't hesitate if you want to use this PR to apply your previous suggestion (about renaming methods that start with geometry_).
Otherwise I think we are close to merging this PR (the setup_helper.rs file should be removed - and I've to take a second look at the few changes before merging but it shouldn't be to long).

EDIT: In fact, the clippy step still don't work on nightly because of the line breaks between doc comments and functions (rustfmt has removed the other superfluous line breaks, but not these ones). But otherwise I think it should be fine on stable / beta.

[ThomasCartier]

Hi @mthh
I corrected the last stuffs. Please tell me if something is missing!
Cheers

[ThomasCartier]

I don't know what's going on, please take over, I won't be able to allocate more time to it! Cheers

[mthh]

No problem, thanks again!

I will try to do a release before the end of the week as I have work to do mobilizing sfcgal, it will be an opportunity to test this new version 👍

[ThomasCartier]

@mthh Hi Matthieu, I just saw that BufferType is not recognized while being a pub enum.
Do you have an idea why?
It's used in the Buffer3d function.
Thanks!

[mthh]

Thanks for the report. Indeed it's not reexported in lib.rs. I will also derive Debug, PartialEq, Eq etc. on BufferType.
I will publish a fix today.

[ThomasCartier]

Nice thanks a lot!