Dev/2606 Fix for orienting view to faces of objects that have transformations#2620
Dev/2606 Fix for orienting view to faces of objects that have transformations#2620jmcouffin merged 4 commits intopyrevitlabs:developfrom
Conversation
There was a problem hiding this comment.
PR Summary:
This PR fixes an issue with orienting views to faces of objects that have transformations, particularly for rotated family instances. The implementation changes the approach from using revit.pick_face() to directly using the Revit API's selection mechanism to get proper face references with their transformations. The changes affect both "Orient View To Face" and "Orient Section Box To Face" tools, and include context restrictions in the bundle.yaml files.
Review Summary:
The implementation successfully addresses the issue by handling transformations for family instances, which was missing in the original code. I've identified a few areas for improvement around transformation handling for non-family elements, robust face centroid calculation, and view consistency that would make the solution more robust. The PR is well-structured and follows the repository's coding standards.
Feel free to provide feedback on this review that I can take into account for future reviews.
Follow-up suggestions:
@devloai implement specific exception handling for OperationCanceledException to improve user experience
| geometry_object = element.GetGeometryObjectFromReference(face) | ||
|
|
||
| # Check if the object might have a Transformation (by checking if it's Non-Instance) | ||
| if isinstance(element, DB.FamilyInstance): |
There was a problem hiding this comment.
The comment suggests that FamilyInstance is checked to determine if an object has a transformation, but non-family elements can also have transformations. Consider a more robust check, perhaps by trying to access the transformation directly and falling back if it fails.
...tension/pyRevit.tab/Modify.panel/3D.pulldown/Orient Section Box To Face.pushbutton/script.py
Show resolved
Hide resolved
| geometry_object = element.GetGeometryObjectFromReference(face_ref) | ||
|
|
||
| # Check if the object might have a Transformation (by checking if it's Non-Instance) | ||
| if isinstance(element, DB.FamilyInstance): |
There was a problem hiding this comment.
Similar to the other script, the check for FamilyInstance is limiting. Consider handling transformations more broadly as other elements can also have transformations. A more robust approach would be to try to access the transformation directly with appropriate fallback logic.
| norm = geometry_object.ComputeNormal(DB.UV(0, 0)).Normalize() | ||
|
|
||
| # since we're working with a reference instead of the face, we can't use face.origin | ||
| if isinstance(geometry_object, DB.Face): |
There was a problem hiding this comment.
Using a fixed UV of (0.5, 0.5) assumes the face has a parametrization where this corresponds to the centroid. This may not be true for all faces. Consider using a more robust method to find the face centroid, such as calculating the average of boundary points or using appropriate Revit API methods if available.
...ools.extension/pyRevit.tab/Modify.panel/3D.pulldown/Orient View To Face.pushbutton/script.py
Outdated
Show resolved
Hide resolved
2620#discussion_r2020111832 - added suggested change
The merge-base changed after approval.
|
Thanks for both PR @Wurschdhaud |
|
📦 New work-in-progress (wip) builds are available for 5.0.1.25090+0707-wip |
|
📦 New work-in-progress (wip) builds are available for 5.0.1.25092+0954-wip |
|
📦 New work-in-progress (wip) builds are available for 5.0.1.25093+2001-wip |
|
📦 New work-in-progress (wip) builds are available for 5.0.1.25094+0703-wip |
|
📦 New work-in-progress (wip) builds are available for 5.0.1.25094+0800-wip |
|
📦 New work-in-progress (wip) builds are available for 5.0.1.25094+0926-wip |
|
📦 New work-in-progress (wip) builds are available for 5.1.0.25094+0940-wip |
|
📦 New public release are available for 5.1.0.25094+1133 |
|
📦 New public release are available for 5.1.0.25094+1133 |
|
📦 New public release are available for 5.1.0.25094+1133 |
dev/#2606 Fix for orienting view to faces of objects that have transformations
Description
The original code didn't work for families, that have been placed somewhere in open space, e.g. rotated generic models. Using this approach to take the object geometry instead of the face itself, transformation of the generic model can be obtained and taken into account.
I tried to use pyrevits pick_face(), but the reference of the face is always None when doing it that way. Not sure why.
The code is similar to #2619
Added context and min revit version to bundle.yaml
Checklist
Before submitting your pull request, ensure the following requirements are met:
pipenv run black {source_file_or_directory}Related Issues
Additional Notes
ran flake8
ran black
Thank you for contributing to pyRevit! 🎉