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

instance openings for ifc #1010

Merged
merged 3 commits into from
Sep 25, 2023
Merged

Conversation

anthonie-kramer
Copy link
Contributor

@anthonie-kramer anthonie-kramer commented Aug 15, 2023

BACKGROUND:

  • What led to this work, and what is the broader context of this change?
  • Openings were not being created for ElementInstance objects
  • Openings were not being transformed for their LocalTransform

DESCRIPTION:

  • What does this PR specifically accomplish?
  • Adds correct transform and creates openings that can be used for the IFCRELVOIDSELEMENT creation

TESTING:

  • How should a reviewer observe the behavior desired by this pull request?
  • I have added a test that uses instances and creates the openings sucessfully

FUTURE WORK:

  • Is there any future work needed or anticipated? Does this PR create any obvious technical debt?
  • We should refactor Elements to find a better way to include Openings in a more straightforward way

REQUIRED:

  • All changes are up to date in CHANGELOG.md.

COMMENTS:

  • Any other notes.

This change is Reviewable

Copy link
Contributor Author

@anthonie-kramer anthonie-kramer left a comment

Choose a reason for hiding this comment

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

Updated with master, and resolved merge conflicts (updated with your ZAxis extrusion fix). So should be good for review!

Reviewable status: 0 of 1 approvals obtained (waiting on @katehryhorenko)

Copy link
Contributor

@katehryhorenko katehryhorenko left a comment

Choose a reason for hiding this comment

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

LGTM, just one small comment about naming

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @anthonie-kramer)


Elements.Serialization.IFC/src/Serialization/IFC/IFCElementExtensions.cs line 44 at r1 (raw file):

                    for (int i = 0; i < geoElementWithOpenings.Openings.Count; i++)
                    {
                        Opening o = geoElementWithOpenings.Openings[i];

Just a small preference here. Could you avoid names like 'o'? It's more readable, when the variable name is more meaningful, like 'opening'

Copy link
Contributor Author

@anthonie-kramer anthonie-kramer left a comment

Choose a reason for hiding this comment

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

Done!

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @katehryhorenko)


Elements.Serialization.IFC/src/Serialization/IFC/IFCElementExtensions.cs line 44 at r1 (raw file):

Previously, katehryhorenko (Kateryna Hryhorenko) wrote…

Just a small preference here. Could you avoid names like 'o'? It's more readable, when the variable name is more meaningful, like 'opening'

great point, fixed!

@anthonie-kramer anthonie-kramer merged commit bce861e into master Sep 25, 2023
2 checks passed
@anthonie-kramer anthonie-kramer deleted the ifc-openings-for-instances branch September 25, 2023 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants