Fix: Issues with material quantities solved

[ks-cph]

Description & motivation

Solves issue #2117 and improves the calculation of material quantities.

There are some problems with obtaining quantities in Revit:

1. RevitAPI GetMaterialIds does not return all materials
2. Computation of area not clear
3. Some categories need further evaluation

Changes:

• Objects/Converters/ConverterRevit/ConverterRevitShared/ConversionUtils.cs get the revit material from MEP elements
• Objects/Converters/ConverterRevit/ConverterRevitShared/Partial Classes/ConvertMaterialQuantities.cs get materials for MEPCurves and compute the quantities based on the geometry
• Objects/Converters/ConverterRevit/ConverterRevitShared/Partial Classes/ConvertMaterialQuantities.cs compute the quantities based on the geometry instead of the Revit API default methods. This is currently only implemented for windows, doors, and railings, but other categories may follow.
• Objects/Converters/ConverterRevit/ConverterRevitShared/Partial Classes/ConvertMaterialQuantities.cs compute the length of railings using the path length
• Objects/Converters/ConverterRevit/ConverterRevitShared/Partial Classes/ConvertMaterialQuantities.cs the new method GetGeometry is based on existing code

https://github.com/specklesystems/speckle-sharp/blob/15f0a8c516b19ae86a6777bd68fcafb2a9ebf29a/Objects/Converters/ConverterRevit/ConverterRevitShared/Partial%20Classes/ConvertMeshUtils.cs#L41-78

Validation of changes:

The changes have been tested extensively with the default Revit model.

• for MEPCurves: Pipse, ducts
• for railings
• windows, doors

Checklist:

• My pull request follows the guidelines in the Contributing guide?
• My pull request does not duplicate any other open Pull Requests for the same update/change?
• My commits are related to the pull request and do not amend unrelated code or documentation.
• My code follows a similar style to existing code.
• I have added appropriate tests.
• I have updated or added relevant documentation.

[connorivy]

Hey @ks-cph,

This is an interesting way of getting materials quantities. We're currently testing for a release, so I won't be able to merge in new functionality for a little while. One quick question for you, does this PR remove the need for #2655?

[ks-cph]

Hey @connorivy,

thanks for the quick feedback. This PR extends the work started in #2655. As this PR is more complex and may require further discussion, it'd be great if we could handle #2655 independently.

[connorivy]

Could you explain a bit why you are using these specific clauses to determine if an object needs geometry computation? Were these just the elements you ran into that needed it? If so, how did you know these elements needed it

[ks-cph]

Hi @connorivy

thanks for the feedback. I am using this specific categories to start with. I am quite sure that there are others but in this way they can be easily added. I introduced the issue previously that the Revit API handles the computation of the areas differently. While for walls, the default computation returns the area of the largest surface (or the area along the LocationCurve multiplied with the height), for other categories, the default method returns the sum of all surfaces.

For now, I chose Windows, doors railing and top rail as those are the ones where we noticed the inconsistency. I am working on a robust testing method where I am comparing the computed quantities to the expected quantities. This should give an overview.

I know that these categories are inconsisten as we are testing the quantities for our use cases.

[connorivy]

Here it looks like you aren't filtering the solids to see if the MaterialElementId == materialId. Could this result in sending incorrect data? Assuming that the material is something that it isn't?

[ks-cph]

Thanks for the very good question. I need to double-check this. I am not completely sure but I don’t see a problem as MEPCurves can only have 1 material. Also, I think that the MEPCurves do not have the material assigned on the solids. But I will verify this once more.

[connorivy]

Hey @ks-cph

I wanted to say thanks for teaching me a bit about MEP systems in Revit and for making these PRs. I merged in the first one because it added some good functionality. I'm okay with your answers to my above questions and I have one hangup that is preventing me from merging this PR.

Let's look at this duct. A diameter of 305mm and a length of 9300mm would give it a volume of 679,472,902.7 mm^3. This is pretty much exactly what your PR gives me as the materialVolume. The issue here is that the duct mesh is solid, but the actual duct is hollow. This means that this volume of material is going to be way too high. I really like the volume hack that you've come up with, but some objects, specifically MEP elements, I'm not sure this is a good way to handle this.

The non-MEP elements such as the railing are a bit more intriguing because theoretically this would work if the mesh is acurate, right? One additional question that I have about these types of elements is how we'd be able to separate parent materials from child materials.

For example, looking at the materialQuantities of this railing, how do we make sure that there isn't any overlap between the parent's material quantities, and the child's (the topRail property of the railing) materialQunatities. They both share the same material, so I'm not sure if that material is being double counted or not.

You've made several PRs dealing with materialQuantities so I am very curious what you think of my comments above. Also maybe fill me in a little more on what you're using materialQuantities for and that may help me understand the data that people actually want to see regarding these quantities.

[ks-cph]

Thanks for the excellent feedback. I am working on the material quantities for Real-Time LCA.

Let me start with the second comment. With the new implementation, the quantities are computed based on all solids belonging to an element. The TopRail is a different element with its own geometry, so the quantities are not double-counted despite sharing the same material.

In regards to the Ducts:
it is a very good comment, and I am unsure how to deal with it. While the pipe volume is computed for the hollow section, the volume for the ducts is calculated for the complete solid. The problem is that Revit does not provide any information on the thickness of a Duct. Revit does not know that the Ducts are hollow. The same applies to the implementation in Speckle. There is no property such as thickness.

The quantities can only be as accurate as the underlying data. So the current implementation is correct. The only option I see for tackling this issue is to expect the user to provide such a property. We can add a thickness property to Objects.BuiltElements.Duct and expect the user to provide it. If there is none, we can assume a default value. From Revit's perspective, this is rather difficult as we would need to introduce a new parameter.
What do you think?

[ks-cph]

Hi @connorivy,

Do you have more input on this? We'd highly appreciate this being included in the next release.

[teocomi]

Hey @ks-cph, jumping in here, thanks again for your PR!
We have a couple of perplexities that are holding us back from merging this:

• Providing incorrect volumes for ducts and pipes is risky and we would rather avoid it. If accuracy cannot be guaranteed, we would prefer if such calculations were done outside of Speckle. Speckle's role is to extract data from the model, not make assumptions about it.
• Calculating a mesh volume can be quite expensive, have you done any benchmarks to see how much this would affect large models?

[ks-cph]

Hey @teocomi,
Thanks for the response.

1. I understand your concerns. Speckle should only provide correct data. In the case of Pipes and Ducts, the proposed implementation does guarantee accurate data. In this specific case, the issue is that Revit does not represent the Ducts correctly. Still, the implementation is valid: the volumes are computed based on the underlying geometry, which is - applying human reasoning - inaccurate (as the Ducts should be hollow). But this fact is nowhere mentioned in the model. So, if you insist on only having accurate data in Speckle, you should consider not streaming Ducts generally, as Revit needs to provide the correct geometry. Please consider it and let me know if you see any option to include this approach. I think it's essential to go forward with the material quantities. Otherwise, we should consider removing them as the current implementation is unsatisfactory.
2. The proposed implementation does not compute volumes for meshes. Calculating it for a mesh would be computationally too expensive, I agree. Here, we only retrieve the volumes for solids, which is a property on the solid objects and, thus, available for streaming without new computations. Do you agree? I could run some tests, but we must find a solution for the first comment, as the second may be redundant.

[teocomi]

Hey @ks-cph let's have a quick chat about this: https://calendly.com/d/4bv-cpb-99q/15-min-chat

[teocomi]

Hey @ks-cph checking in to see if this is still an issue?

[ks-cph]

I have changed the code. Now, only Pipes and flex pipes return a volume and area. To point out that the quantities are not correctly calculated, the values are NaN instead of 0.

[martinromby]

Hi' @connorivy and @teocomi

@ks-cph committed a new solution to this PR last week addressing the previously discussed issues.
Have you had time to look at our contribution? We would highly appreciate this PR being merged into the next stable release.
Let us know if you want us to take any further actions on this PR.

Thanks :)

[teocomi]

Hey Martin, we'll get back to you on this next week! Sorry, we were at our retreat.

[connorivy]

Hello @ks-cph and @martinromby ,

We've discussed it and have decided that we're okay with sending incorrect material quantity values if that is what lives in Revit. However, you've made some recent changes that we don't want to accept. The main change that I'm referring to is narrowing the scope of the legit api calls to only be called for "HostObjects". Now the default materialQuantity retrieval method is to use the "from mesh" version that is more expensive and potentially incorrect. Also, I'm not sure if you are aware that MEPCurve element inherits from HostObject, so the next clause where you are checking if an element is a pipe is unreachable code.

In an attempt to make things a bit more performant, I took a stab at implementing the functionality that you are looking for here. Take a look at this branch and let me know if this does what you are looking for. https://github.com/specklesystems/speckle-sharp/tree/revit/connor/material-quantites-add-by-mesh

[ks-cph]

Hi Connor,
We are glad to hear that you are considering the implementation, and you are right; we made a mistake with the last commit.

