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

Ifc serialization improvements #1021

Merged

Conversation

srudenkoamc
Copy link
Contributor

@srudenkoamc srudenkoamc commented Sep 7, 2023

BACKGROUND:

  • IFC serialization needs improvement.

DESCRIPTION:

  • Massive refactoring of IFC serialization.
  • Changes from Everything ifc #531 are included.
  • Recognition of core Hypar Element types from original IFC serialization is preserved.
  • Added fallback conversion to GeometricElement in case if IfcBuildingElement is not supported.

FUTURE WORK

  • Representations extraction should be revised after multiple representations concept implementation.
  • For the most types only extrude and mapped representations are supported. But each IFC type supports it's own list of Geometric Representations. They may get support in the future.
  • Each IfcRepresentationItem has it's own maretial, but Element can have only one material. It may change after multiple representations concept is implemented.
  • There are many door operation types that aren't supported now.
  • IFC property sets and IFC quantity sets may be used for extraction of some properties.
  • IfcBooleanClippingResultParser doesn't apply boolean clipping operation.
  • German names aren't extracted properly (see 'AC20-Institute-Var-2.ifc' for example).
  • IFC files for "rac_sample" and "rme_sample" tests cannot be read because of some errors during file parsing in Document class.
  • The original representation of an object won't be preserved, if it is converted to one of the core types (not Geometric Element). Instead the representation from 'UpdateRepresentation()' method will be used.

TESTING:

  • Existing tests are improved in order to observe the effect of changes.

This change is Reviewable

srudenkoamc and others added 30 commits August 29, 2023 15:41
Also made extensions for basic geometric elements conversion (like vectors or transforms) internal instead of private.
The logic is taken from IFCExtensions mostly without changes. The only thing was excluded is openings processing. The openings will be processed separately.
The logic for the new class and the changes in IFCExtensions are taken from hypar-io#531 PR.
No logic changes, just namespaces structure simplification.
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.

Everything looks good. I did some testing, and found a bug with the current version of the Hypar.IFC4 package, so I'm working on an update to downgrade it to net6.0, since we don't use net7.0 anywhere else. Would like to get that working before we merge this work in.

The code is here if you want to take a look at it. Feel free to review and make any other suggestions that would be helpful for these changes before I publish a new Nuget package!

https://github.com/hypar-io/IFC-gen

Reviewed 4 of 19 files at r2, 5 of 36 files at r5, 4 of 19 files at r6, 1 of 22 files at r10, all commit messages.
Reviewable status: 0 of 1 approvals obtained

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 1 of 19 files at r2, 13 of 36 files at r5, 5 of 19 files at r6, 1 of 3 files at r8, 19 of 22 files at r10, all commit messages.
Reviewable status: 0 of 1 approvals obtained (waiting on @srudenkoamc)


