Skip to content

Spatial_Engine: Mirror implemented for IElements#3579

Open
pawelbaran wants to merge 11 commits into
developfrom
BHoM_Engine-#3578-MirrorOfIElements
Open

Spatial_Engine: Mirror implemented for IElements#3579
pawelbaran wants to merge 11 commits into
developfrom
BHoM_Engine-#3578-MirrorOfIElements

Conversation

@pawelbaran

@pawelbaran pawelbaran commented Apr 13, 2026

Copy link
Copy Markdown
Member

Issues addressed by this PR

Closes #3578

Test files

On SharePoint

Changelog

Additional comments

The code has been built based on the assumption that all transformations (translation, rotation, mirror, shear) should default down to Transform methods being called from within. This is exactly what is being done in this PR, requirement for transform methods being loosened to allow pure reflection.

Following the above, there is no need for extensive tests because literally all spatial mirrors will call Transform - if the latter is fine, we're good.

@peterjamesnugent peterjamesnugent left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One minor comment on using the existing Vector methods and one larger one on making sure the Mirror follows through for asymmetric profiles (similar case for Pile and anything that has an IProfile property.

Also, are we not supporting IElement2D?

Comment thread Geometry_Engine/Create/TransformMatrix.cs Outdated
Comment thread Structure_Engine/Modify/Transform.cs
@pawelbaran

Copy link
Copy Markdown
Member Author

Thanks for the review @peterjamesnugent, I've addressed all comments, also expanded the test script - would be great if you could have another look and address two things:

  • correctness of the current solution
  • whether Bar.Flip() is equal to mirroring against the plane with origin at bar center and normal parallel to bar tangent

...once this is done, I will update Flip method (if 2nd question receives positive answer), then create/update the tests and (hopefully) we will be ready to proceed with merging. Thanks!

@pawelbaran

Copy link
Copy Markdown
Member Author

@BHoMBot check compliance

@bhombot-ci

bhombot-ci Bot commented Jun 19, 2026

Copy link
Copy Markdown

@pawelbaran to confirm, the following actions are now queued:

  • check code-compliance
  • check documentation-compliance
  • check project-compliance
  • check branch-compliance
  • check dataset-compliance
  • check copyright-compliance

@bhombot-ci

bhombot-ci Bot commented Jun 19, 2026

Copy link
Copy Markdown

Please be advised that the check with reference 82350084431 has more than 50 annotations of notes. API limitations restrict annotations to 50. You may need to rerun this check to obtain the next set when you make changes. At the time of reporting this check, there are 7 additional annotations waiting, made up of 0 errors and 7 warnings.

@peterjamesnugent peterjamesnugent left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  • Script works as intended;
  • We can raise a seperate issue if we want to update Flip to make use of Transform
  • For the transformMatrix creation I think you had a reference for how that's derived, can we include that as //comment for future reference?

@pawelbaran

Copy link
Copy Markdown
Member Author

@BHoMBot check compliance
@BHoMBot check unit-tests

@bhombot-ci

bhombot-ci Bot commented Jun 19, 2026

Copy link
Copy Markdown

@pawelbaran to confirm, the following actions are now queued:

  • check code-compliance
  • check documentation-compliance
  • check project-compliance
  • check branch-compliance
  • check dataset-compliance
  • check copyright-compliance
  • check unit-tests

@bhombot-ci

bhombot-ci Bot commented Jun 19, 2026

Copy link
Copy Markdown

Please be advised that the check with reference 82371330239 has more than 50 annotations of notes. API limitations restrict annotations to 50. You may need to rerun this check to obtain the next set when you make changes. At the time of reporting this check, there are 7 additional annotations waiting, made up of 0 errors and 7 warnings.

@peterjamesnugent peterjamesnugent left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approving after compliance fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:feature New capability or enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Spatial_Engine: Add Mirror method for IElements

2 participants