Updated Missing functions #11
Conversation
|
Thanks for the PR! |
|
Did it! Is that ok for you? |
|
Thanks! I would have preferred a rebase (to avoid having a merge commit, it keeps a cleaner commit history) but no worries :) |
|
I will be in front of my computer in 1 hour will correct that! |
|
I am not very comfy with git commands, not sure I accessed your request ( I would like to, I use VSCode, if you know more, and I did wrong, please do not hesitate) |
|
I don't think you've done what's necessary because I can still see the merge commit. If you haven't added anything (locally) since the merge commit on your branch, I think you can do the following in a terminal: # Check the last commit is the merge commit
git log -n 1
# You should see the commit info, with the message
# "Merge remote-tracking branch 'original/master'"
# Otherwise, do not continue to follow the instructions below!
# Delete the last commit (the merge commit)
git reset --hard HEAD^
# Add this repo as a remote called 'upstream'
git remote add upstream https://github.com/mthh/sfcgal-rs
# Fetch the latest changes from this repo
git fetch upstream
# Rebase your branch onto the latest master branch from this repo
git rebase upstream/master
# Push your updated branch to your fork
# (--force/--force-with-lease is needed to overwrite the history)
git push --force-with-leaseNote that these instructions are based on what I see (on GitHub) of the state of the master branch from which you made the PR (but I don't know what you tried to do in your last post, I don't see any changes on GitHub). If you don't feel confortable doing this (or if you have new local commits, or unstaged changes), really, no worries, I could always squash the commits when I accept this PR. |
|
Ok I just did your listed actions. Looks like it did it :) I am not sure if it's possible to do it directly with the VSCode options though (GUI only, without using the terminal). Maybe next time I should follow up on the different branch "update functions" instead of working on the main branch.. |
Great, thanks :) |
|
|
||
| // For review | ||
| use sfcgal_sys::{ | ||
| sfcgal_geometry_alpha_wrapping_3d, sfcgal_geometry_as_stl, sfcgal_geometry_as_stl_file, |
There was a problem hiding this comment.
The CI is running against the latest stable sfcgal release (2.1.0) which doesn't seem to contain the following functions (sfcgal_sys::sfcgal_geometry_as_stl, sfcgal_sys::sfcgal_geometry_as_stl_file, sfcgal_sys::sfcgal_geometry_get_geometry_n, sfcgal_sys::sfcgal_geometry_get_geometry_n, sfcgal_sys::sfcgal_geometry_set_geometry_n).
I tested locally (and checked in the source distribution of sfcgal 2.1.0) and I can't find them either (although they are in the documentation https://sfcgal.gitlab.io/SFCGAL/API/group__capi/, which is probably based on the current source tree of sfcgal).
| // For review | ||
| use sfcgal_sys::{ | ||
| sfcgal_geometry_alpha_wrapping_3d, sfcgal_geometry_as_stl, sfcgal_geometry_as_stl_file, | ||
| sfcgal_geometry_boundary, sfcgal_geometry_centroid, sfcgal_geometry_centroid, |
There was a problem hiding this comment.
Duplicated sfcgal_geometry_centroid should be removed.
| SFCGeometry::new_from_raw(converted, true) | ||
| } | ||
|
|
||
| // For review |
There was a problem hiding this comment.
The newly added function looks good to me. It is just lacking documentation for now but I guess you were planning to add documentation to them? Otherwise I could accept it as it and do it myself :)
Concerning the functions that depend on the new sfcgal function there weren't included in 2.1.0 (namely sfcgal_sys::sfcgal_geometry_as_stl, sfcgal_sys::sfcgal_geometry_as_stl_file, sfcgal_sys::sfcgal_geometry_get_geometry_n, sfcgal_sys::sfcgal_geometry_get_geometry_n, sfcgal_sys::sfcgal_geometry_set_geometry_n), you can just comment the code and the imports and we will add them when 2.2.0 will be released ? (or you can remove them totally from this PR, and create a new draft PR for them, but it's a bit of extra work that's not necessarily necessary).
There was a problem hiding this comment.
Yes you are right, I probably pulled the trigger too fast, many functions are planned for 2.2.0 (that will be less work for the future)! I will double check that, comment the offenders and resubmit! :)
|
Thanks for this new contributions :) It's not directly concerning this PR, but as you often contribute new features (and as there's a notable difference here between the latest stable version and the development version), I wanted to inform you that I may try to integrate some form of versioning into the next version of sfcgal-sys / sfcgal-rs (a bit like what GEOS bindings do in Rust) using feature flags, allowing users to build the library against the version of SFCGAL they own (like |
|
I was thinking about it while separating the new functions, that would be much clearer to roam in the code! Excellent idea! 😉 |
2 participants
Updated the missing functions since last C api update.
I didn't touch sfcgal-sys.
I also marked some functions as deprecated, to follow the sfcgal guideline.
To ease the review, I separated the newly added sfcgal_ function in their own block.
The rust methods were grouped below the "For review" to ease the double check.
Some new functions were not decorated with pre checks: this is intentional because it would require more work. I may do that later.
Please feel free to point errors I could make. I will correct them. :)