The implemented solution on the branch mentioned would solve parts of our problems. However, it would not solve one of our primary concerns: GetMaterialArea() computes the quantities differently depending on the category. e.g. for a wall, it returns the area of the side, while for other categories, it returns the aggregated area of all faces.

So we would need more exceptions. As mentioned before, we added the exception for doors, windows, and railings for this reason.

[connorivy]

@ks-cph,

I was looking into this issue this morning and ran into the post that you made on the autodesk forum https://forums.autodesk.com/t5/revit-api-forum/method-getmaterialarea-appears-to-use-different-formulas-for/td-p/11988215.

The response from the developers is quite helpful, and I now see what you have been going for this whole time. Hearing directly from the development team about which elements return the area of a single face versus the area of all sides makes this functionality feel much more solid and less like a shot in the dark.

I made some addition adjustments to this branch and I think it is about what you are looking for https://github.com/specklesystems/speckle-sharp/tree/revit/connor/material-quantites-add-by-mesh. It is basically just your PR except I split up the methods in a way that helped me understand better what was going on. Let me know if we are on the same page like I think we are, or not.

Pay special attention to the very last method in the ConvertMaterialQuantities file, "MaterialAreaAPICallWillReportSingleFace". Hopefully what this method does is clear because of the name. Let me know if there are any other cases that need to be added

[ks-cph]

Hi @connorivy,
Thanks for the effort and the response. The code on the other branch looks very promising and targets our issues. Testing it on our models reveals only one remaining problem: PipeInsulation and DuctInsulation have, unlike the other MEPCurves, no materials assigned. The proposed implementation on the referenced branch would add the material quantities as null.

speckle-sharp/Objects/Converters/ConverterRevit/ConverterRevitShared/Partial Classes/ConvertMaterialQuantities.cs

Lines 51 to 56 in 7b3040c

	{
	DB.Material material = GetMEPSystemRevitMaterial(element);
	if (material == null)
	{
	return null;
	}

Could we call GetMaterialQuantitiesFromSolids if the MEP material is null?
Something like this:
https://github.com/ks-cph/speckle-sharp/blob/96c10e509bd1a55f82934c270bc832d6c03c968a/Objects/Converters/ConverterRevit/ConverterRevitShared/Partial%20Classes/ConvertMaterialQuantities.cs#L50-L65

[martinromby]

Hi @connorivy

Looks like this is a very promising solution and with the small additions from @ks-cph, regarding Pipe and Duct Insulation, it satisfies the current needs and closes the remaining issues.
Would this be something that could be added in a patch version to the 2.16.x @teocomi ?

[connorivy]

@ks-cph,

Ah okay, my assumption (and the assumption that this code makes) was that anything that was part of an MEP system would use the material of the MEP system. Can you think of other cases when this is not true.

The code addition that you proposed looks fine. One question is that you're using ElementId matId = GetMaterialsFromSolids(solids)?.First();. Are you confident that you would only want the first material found?

[ks-cph]

Hi @connorivy,
Thanks for the prompt response.

I checked this for all classes that inherit from MEPCurve (see the table below). Most classes do not define the MEPSystem. Retrieving them from the Solid works for all but not for Linings and Wires (they don't have a Solid). If you think that this is computationally too expensive, I can try to find another way of finding the material for these categories.

I cannot think of any element where there should be multiple solids in one element. But it would be correct to return a collection in case there are more.

Class	MEPSystem	Material on geometry
Autodesk.Revit.DB.Electrical CableTray	no	yes
Autodesk.Revit.DB.Electrical Conduit	no	yes
Autodesk.Revit.DB.Electrical Wire	no	No Solid
Autodesk.Revit.DB.Mechanical DuctInsulation	no	yes
Autodesk.Revit.DB.Mechanical DuctLining	no	no
Autodesk.Revit.DB.Plumbing PipeInsulation	no	yes
Autodesk.Revit.DB.Mechanical Duct	yes	yes
Autodesk.Revit.DB.Mechanical FlexDuct	yes	yes
Autodesk.Revit.DB MEPAnalyticalConnection	-	-
Autodesk.Revit.DB.Plumbing FlexPipe	yes	yes
Autodesk.Revit.DB.Plumbing Pipe	yes	yes

[connorivy]

Hey @ks-cph

Check thanks very much for looking into that, that info is very helpful. Check out the branch now. I just added another function "MaterialIsAttachedToMEPSystem" which just controls the route for which elements will have their materials extracted from the mep system. This will fix the issue with insulation because it will no longer use the MEP route. Let me know if this looks good

[ks-cph]

Hey @connorivy ,

Thanks again for your updates. This implementation should now fix the last issues.
We'd appreciate it if you could add this to a patch version of 2.16.x @teocomi