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

Added deep copy utility for stl_file. #50

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

andreondra
Copy link
Contributor

The function allows to copy stl_file and create an independent instance. Some members are not copied, because they are used only internally by some procedures (see comments).

src/util.c Show resolved Hide resolved
src/util.c Outdated Show resolved Hide resolved
src/util.c Show resolved Hide resolved
src/util.c Outdated Show resolved Hide resolved
src/util.c Outdated Show resolved Hide resolved
src/util.c Outdated Show resolved Hide resolved

if(src->v_shared != NULL){

dst->v_shared = (stl_vertex*) calloc((dst->stats.number_of_facets / 2), sizeof(stl_vertex));
Copy link
Member

Choose a reason for hiding this comment

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

The "half the size" thing might benefit from a comment.

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 just copied the allocation from the stlinit.h to get the right size, so I'm not sure about it myself but I agree a comment would be good. I suggest adding the comment to the struct member/init routine, because this is not the only occurrence of the v_shared. I have the stl.h docs almost ready in my fork, so if you know the reason for the size, I will add the information to the documentation.

Copy link
Member

Choose a reason for hiding this comment

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

If I had to guess, it might have something to do with the fact that one edge belongs to 2 facets? No idea without reading the code. My idea for the comment was something like /* Mimic function X from stlinit.c */

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, added.

src/util.c Outdated
if(src->facet_start != NULL){

dst->facet_start = (stl_facet*)calloc(dst->stats.number_of_facets, sizeof(stl_facet));
if(dst->facet_start == NULL) perror("stl_copy");
Copy link
Member

Choose a reason for hiding this comment

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

We should probably set the exit flag here as well and return early?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, I'd return error on every allocation failure to guarantee data integrity when returning success.

@hroncok
Copy link
Member

hroncok commented Apr 25, 2022

@gladk If we are to add a small test program (in c) that uses this and exports various different STLs would you be able to help us wrap it in cmaketests to verify which files are identical and which are different?

@gladk
Copy link
Contributor

gladk commented Apr 25, 2022

Yes, sure! Please commit this test program here, so I will integrate this change into this pull request.

@hroncok hroncok added the api-stable This does not change the API label Apr 25, 2022
@andreondra
Copy link
Contributor Author

@gladk How should the test program look like? Probably a standalone C code that takes a STL filename as an argument, opens and copies the file, runs a series of repairs and then export the copied mesh for comparison using CMakeTest or should the assertions be done directly in the test program? Like comparing the internal stl_file structures etc. Please tell what's the best for you to work with :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-stable This does not change the API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants