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

Better exception messages for breps that fail to meshify #634

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

JR-Morgan
Copy link
Member

CreateFromBrep will return null if a brep fails to meshify with the provided meshing options.
I notice that this with minimumEdgeLengh in particular on some breps.

image

In v2, we exposed these tolerances as options. But in leu of that, this PR just gives a clearer exception message

@JR-Morgan JR-Morgan enabled auto-merge (squash) February 27, 2025 14:55
Copy link

codecov bot commented Feb 27, 2025

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 8.71%. Comparing base (ddd6039) to head (eb8580a).

Files with missing lines Patch % Lines
...noShared/ToSpeckle/Meshing/DisplayMeshExtractor.cs 0.00% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             dev    #634      +/-   ##
========================================
- Coverage   8.71%   8.71%   -0.01%     
========================================
  Files        227     227              
  Lines       4370    4372       +2     
  Branches     545     546       +1     
========================================
  Hits         381     381              
- Misses      3974    3976       +2     
  Partials      15      15              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -29,6 +29,11 @@ public static RG.Mesh GetDisplayMesh(RhinoObject obj)
throw new ConversionException($"Unsupported object for display mesh generation {obj.GetType().FullName}");
}
}
if (renderMeshes == null)
Copy link
Member

Choose a reason for hiding this comment

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

The append method will also throw if any of the meshes inside render meshes is null, not just the list itself:
{7D4F196E-E1E7-413E-9563-5EC4E13AA3B7}
In v2, we were trycatching this operation. Can do the same and rethrow the exception

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not attempting to make it impossible for this function to throw an ArgumentNullException, I simply want to handle the expected case where Rhino's API returns an empty Mesh[] when a BREP can't be meshified.

In my testing, I did not see any cases where it returns an array with null values inside of it.
So if that happens, it would actually quite like for it to throw something dangerous that we can notice and raise our eyebrows at.

@JR-Morgan JR-Morgan requested a review from clairekuang March 3, 2025 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants