-
Notifications
You must be signed in to change notification settings - Fork 18
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
IVS-197 - Fix / GEM004 - IfcTopologyRepresentation #319
IVS-197 - Fix / GEM004 - IfcTopologyRepresentation #319
Conversation
Ghesselink
commented
Oct 31, 2024
- Consider valid types of IfcTopologyRepresentation as opposed to IfcShapeRepresentation
- Add testfiles for topology representation, one per schema
- Organize valid types and identifiers per schema in resource folder
- Version bump GEM004 & GEM052 (?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, but as said in my last comment, I think this alludes to some bigger inconsistency issues regarding topology representation that we should get a better feel for first.
@@ -24,8 +24,8 @@ Reference: https://github.com/buildingSMART/Sample-Test-Files/issues/137. | |||
|
|||
Examples: | |||
| schema | source | | |||
| IFC4.3 | valid_RepresentationIdentifier_IFC4.3.csv | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case we can just remove the outline. and start with the same given as above:
Given a model with Schema "IFC4.3" or "IFC4"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true. I think we can even remove the schema selection from the Given statements; they are already selected in the python implementation. So then, we'll simply have three scenario's;
- Shape representation identifiers
- Shape representation types
- Topology representation types
Something like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why this is in the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's from PJS002 and uses the same gherkin implementation as GEM004 (the current rule) and GEM052.
The gherkin impl won't find resources/valid_ProjectDeclaration.csv
anymore because it'll always look for resourses/schema/csv_file.csv.
I thought to duplicate the csv and move it to the folder of each schema would be clearer than doing tricks in the gherkin impementation or split them up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like there is still a bit of an open question here. For these items in topologyrepresentation, which ContextType/Identifier are supposed to be used in Ifc(Geometric)RepresentationContext?
https://ifc43-docs.standards.buildingsmart.org/IFC/RELEASE/IFC4x3/HTML/annex_e/structural-analysis-model/structural-curve-member.html doesn't use subcontexts, so also doesn't pass our rules, but I don't think there is a reasonable geometric item that fits these topologies.
I think we should try and get structural-curve-member.ifc to be meaningful and valid for all geometric contexts checks before we can make a final decision here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For these items in topologyrepresentation, which ContextType/Identifier are supposed to be used in Ifc(Geometric)RepresentationContext?
ContextIdentifier
should be 'Reference' per CT 4.1.7.5.1 which also corresponds to the value for RepresentationIdentifier
used for IfcTopologyRepresentation in the example model.
As you've noted, IfcTopologyRepresentation does not have to be (but may be) in a GeometricRepresentationContext. So IfcRepresentationContext
and IfcGeometricRepresentationContext
are both valid.
IfcGeometricRepresentationContext lists the recognized values of ContextType
as 'Model', 'Plan', and 'NotDefined'.
Requiring 'Model' as the ContextType for topology representations seems to be the best fit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not completely sure I understand, but I've added a modified ifc file to the unit tests that;
- Passes the geometric contexts checks (GEM052)
- Uses 'Reference' as Identifier
- Uses 'Model' as Type
- We can hopefully use to derive more testfiles for afterwards
should be 'Reference' per CT 4.1.7.5.1
I read 'Structural activities may
have a 'Reference' '. 'May' is rather vague.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now that we have an explicit file for TopologyRepresentationType
this should be renamed to ShapeRepresentationType
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commits speak louder than words
Thoughts?