Door-representation-improvements-ak #1057

anthonie-kramer · 2023-11-15T21:56:17Z

BACKGROUND:

The door class needed updates to include representations. I have also done a bit of refactoring to include Door specific types within the same file. This includes changes from Serhii at AMC.

DESCRIPTION:

Provides multiple representations for different parts of the door. Provides additional simplifications to reduce extra code.

TESTING:

This is now used in the Doors function in this workflow in test mode:
https://hypar.io/workflows/46428205-afdf-41bd-af52-4624c8e9f88e?view=b09eebe9-9857-4fcf-92e4-e4173e928289

FUTURE WORK:

Doors could probably undergo another round of refactoring to clean up and we should think about grouping other "BIM" types in Elements

REQUIRED:

All changes are up to date in CHANGELOG.md.

COMMENTS:

Any other notes.

This change is

- Fixed Door deserialization issue - Door can be deserialized now; - Removed 'Wall' property; - Added 'isElementDefinition' property to constructors; - Added DoorTest.

The representation is a RepresentationInstance now instead of Representation.

- Moved curve representation to DoorRepresentationFactory; - Added curve representation to UpdateRepresentations.

…/srudenkoamc/Elements into door-representation-improvements

…ctory

…/srudenkoamc/Elements into door-representation-improvements

…on-improvements

- Method CreateDoorOpening is used now instead of Opening property; - Removed opening properties from constructor; - Fixed issue in Opening class; - Fixed several small issues in door opening size computation.

The tests failed because doors don't have Opening members anymore.

wynged

Reviewed 7 of 9 files at r1, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @anthonie-kramer)

Elements/src/Door.cs line 172 at r1 (raw file):

                {
                        var representationInstances = new List<RepresentationInstance>()
            {

this formatting looks pretty weird.

Elements/src/Door.cs line 207 at r1 (raw file):

                private RepresentationInstance CreateDoorCurveRepresentation()
                {
                        var points = CollectPointsForSchematicVisualization();

in all of the methods that make representations it seems like we're always making a new RepresentationInstance for each door, but a big reason to move to RepresentationInstances is so that we can re-use instances. This will keep the glb model size down which makes function execution faster.

Elements/src/Door.cs line 380 at r1 (raw file):

        }

        public enum DoorOpeningSide

in general we like to give classes and enums their own file. looks like these got merged in for some reason?

Elements/src/StandardWall.cs line 122 at r1 (raw file):

            // Opening transform is still off... `outOfPlane` added because transform is not working correctly
            var outOfPlane = xAxis.Cross(Vector3.ZAxis);
            // Opening transform is still off?

are these comments correct? does this adjustment fix the opening transform or not?

Elements/test/DoorTest.cs line 17 at r1 (raw file):

        public void Example()
        {
            this.Name = "Elements_Door";

use nameof(MakeDoorElement) and change the name of this method to MakeDoorElement keeps the names in sync

Elements/test/DoorTest.cs line 22 at r1 (raw file):

            var wall = new StandardWall(line, 0.1, 3.0);
            var door = new Door(wall.CenterLine, 0.5, 2.0, 2.0, Door.DOOR_THICKNESS, DoorOpeningSide.LeftHand, DoorOpeningType.SingleSwing);
            wall.AddDoorOpening(door);

can we make an assertion about the wall having an opening here.

anthonie-kramer

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @wynged)

Elements/src/Door.cs line 172 at r1 (raw file):

Previously, wynged (Eric Wassail) wrote…

this formatting looks pretty weird.

Good catch, I'll fix

Elements/src/Door.cs line 207 at r1 (raw file):

Previously, wynged (Eric Wassail) wrote…

in all of the methods that make representations it seems like we're always making a new RepresentationInstance for each door, but a big reason to move to RepresentationInstances is so that we can re-use instances. This will keep the glb model size down which makes function execution faster.

What is the expected process? Should we always create representations in the function? We could do this automatically by comparing elements when they are created and seeing if they have identical properties, and then re-using the representation. I'd like to keep this as simple as possible without requiring the user to manage representations.

Elements/src/Door.cs line 380 at r1 (raw file):

Previously, wynged (Eric Wassail) wrote…

in general we like to give classes and enums their own file. looks like these got merged in for some reason?

I was trying to de-clutter the Elements folder... I could put this into the Utilities folder... I think this might be where we should break out the BIM stuff into their own folders. I just didn't want Enum files in the src folder

Elements/src/StandardWall.cs line 122 at r1 (raw file):

Previously, wynged (Eric Wassail) wrote…

are these comments correct? does this adjustment fix the opening transform or not?

Done. Removed

Elements/test/DoorTest.cs line 17 at r1 (raw file):

Previously, wynged (Eric Wassail) wrote…

use nameof(MakeDoorElement) and change the name of this method to MakeDoorElement keeps the names in sync

Done.

Elements/test/DoorTest.cs line 22 at r1 (raw file):

Previously, wynged (Eric Wassail) wrote…

can we make an assertion about the wall having an opening here.

Done.

wynged

Reviewed 1 of 3 files at r2, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @anthonie-kramer)

Elements/src/Door.cs line 207 at r1 (raw file):

Previously, anthonie-kramer (Anthonie Kramer) wrote…

What is the expected process? Should we always create representations in the function? We could do this automatically by comparing elements when they are created and seeing if they have identical properties, and then re-using the representation. I'd like to keep this as simple as possible without requiring the user to manage representations.

great Q, I don't think we've really formalized the pattern, but here's a doc that Kate sent to me https://www.notion.so/hyparaec/Multiple-Representations-b8a5754229c940698c5fb73608e6bd8c?pvs=4
and here's a PR where I'm starting to do this for fittings. https://github.com/hypar-io/Elements/pull/1056/files#diff-cf24189f505ad5e3c1484c59c3aa75ffb8324341ce1a61717c47af0410a4b2f5
I opted for adding a hashing method on the class that helped determine if something should be re-used. if you like this pattern we can discuss with kate if this could be elevated to "infrastructure"

Elements/src/Door.cs line 380 at r1 (raw file):

Previously, anthonie-kramer (Anthonie Kramer) wrote…

I was trying to de-clutter the Elements folder... I could put this into the Utilities folder... I think this might be where we should break out the BIM stuff into their own folders. I just didn't want Enum files in the src folder

ah, well a folder sounds fine to me! you could make one for 'enums' or for 'doors' or .... you could start the BIM folder? I think having separate files makes code much easier to navigate though.

anthonie-kramer

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @wynged)

Elements/src/Door.cs line 207 at r1 (raw file):

Previously, wynged (Eric Wassail) wrote…

great Q, I don't think we've really formalized the pattern, but here's a doc that Kate sent to me https://www.notion.so/hyparaec/Multiple-Representations-b8a5754229c940698c5fb73608e6bd8c?pvs=4
and here's a PR where I'm starting to do this for fittings. https://github.com/hypar-io/Elements/pull/1056/files#diff-cf24189f505ad5e3c1484c59c3aa75ffb8324341ce1a61717c47af0410a4b2f5
I opted for adding a hashing method on the class that helped determine if something should be re-used. if you like this pattern we can discuss with kate if this could be elevated to "infrastructure"

Added a comment to the notion doc... Let me know if you want to discuss the solution in more detail and we can start building out a standard approach.

Elements/src/Door.cs line 380 at r1 (raw file):

Previously, wynged (Eric Wassail) wrote…

ah, well a folder sounds fine to me! you could make one for 'enums' or for 'doors' or .... you could start the BIM folder? I think having separate files makes code much easier to navigate though.

Done. Also started a BIM folder and updated the Doors namespace... we should migrate other BIM things into it in a future refactor.

…resentation-improvements-ak

anthonie-kramer

This should be the final version... until we find a generic solution for storage

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @wynged)

Elements/src/Door.cs line 207 at r1 (raw file):

Previously, anthonie-kramer (Anthonie Kramer) wrote…

Added a comment to the notion doc... Let me know if you want to discuss the solution in more detail and we can start building out a standard approach.

Added a storage class and provided some additional updates.

wynged

just one things needs to be renamed :)

Reviewed 7 of 8 files at r3, 10 of 10 files at r4, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @anthonie-kramer)

Elements/src/BIM/Door/DoorRepresentationStorage.cs line 9 at r4 (raw file):

    static class DoorRepresentationStorage
    {
        private static readonly Dictionary<string, List<RepresentationInstance>> _fittings = new Dictionary<string, List<RepresentationInstance>>();

rename to not be _fittings

Elements/src/Door.cs line 207 at r1 (raw file):

Previously, anthonie-kramer (Anthonie Kramer) wrote…

Added a storage class and provided some additional updates.

done lgtm

anthonie-kramer

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @wynged)

Elements/src/BIM/Door/DoorRepresentationStorage.cs line 9 at r4 (raw file):

Previously, wynged (Eric Wassail) wrote…

rename to not be _fittings

Done.

wynged

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: complete! 1 of 1 approvals obtained

srudenkoamc and others added 30 commits October 27, 2023 11:33

Added summaries and improved Door constructors

fa81fc2

Removed OriginalPosition property

5bddae2

Removed redundant constructor

419f7c0

Fixed Door deserialization issue

7196aa2

- Fixed Door deserialization issue - Door can be deserialized now; - Removed 'Wall' property; - Added 'isElementDefinition' property to constructors; - Added DoorTest.

Move Door related staff to separate folder

8dc16fb

Moved solid representation creation logic to a separate class

438932f

The representation is a RepresentationInstance now instead of Representation.

Moved curve representation to DoorRepresentationFactory

1295d25

- Moved curve representation to DoorRepresentationFactory; - Added curve representation to UpdateRepresentations.

Merge branch 'hypar-io:master' into door-representation-improvements

b1f3bd2

Merge branch 'door-representation-improvements' of https://github.com…

6d90fea

…/srudenkoamc/Elements into door-representation-improvements

Use DoorRepresentationProvider instead of static DoorRepresentationFa…

ebd35b5

…ctory

Merge branch 'master' into door-representation-improvements

7a3999a

Moved Door related classes back to Elements

20c134f

Merge branch 'door-representation-improvements' of https://github.com…

b2955d3

…/srudenkoamc/Elements into door-representation-improvements

Added dictionary that stores created representations

43a9602

Merge branch 'master' into door-representation-improvements

9f71429

Reverted adding of RepresentationProvider class

2ca2fb4

Merge commit '6d1006514e46fa15e5214b0a4d5c96a10f1c26e2' into HEAD

c3640b7

updates for frame and opening

93d5af2

Merge remote-tracking branch 'upstream/master' into door-representati…

8009b04

…on-improvements

Update IFCTests.cs

5b8de09

Merge branch 'master' into door-representation-improvements

438b5e1

Merge branch 'master' into door-representation-improvements

92ad64f

Move Door schematic visualization to a separate class

d7a673f

Split frame representation from solid representation

671e4e4

Added Thickness property to Door class

c2a2610

Fixed an issue with Opening

599889e

- Method CreateDoorOpening is used now instead of Opening property; - Removed opening properties from constructor; - Fixed issue in Opening class; - Fixed several small issues in door opening size computation.

Update DoorTest.cs

9e004b5

Fixed door ifc test

6410b9d

Fixed IFCTests

5fa8f81

The tests failed because doors don't have Opening members anymore.

Added AddDoorOpening method to StandardWall class

f3ebe0a

anthonie-kramer added 3 commits November 15, 2023 10:14

refactoring during merge

534ebcf

removals

282075f

door refactor

cef84bb

anthonie-kramer requested a review from wynged November 15, 2023 21:56

wynged requested changes Nov 16, 2023

View reviewed changes

format comments

a5febf9

anthonie-kramer commented Nov 16, 2023

View reviewed changes

wynged requested changes Nov 16, 2023

View reviewed changes

starting BIM folder and refactor Door namespace

209214b

anthonie-kramer commented Nov 17, 2023

View reviewed changes

anthonie-kramer added 5 commits November 17, 2023 13:04

storage

45e7095

no bim

92a342f

Merge commit '7db39fc49371157a3118840f8e22063d24fe3992' into door-rep…

43cf11d

…resentation-improvements-ak

revert BIM namespace

af64a8c

handles n stuff

f3e9129

anthonie-kramer commented Nov 17, 2023

View reviewed changes

wynged requested changes Nov 20, 2023

View reviewed changes

rename

349d095

anthonie-kramer commented Nov 20, 2023

View reviewed changes

wynged approved these changes Nov 20, 2023

View reviewed changes

anthonie-kramer added 2 commits November 20, 2023 11:00

Merge master into door-representation-improvements-ak

4640a78

test update

f4e1965

anthonie-kramer merged commit f7525d2 into master Nov 20, 2023
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Door-representation-improvements-ak #1057

Door-representation-improvements-ak #1057

anthonie-kramer commented Nov 15, 2023 •

edited

Loading

wynged left a comment

anthonie-kramer left a comment

wynged left a comment

anthonie-kramer left a comment

anthonie-kramer left a comment

wynged left a comment

anthonie-kramer left a comment

wynged left a comment

Door-representation-improvements-ak #1057

Door-representation-improvements-ak #1057

Conversation

anthonie-kramer commented Nov 15, 2023 • edited Loading

wynged left a comment

Choose a reason for hiding this comment

anthonie-kramer left a comment

Choose a reason for hiding this comment

wynged left a comment

Choose a reason for hiding this comment

anthonie-kramer left a comment

Choose a reason for hiding this comment

anthonie-kramer left a comment

Choose a reason for hiding this comment

wynged left a comment

Choose a reason for hiding this comment

anthonie-kramer left a comment

Choose a reason for hiding this comment

wynged left a comment

Choose a reason for hiding this comment

anthonie-kramer commented Nov 15, 2023 •

edited

Loading