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

Graveyard updates #855

Merged
merged 9 commits into from
Mar 21, 2023
Merged

Graveyard updates #855

merged 9 commits into from
Mar 21, 2023

Conversation

pshriwise
Copy link
Member

@pshriwise pshriwise commented Feb 13, 2023

Description

These changes update some behavior of the DagMC::create/remove_graveyard function.

create_graveyard

  • create_graveyard can now create a graveyard without acceleration data structures using the vertices of the model to create a bounding box as a basis for the graveyard volume.
  • create_graveyard will create an implicit complement if none exists so the graveyard volume can be inserted into the model topology correctly.
  • create_graveyard will only (re)construct acceleration data structures for the new graveyard volume and the implicit complement if these data structures were present when the method was initially called.

remove_graveyard

There's a bug when calling create_graveyard with overwrite == true when a graveyard is not present. Previously, this would fail when trying to remove a graveyard that isn't present. This adds a check at the beginning of remove_graveyard to simply return MB_SUCCESS if no graveyard is present.

Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

Thanks @pshriwise - just a couple of maintainability questions

src/dagmc/DagMC.cpp Show resolved Hide resolved
src/dagmc/DagMC.cpp Outdated Show resolved Hide resolved
@makeclean
Copy link
Contributor

Should we ever replace an existing graveyard? Should we just not add another to the set?

@pshriwise
Copy link
Member Author

pshriwise commented Feb 13, 2023

Should we ever replace an existing graveyard? Should we just not add another to the set?

I can see someone wanting to replace a spherical shell graveyard in case they're learning DAGMC and didn't realize how many triangles this might add to their model without having to re-tessellate the entire thing. Granted, some kind of utility program in the repo would be better than writing it yourself own, which is what they'd have to now.

@gonuke
Copy link
Member

gonuke commented Feb 16, 2023

I'm ready to merge but want to give @makeclean a chance to either withdraw or amplify his concern?

Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

Only one typo in a comment....

src/dagmc/DagMC.cpp Outdated Show resolved Hide resolved
Co-authored-by: Paul Wilson <paul.wilson@wisc.edu>
@gonuke
Copy link
Member

gonuke commented Mar 21, 2023

Thanks @pshriwise

@gonuke gonuke merged commit a3f5936 into svalinn:develop Mar 21, 2023
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.

3 participants