Elements/src/Utilities/SolidOperationUtils.cs line 101 at r10 (raw file):

            if (voids.Count == 1 && solids.Count == 0)
            {
                csg = voids.First();

Make sure that this is used for subtraction.


Elements.Serialization.IFC/src/IFCToHypar/Converters/FromIfcDoorConverter.cs line 66 at r10 (raw file):

            var matchingWalls = allWalls.Where(w => w.Id.Equals(IfcGuid.FromIfcGUID(ifcWall.GlobalId)));

            return matchingWalls.Any() ? matchingWalls.First() : null;

FirstOrDefault()


Elements.Serialization.IFC/src/IFCToHypar/RepresentationsExtraction/MaterialExtractor.cs line 73 at r10 (raw file):

                                Material material;
                                if (!MaterialByName.ContainsKey(name))

TryGetValue


Elements.Serialization.IFC/src/IFCToHypar/FromIfcModelProvider.cs line 119 at r10 (raw file):

                {
                    _constructionErrors.Add(ex.Message);
                    continue;

Looks like this continue can be removed.


Elements.Serialization.IFC/src/IFCToHypar/FromIfcModelProvider.cs line 150 at r10 (raw file):

                // - Idea: Make PropertySet an Element. PropertySets can store type properties.
                GeometricElement definition;
                if (_elementDefinitions.ContainsKey(repData.MappingInfo.MappingId))

if(!TryGetValue()) is better here


Elements.Serialization.IFC/src/IFCToHypar/FromIfcModelProvider.cs line 197 at r10 (raw file):

            {
                var ifcElement = _elementToIfcProduct[elementWithOpenings];
                var ifcOpeningElements = ifcOpenings.Where(v => v.RelatingBuildingElement == ifcElement)

Looks like all ToList can be removed in this function as Linq can just chain together.


Elements.Serialization.IFC/src/IFCToHypar/IFCExtensions.cs line 59 at r10 (raw file):

            var extrudes = opening.RepresentationsOfType<IfcExtrudedAreaSolid>().ToList();

            if (extrudes?.Count == 0)

If extrudes is null return will not happen and next foreach with throw. It needs either != null or remove ? to show that null is not expected here.


Elements.Serialization.IFC/src/IFCToHypar/IFCExtensions.cs line 163 at r10 (raw file):

            {
                var p = Polygon.Rectangle((IfcLengthMeasure)rect.XDim, (IfcLengthMeasure)rect.YDim);
                var t = new Transform(rect.Position.Location.ToVector3());

Why here transform applied but not in other places? Why IfcCircleProfileDef is created as Circle here but as Polygon above?


Elements.Serialization.IFC/src/IFCToHypar/IFCExtensions.cs line 241 at r10 (raw file):

            {
                var p = Polygon.Rectangle((IfcLengthMeasure)rect.XDim, (IfcLengthMeasure)rect.YDim);
                var t = new Transform(rect.Position.Location.ToVector3());

Same as above


Elements.Serialization.IFC/src/IFCToHypar/IFCExtensions.cs line 314 at r10 (raw file):

                        foreach (IfcInteger segmentIndex in (List<IfcPositiveInteger>)ai)
                        {
                            segmentIndices.Add(segmentIndex - 1);

Note that we support exactly 2 indices for line and 3 for arc.


Elements.Serialization.IFC/src/IFCToHypar/IFCExtensions.cs line 378 at r10 (raw file):

            for (var i = 0; i < point.Coordinates.Count; i++)
            {
                if (point.Coordinates[i] != other.Coordinates[i])

If coordinates are floating point - there can be risk of misinterpreting two points as different.


Elements.Serialization.IFC/src/IFCToHypar/Converters/FromIfcSpaceConverter.cs line 41 at r10 (raw file):

            }

            var solid = repData.SolidOperations.First()?.Solid;

First() throws if no items, use FirstOrDefault()

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 (waiting on @srudenkoamc)


Elements.Serialization.IFC/src/IFCToHypar/IFCExtensions.cs line 93 at r10 (raw file):

        }

        // private static IfcOpeningElement ToIfcOpeningElement(this Opening opening, IfcRepresentationContext context, Document doc, IfcObjectPlacement parent)

Is whole commented function needed?

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.

Reviewed 1 of 22 files at r10, 6 of 7 files at r11, 2 of 2 files at r12, 1 of 1 files at r13, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @srudenkoamc)


Elements/src/Door.cs line 86 at r13 (raw file):

            Representation = representation ?? new Representation(new List<SolidOperation>() { });
            Opening = new Opening(Polygon.Rectangle(width, height), depthFront, depthBack, GetOpeningTransform());
            Id = id != default ? id : Guid.NewGuid();

was this due to a serialization issue? I believe we can also create an empty constructor which may also fix the serialization issues with newtonsoft

@srudenkoamc
Copy link
Contributor Author

Reviewed 1 of 22 files at r10, 6 of 7 files at r11, 2 of 2 files at r12, 1 of 1 files at r13, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @srudenkoamc)

Elements/src/Door.cs line 86 at r13 (raw file):

            Representation = representation ?? new Representation(new List<SolidOperation>() { });
            Opening = new Opening(Polygon.Rectangle(width, height), depthFront, depthBack, GetOpeningTransform());
            Id = id != default ? id : Guid.NewGuid();

was this due to a serialization issue? I believe we can also create an empty constructor which may also fix the serialization issues with newtonsoft

This is a fix for a situation when a door has the default Guid instead of a generated one when a Guid isn't specified in the constructor parameters. The fix for the serialization issue is in the another PR (#1050).

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 I have updated the IFC Nuget package so we should be good to finish this PR... Let me know when it's ready to go!

Reviewed 4 of 4 files at r14, all commit messages.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @srudenkoamc)

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: ... It looks like all the comments were addressed... If there are no more updates I will merge!

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

@srudenkoamc
Copy link
Contributor Author

srudenkoamc commented Nov 8, 2023

:lgtm: ... It looks like all the comments were addressed... If there are no more updates I will merge!

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

No updates from me. The changes may be merged.

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.

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

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.

Reviewed 1 of 19 files at r2, 8 of 36 files at r5, 1 of 19 files at r6, 1 of 3 files at r8, 15 of 22 files at r10.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @srudenkoamc)

@anthonie-kramer anthonie-kramer merged commit 6d10065 into hypar-io:master Nov 8, 2023
2 checks passed
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.

3 participants