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

Physical_Engine: Make physical surfaces return material and volume of all content #3008

Conversation

IsakNaslundBh
Copy link
Contributor

Issues addressed by this PR

Closes #2895

Updates so that Physical ISurface objects account for inner opening type objects, such as doors and walls, for computation of SolidVolume as well as MaterialComposition.

  • Add method for VolumetricMaterialTakeoff
  • Make MaterialComposition method rely on new method
  • Make SolidVolume rely on the new method

Important that this is changing behaviour, and would impact workflows like LCA, but should be to the better. Important that people using the tools are aware of this. @enarhi when review/merged would you be able to help spread this?

Also, smaller change, is I removed all errors/warnings regarding Void type openings, as imo, it is safe to simply return 0 volume/empty material composition for this type of object, given it by definition is meant to model nothingness.

Test files

Testfile from issue:

Test File

Also good ofc if this can be tested through a few cases.

Changelog

Additional comments

Method could potentially be versioned over to the methods in Matter_Engine, which does the same thing, but wanted to change as little across engines as possible.
…f method

Method could potentially be versioned over to the methods in Matter_Engine, which does the same thing, but wanted to change as little across engines as possible.
@IsakNaslundBh IsakNaslundBh added the type:bug Error or unexpected behaviour label Feb 23, 2023
@IsakNaslundBh IsakNaslundBh self-assigned this Feb 23, 2023
@IsakNaslundBh IsakNaslundBh changed the title Physical engine: Make physical surfaces return material and volume of all content Physical_Engine: Make physical surfaces return material and volume of all content Feb 23, 2023
@IsakNaslundBh
Copy link
Contributor Author

@enarhi if you want this to make the Beta, we need it reviewed kind of ASAP.

enarhi
enarhi previously approved these changes Mar 8, 2023
Copy link
Member

@enarhi enarhi left a comment

Choose a reason for hiding this comment

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

LGTM, tested test file along with a few other use cases and results are consistent and as expected per compliance with other similar calc methods.

@enarhi
Copy link
Member

enarhi commented Mar 8, 2023

@BHoMBot check compliance

@IsakNaslundBh
Copy link
Contributor Author

@BHoMBot check compliance
@BHoMBot check required

@bhombot-ci
Copy link

bhombot-ci bot commented Mar 9, 2023

@IsakNaslundBh to confirm, the following actions are now queued:

  • check code-compliance
  • check documentation-compliance
  • check project-compliance
  • check branch-compliance
  • check dataset-compliance
  • check copyright-compliance
  • check code-compliance
  • check documentation-compliance
  • check project-compliance
  • check core
  • check null-handling
  • check serialisation
  • check versioning
  • check installer

There are 69 requests in the queue ahead of you.

@bhombot-ci
Copy link

bhombot-ci bot commented Mar 9, 2023

The check code-compliance has already been run previously and recorded as a successful check. This check has not been run again at this time.

@IsakNaslundBh
Copy link
Contributor Author

@BHoMBot check compliance

@bhombot-ci
Copy link

bhombot-ci bot commented Mar 9, 2023

@IsakNaslundBh to confirm, the following actions are now queued:

  • check code-compliance
  • check documentation-compliance
  • check project-compliance
  • check branch-compliance
  • check dataset-compliance
  • check copyright-compliance

There are 4 requests in the queue ahead of you.

@IsakNaslundBh
Copy link
Contributor Author

@BHoMBot check required
@BHoMBot check unit-tests

@IsakNaslundBh
Copy link
Contributor Author

@enarhi had some silly description mistakes I had to fix. Do you mind re-approving?

@IsakNaslundBh IsakNaslundBh requested a review from enarhi March 9, 2023 14:55
@bhombot-ci
Copy link

bhombot-ci bot commented Mar 9, 2023

@IsakNaslundBh to confirm, the following actions are now queued:

  • check code-compliance
  • check documentation-compliance
  • check project-compliance
  • check core
  • check null-handling
  • check serialisation
  • check versioning
  • check installer
  • check unit-tests

There are 4 requests in the queue ahead of you.

@bhombot-ci
Copy link

bhombot-ci bot commented Mar 9, 2023

The check code-compliance has already been run previously and recorded as a successful check. This check has not been run again at this time.

@bhombot-ci
Copy link

bhombot-ci bot commented Mar 9, 2023

The check documentation-compliance has already been run previously and recorded as a successful check. This check has not been run again at this time.

@bhombot-ci
Copy link

bhombot-ci bot commented Mar 9, 2023

Please be advised that the check with reference 11883804830 has more than 50 annotations of notes. API limitations restrict annotations to 50. You may need to rerun this check to obtain the next set when you make changes. At the time of reporting this check, there are 70 additional annotations waiting, made up of 70 errors and 0 warnings.

enarhi
enarhi previously approved these changes Mar 9, 2023
Copy link
Member

@enarhi enarhi left a comment

Choose a reason for hiding this comment

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

LGTM, functionality all there as expected as per previous review.

@FraserGreenroyd
Copy link
Contributor

@BHoMBot check null-handling
@BHoMBot check serialisation

@bhombot-ci
Copy link

bhombot-ci bot commented Mar 9, 2023

@FraserGreenroyd to confirm, the following actions are now queued:

  • check null-handling
  • check serialisation

There are 5 requests in the queue ahead of you.

@IsakNaslundBh
Copy link
Contributor Author

IsakNaslundBh commented Mar 10, 2023

@BHoMBot check unit-tests

1 similar comment
@FraserGreenroyd
Copy link
Contributor

@BHoMBot check unit-tests

@bhombot-ci
Copy link

bhombot-ci bot commented Mar 10, 2023

@FraserGreenroyd to confirm, the following actions are now queued:

  • check unit-tests

@FraserGreenroyd
Copy link
Contributor

@BHoMBot check compliance

@bhombot-ci
Copy link

bhombot-ci bot commented Mar 10, 2023

@FraserGreenroyd to confirm, the following actions are now queued:

  • check code-compliance
  • check documentation-compliance
  • check project-compliance
  • check branch-compliance
  • check dataset-compliance
  • check copyright-compliance

There are 40 requests in the queue ahead of you.

@FraserGreenroyd
Copy link
Contributor

@BHoMBot check core
@BHoMBot check null-handling
@BHoMBot check serialisation
@BHoMBot check versioning

@bhombot-ci
Copy link

bhombot-ci bot commented Mar 10, 2023

@FraserGreenroyd to confirm, the following actions are now queued:

  • check core
  • check null-handling
  • check serialisation
  • check versioning

There are 2 requests in the queue ahead of you.

@IsakNaslundBh
Copy link
Contributor Author

@BHoMBot check installer

1 similar comment
@IsakNaslundBh
Copy link
Contributor Author

@BHoMBot check installer

@bhombot-ci
Copy link

bhombot-ci bot commented Mar 10, 2023

@IsakNaslundBh to confirm, the following actions are now queued:

  • check installer

There are 7 requests in the queue ahead of you.

Copy link
Contributor

@FraserGreenroyd FraserGreenroyd left a comment

Choose a reason for hiding this comment

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

Last commit is minor and not sufficiently changes @enarhi review following bot reports to reapproving for merge.

@IsakNaslundBh
Copy link
Contributor Author

@BHoMBot check ready-to-merge

@bhombot-ci
Copy link

bhombot-ci bot commented Mar 10, 2023

@IsakNaslundBh to confirm, the following actions are now queued:

  • check ready-to-merge

There are 4 requests in the queue ahead of you.

@IsakNaslundBh IsakNaslundBh merged commit 7558747 into develop Mar 10, 2023
@IsakNaslundBh IsakNaslundBh deleted the Physical_Engine-#2895-MakePhsyicalSurfacesRetunMaterialAndVolumeOfAllCOntent branch March 10, 2023 14:01
@bhombot-ci bhombot-ci bot mentioned this pull request Mar 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Error or unexpected behaviour
Projects
None yet
3 participants