buildingSMART / IFC4.3.x-development

Repository to collect updates to the IFC4.3 Specification
Other
148 stars 74 forks source link

Concept: Property Sets for Materials - how to report mistakes? #557

Open pjanck opened 1 year ago

pjanck commented 1 year ago

Looking at: docs/templates/Object Definition/Property Sets/Property Sets for Materials/README.md

I wonder how to report about found mistakes in an IFC file. Other ConceptTemplates use IfcRoot-based applicableEntity, which means that a GUID can be queried. What to do here?

It seems that this is a mistake in the modelling. @aothms @TLiebich @berlotti @SergejMuhic @MatthiasWeise

aothms commented 1 year ago

I think it's generally understood that in hindsight it was not ideal not to make IfcMaterial and IfcProfileDef rooted entities.

For now there is not a lot we can do until we allow ourselves these kind of backwards incompatible changes. Or in what kind of solution direction are you looking?

For reporting I would simply take the string attribute values from the instance and maybe also report the numeric instance id.

SergejMuhic commented 1 year ago

I think it's generally understood that in hindsight it was not ideal not to make IfcMaterial and IfcProfileDef rooted entities.

Really? How so? I cannot recall hearing something like that from others. Actually, hearing this the first time. I also do not see a big problem with how it is atm. It might even cause more problems than resolve. Suddenly, profiles and materials could be nested, aggregated, associated etc., not to start with how profiles could not be referenced by sweeps and so on. Not saying it is bad, but requires a lot more investment into documentation, resolving usages and implementation. Essentially, opening a can of worms.

For now there is not a lot we can do until we allow ourselves these kind of backwards incompatible changes. Or in what kind of solution direction are you looking?

I do not see this as the issue here. The definition of the template is. It should be a partial template and assigned to Material Association. This is how the rest of the non rooted ConceptTemplate.applicableEntity entries were handled.

pjanck commented 1 year ago

it was not ideal not to make IfcMaterial and IfcProfileDef rooted entities

IMHO, I think it was a good decision for them to not be rooted, because neither of those can be self-sufficient instances in an IFC dataset. I can't imagine a scenario where it would make sense to have a parameterized T-Profile with a unique ID. To distinguish it from what? (We don't need to start a discussion on this topic here.)

Or in what kind of solution direction are you looking?

I propose to make the ConceptTemplate .isPartial = true, similar to the ConceptTemplate for IfcPropertySingleValue. (As @SergejMuhic suggests.) I think the suggestions are not generally applicable:

the string attribute values from the instance

Well, if the check tries to verify that these values are present, and fails, one would report empty strings. While this would be a good workaround for some cases, it does not cover all scenarios.

numeric instance id

Where can I find that in ifcXML or ifcJSON? This is an EXPRESS-specific solution that does not translate well to other data formats.

TLiebich commented 1 year ago

in my understanding, the original question was, why the concept template for "Property Sets for Matarial" is not marked as a partial template .isPartial = true ? It should be. In the documentation it should be noted that this partial template expands the other partial templates on "4.7.4.1 Material".

One side note - in the HTML documentation: the information, that it is a partial template is not rendered at all (at least I could not see it). It would be great, it it could be shown as a tag underneath the concept name. Also - the whole tree of partial templates is now alphabetically ordered at 4.7.x (and not at the end of all templates as in IFC 4.0, showing it as being specific - i.e. partial).

aothms commented 1 year ago

why the concept template ... is not marked as a partial template .isPartial = true ? It should be.

Well, we reviewed it at that time and thought it was appropriate https://github.com/buildingSMART/IFC4.3.x-development/pull/400 It's the most backwards compatible as previously these entries were (erroneously) under psets for objects. We have harmonized the wording and even added a note specifically on this:

NOTE: An IfcMaterial is not an IfcObject. For legacy reasons, this concept is listed under Object Definition. In future versions of the standard the various property set association mechanisms will likely be unified.

One central question for new users, developers, everybody is: "what types can I extend with user-defined data?". Currently that's conveniently grouped under 4.6.2 Property Sets, as everybody came to know them. And now we're discussing changes based on topics that only a handful of people can relate to such as "partial templates". I'm all for consistency, but aren't we doing here a disservice to the readability of the standard?

@pjanck and is this really a step in the right direction for readability of reporting? Sure you get a guid back (of the 1000 elements that happen to use that material). It can be quite a puzzle to trace it back the source of the error in a specific material through the maze of templates and relationships, especially in case of material sets such as http://ifc43-docs.standards.buildingsmart.org/IFC/RELEASE/IFC4x3/HTML/concepts/Object_Association/Material_Association/Material_Constituent_Set/content.html

Also - the whole tree of partial templates is now alphabetically ordered at 4.7.x

I agree, has been an annoyance for me as well. I'll readd it to my list.

SergejMuhic commented 1 year ago

https://github.com/buildingSMART/IFC4.3.x-development/pull/400#discussion_r817957998

Like I said in this comment, it is hard for me to comment on the definition because I have no idea what it means. I was actually asking about the partial template references because I did not understand how that is encoded and how it is supposed to work. I should have been more observant of the path of the template since it is not in the partial chapter but other than that, hard to decode what is a partial and what not. Come to think of it, @MatthiasWeise mentioned this template to me once before.

Going back to the original issue, it is not about the readability but actual implementation. Templates are not supposed to be merely human readable documentation but also machine readable for feeding an implementation of checking algorithms. As an example, a usage instantiated from that template cannot be consistently added into a BCF file due to not having an IFC Guid available. Sure, hacking is always possible but that is not really production code and what I would refer to as an "implementable standard".

MatthiasWeise commented 1 year ago

I would also argue that property definition of IfcMaterialshould be in the partial template section. I suggest to be a child of 4.7.4 Material Definition, which in my view should be an empty super template of possible material functionalities (definition/identification, properties, style). The empty super template 4.7.4 Material Definition is then to be used by sub templates of 4.2.6 Material Association. What we would also need is an MVD scope setting on partial template level, which would allow to (globally) switch on/off specific functionalities supported in different views.

Use of those partial templates for model checking and proper reporting in BCF (using a GUID) is another topic. Technically a partial template could well be used to define a global check for material properties. Tracing all Root-based objects using this material is one option for error reporting. Another alternative might be to use the GUID of IfcProjectto report about global violation of IfcMaterialrequirements