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

feat(revit): add density to material quantities #558

Conversation

bjoernsteinhagen
Copy link
Contributor

@bjoernsteinhagen bjoernsteinhagen commented Feb 4, 2025

Description

User Value

Material Quantities can be used to calculate object mass; however, in order to compute mass, users need to have density made available to them.

Changes:

  • Implemented StructuralMaterialAssetExtractor.cs to handle extraction of density from StructuralAsset
  • Since area and volume are scaled from internal units to model units, the same is done for density to avoid confusion
  • Display units added to the properties for clarity
  • density per StructuralAsset cached locally in the MaterialQuantitiesToSpeckle.cs
  • Since the current scope was limited to density ONLY, no need to create MaterialProxy as initially thought. Should the extracted parameters grow, this must be re-evaluated / refactored!
  • Note if anything on the extracting parameters from structural asset fails, the density will simply not be appended to the object. This isn't seen as a failure case.

Screenshots:

Before:

image

After:

image

Validation of changes:

Tested on various structural and architectural models for Revit versions 2022-2024:

Checklist:

  • My commits are related to the pull request and do not amend unrelated code or documentation.
  • I have added appropriate tests.
  • I have updated or added relevant documentation.

…dd-structural-and-thermal-properties-to-materials
- scaling internal units can't be so complicated?
- to discuss :(
- Simplification to just extract density from structural asset (if present)
- Just one property extraction => no need to create material proxy
- Scaled from internal units to match scaling to model units which occurs on other material quantities
…dd-structural-and-thermal-properties-to-materials
…dd-structural-and-thermal-properties-to-materials
Copy link

linear bot commented Feb 4, 2025

Copy link

codecov bot commented Feb 4, 2025

Codecov Report

Attention: Patch coverage is 0% with 61 lines in your changes missing coverage. Please review.

Project coverage is 18.58%. Comparing base (1c11e4a) to head (38c0eb0).
Report is 1 commits behind head on dev.

Files with missing lines Patch % Lines
...hared/ToSpeckle/Raw/MaterialQuantitiesToSpeckle.cs 0.00% 42 Missing ⚠️
...kle/Properties/StructuralMaterialAssetExtractor.cs 0.00% 18 Missing ⚠️
...ckle.Converters.RevitShared/ServiceRegistration.cs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #558      +/-   ##
==========================================
- Coverage   18.79%   18.58%   -0.22%     
==========================================
  Files         240      241       +1     
  Lines        4883     4939      +56     
  Branches      579      586       +7     
==========================================
  Hits          918      918              
- Misses       3934     3990      +56     
  Partials       31       31              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@KatKatKateryna KatKatKateryna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, including a nice fix for the units for Area and Volume.
From my limited knowledge of Revit materials the next comment might not make sence so correct me if I'm wrong:
Would it make sense to check "if (_converterSettings.Current.Document.GetElement(matId) is DB.Material material)" even before assigning Area and Volume? It seems for that step ".GetElement(matId)" is already expected to be a Material

@bjoernsteinhagen
Copy link
Contributor Author

Looks good to me, including a nice fix for the units for Area and Volume. From my limited knowledge of Revit materials the next comment might not make sence so correct me if I'm wrong: Would it make sense to check "if (_converterSettings.Current.Document.GetElement(matId) is DB.Material material)" even before assigning Area and Volume? It seems for that step ".GetElement(matId)" is already expected to be a Material

That's a good question. This was taken over from earlier definition. From what I know, the GetElement is not type specific and to access MaterialCategory and MaterialClass, we need to type cast to DB.Material and this is the safest way. But I'm not hundred percent sure on this.

@bjoernsteinhagen bjoernsteinhagen merged commit 4ee48bd into dev Feb 6, 2025
5 checks passed
@bjoernsteinhagen bjoernsteinhagen deleted the bjorn/cnx-1088-revit-add-structural-and-thermal-properties-to-materials branch February 6, 2025 07:49
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.

2 participants