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

Enhancements error viz #115

Merged
merged 14 commits into from
Sep 24, 2024
Merged

Enhancements error viz #115

merged 14 commits into from
Sep 24, 2024

Conversation

eleniv3d
Copy link
Collaborator

@eleniv3d eleniv3d commented Sep 20, 2024

Still work in progress. I open it for discussion and in order to keep track of what's missing

@DamienGilliard
Copy link
Collaborator

@eleniv3d Thanks ! I took the liberty of linking the issues, so they are closed automatically when merging.

I was just looking at the per-face metric issue. Do you see a clear way to solve it ? I'm just diving in and it's not 100% clear to me how we can do that the most efficiently.

@DamienGilliard DamienGilliard linked an issue Sep 20, 2024 that may be closed by this pull request
@eleniv3d
Copy link
Collaborator Author

eleniv3d commented Sep 20, 2024

@eleniv3d Thanks ! I took the liberty of linking the issues, so they are closed automatically when merging.

I was just looking at the per-face metric issue. Do you see a clear way to solve it ? I'm just diving in and it's not 100% clear to me how we can do that the most efficiently.

@DamienGilliard
I guess one way could be to already segment the pcd per face with a similar mechanism as for the joints. Perhaps the joint segmentator could already output clusters for faces directly if it doesn't add to the calculation time. Then the pcd2mesh distance would automatically know that it is calculating for every face and will pop the corresponding message as is now the case for joints vs beams

The other way would be to specify that we want the calculation to happen per face with another toggle or ideally if we manage to make issue #116 work. but then again the distance calculation would look in the whole pcd of the joint.

@9and3
Copy link
Contributor

9and3 commented Sep 20, 2024

@DamienGilliard
I guess one way could be to already segment the pcd per face with a similar mechanism as for the joints. Perhaps the joint segmentator could already output clusters for faces directly if it doesn't add to the calculation time. Then the pcd2mesh distance would automatically know that it is calculating for every face and will pop the corresponding message as is now the case for joints vs beams

can we not use the DFFace's brep face instead of the entire brep when doing the calculation of the error @eleniv3d ? And repeat it for each DFFace. I have some doubts about having an extra clustering component just for faces.

The other way would be to specify that we want the calculation to happen per face with another toggle or ideally if we manage to make issue #116 work. but then again the distance calculation would look in the whole pcd of the joint.

This could be nice and easy the same way we did it for the VIzSettings

@DamienGilliard
Copy link
Collaborator

DamienGilliard commented Sep 20, 2024

@eleniv3d we do agree that you want a list of lists of pointclouds (and None if no pointcloud was found)? :
[[face_1_joint_1, face_2_joint_1, ...], [face_1_joint_2, face_2_joint_2, ...], ..., [face_1_joint_n, face_2_joint_n...]]
(Just double checking, I am the kind of person who messes this kind of things easily)

@eleniv3d
Copy link
Collaborator Author

@eleniv3d we do agree that you want a list of lists of pointclouds (and None if no pointcloud was found)? : [[face_1_joint_1, face_2_joint_1, ...], [face_1_joint_2, face_2_joint_2, ...], ..., [face_1_joint_n, face_2_joint_n...]] (Just double checking, I am the kind of person who messes this kind of things easily)

Sounds good!

@eleniv3d
Copy link
Collaborator Author

Possibly okay to review now. I struggled a bit with getting the desired data from the segmentators and in the end hacked it a bit. Maybe we can see together in the next meeting.

Notes

  1. The input of the source point cloud can now be joints, beams or faces of any data structure. We internally flatten it and always compare it with the length of the corresponding lists from the assembly
  2. We handle the exception if a pcd is invalid and output the same invalid pcd or color its "twin" mesh white
  3. I'll do a separate PR for the CVS exporter

(per face calculation)
face

(invalid items visualization on pcd)
pcd

(invalid items visualization on mesh)
mesh

Copy link
Contributor

@9and3 9and3 left a comment

Choose a reason for hiding this comment

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

hello @eleniv3d ! Thanks for this PR, I just left some comments and did some tiny cleaning myself! Merging this..

Comment on lines 67 to 75
# if __name__ == "__main__":
# com = DFCloudMeshDistance()
# o_viz_settings = com.RunScript(
# i_cloud_source,
# i_assembly,
# i_signed_flag,
# i_swap,
# i_analysis_resolution
# )
Copy link
Contributor

Choose a reason for hiding this comment

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

to remove

Comment on lines +39 to +47
# if __name__ == "__main__":
# com = DFCsvExporter()
# o_viz_settings = com.RunScript(
# i_dump,
# i_export_dir,
# i_file_name,
# i_export_seperate_files,
# i_result
# )
Copy link
Contributor

Choose a reason for hiding this comment

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

to remove

Copy link
Contributor

Choose a reason for hiding this comment

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

In line 22 I propose to add this to avoid drop-on-canvas error:

if i_cloud_source is None or i_assembly is None:
            return None, None, None, None, None, None

Comment on lines 63 to 68
# if __name__ == "__main__":
# com = DFVisualization()
# o_colored_geo, o_legend, o_histogram = com.RunScript(
# i_result,
# i_viz_settings
# )
Copy link
Contributor

Choose a reason for hiding this comment

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

to remove

class DFVisualization(component):
def RunScript(self,
i_result: DFVizResults,
i_viz_settings: DFVizSettings):

Copy link
Contributor

Choose a reason for hiding this comment

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

the same we could add:

if i_result or i_viz_settings:
    return None, None

@9and3 9and3 merged commit f205939 into main Sep 24, 2024
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment