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

Adjust the Doors function to the changes in the Door class #56

Merged
merged 14 commits into from
Dec 2, 2023

Conversation

srudenkoamc
Copy link
Contributor

@srudenkoamc srudenkoamc commented Aug 18, 2023

BACKGROUND

  • Door class has been moved to Elements repo so it can be used during IFC serialization. The Doors function should be modified accordingly.

DESCRIPTION

  • Door class is removed. Modified version of the class is in the Hypar.Elements repo now.
  • Doors function is adjusted to the changes, made in the Door class.
  • Added workarounds for several bugs when several different walls had the same center line.

This change is Reviewable

It is done because of the following two reasons:
1) StandardWall is in the Elements and Door is to be moved to Elements.
2) StandardWall contains info about openings when WallCandidate does not.

Wall CenterLine is used for the mapping because id's in "Wall Candidate" additional property of StandardWall don't correspond to any WallCandidate's id's for some reason.
@srudenkoamc srudenkoamc marked this pull request as ready for review August 24, 2023 10:59
Copy link
Contributor

@DmytroMuravskyi DmytroMuravskyi left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 1 approvals obtained, 1 unresolved discussion (waiting on @anthonie-kramer and @srudenkoamc)


LayoutFunctions/Doors/hypar.json line 66 at r1 (raw file):

        "$hyparOrder": 2,
        "enum": [
          "SingleSwing",

These enums are user facing stings, I think whey should have spaces.

Copy link
Contributor

@DmytroMuravskyi DmytroMuravskyi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 approvals obtained, 2 unresolved discussions (waiting on @anthonie-kramer and @srudenkoamc)


LayoutFunctions/Doors/dependencies/Door.cs line 51 at r1 (raw file):

            Transform = GetDoorTransform(currentPosition, wallLine, flip);
            Representation = new Representation(new List<SolidOperation>() { });
            Opening = new Opening(Polygon.Rectangle(width, height), depthFront, depthBack, GetOpeningTransform(wallLine.Direction()));

Door transform should be already set up then Opening is created. I think GetOpeningTransform should have no parameters and fully use Door data.

@srudenkoamc
Copy link
Contributor Author

Reviewed all commit messages.
Reviewable status: 0 of 1 approvals obtained, 1 unresolved discussion (waiting on @anthonie-kramer and @srudenkoamc)

LayoutFunctions/Doors/hypar.json line 66 at r1 (raw file):

        "$hyparOrder": 2,
        "enum": [
          "SingleSwing",

These enums are user facing stings, I think whey should have spaces.

Fixed at 60834bd

@srudenkoamc
Copy link
Contributor Author

Reviewable status: 0 of 1 approvals obtained, 2 unresolved discussions (waiting on @anthonie-kramer and @srudenkoamc)

LayoutFunctions/Doors/dependencies/Door.cs line 51 at r1 (raw file):

            Transform = GetDoorTransform(currentPosition, wallLine, flip);
            Representation = new Representation(new List<SolidOperation>() { });
            Opening = new Opening(Polygon.Rectangle(width, height), depthFront, depthBack, GetOpeningTransform(wallLine.Direction()));

Door transform should be already set up then Opening is created. I think GetOpeningTransform should have no parameters and fully use Door data.

Fixed at c2d26d9

Copy link
Contributor

@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.

:LGTM:

Do we have a workflow where we are testing the updated Doors function?

Reviewed 4 of 9 files at r1, 5 of 5 files at r2, all commit messages.
Reviewable status: 1 of 1 approvals obtained, 2 unresolved discussions (waiting on @DmytroMuravskyi)

@srudenkoamc
Copy link
Contributor Author

srudenkoamc commented Sep 7, 2023

:lgtm:

Do we have a workflow where we are testing the updated Doors function?

Reviewed 4 of 9 files at r1, 5 of 5 files at r2, all commit messages.
Reviewable status: 1 of 1 approvals obtained, 2 unresolved discussions (waiting on @DmytroMuravskyi)

I don't think we do.

srudenkoamc and others added 5 commits October 25, 2023 14:02
- Updated version of Elements to one that contains Door class;
- Removed Door class, DoorOpeningSide and DoorOpeningType enums
@srudenkoamc srudenkoamc changed the title Prepare Door class to be moved to Elements Adjust the Doors function to the changes in the Door class Oct 26, 2023
Copy link
Contributor

@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.

I think we can pause the work on the Doors function, since we are likely changing the strategy to have the Door created as part of the SpaceBoundary and Wall definitions. Let's just focus on improving the Door class and prepare it to have a relationship with the Wall

Reviewed 6 of 9 files at r3, 2 of 2 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained, 3 unresolved discussions (waiting on @DmytroMuravskyi and @srudenkoamc)


LayoutFunctions/Doors/src/DoorOpeningEnumsHelper.cs line 5 at r5 (raw file):

namespace Doors
{
    internal static class DoorOpeningEnumsHelper

There aren't many who use the Doors function, so we can change it however we want... In this case, we should just change the override definitions in hypar.json to conform to what we need.

Copy link
Contributor

@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.

@srudenkoamc Disregard the comment above, I think we will use the Doors function as a starting place for the functionality that we want... I will probably try to test this out tomorrow with the updated Door class changes...

I think we will still want to generate Doors initially using the WallCandidate, and then allow for overrides to change location, and place the Door on the closest wall, and cut out an opening... This will probably happen in Interior Partitions which means I will makeDoors a dependency for Interior Partitions... I can handle the Wall stuff if you want to get the Door stuff in a good place for me to work with.

Reviewable status: 1 change requests, 0 of 1 approvals obtained, 3 unresolved discussions (waiting on @DmytroMuravskyi and @srudenkoamc)

@anthonie-kramer anthonie-kramer merged commit d1b4436 into hypar-io:master Dec 2, 2023
1 check failed
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.

None yet

3 participants