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

Moving decompose logic out of CadToCsg.start to its own method #189

Closed

Conversation

shimwell
Copy link
Collaborator

Description

This PR moves some decomposition logic out of the start method

Fixes issue

partly helps with #184 and #114

Please link to any issues that this PR fixes

Checklist

  • I'm making a PR from a feature branch on my fork into GEOUNED-org/GEOUNED/dev branch
  • I have followed PEP8 style guide for Python or run a formatter such as black or ruff format on my code

@shimwell shimwell changed the title Moving decompose all out of cad to csg Moving decompose loginc out of CadToCsg.start to its own method May 22, 2024
@shimwell shimwell changed the title Moving decompose loginc out of CadToCsg.start to its own method Moving decompose logic out of CadToCsg.start to its own method May 22, 2024
Copy link
Member

@psauvan psauvan left a comment

Choose a reason for hiding this comment

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

This function cannot be extracted like this.
The function process_cones must be called after the full geometry including void is process, otherwise this function would have no meanings.

This functions was introduced because during simplification process some additional planes located at the cones apex to select one or the other sheet of the cone are removed.

@shimwell
Copy link
Collaborator Author

Thanks for letting me know, I shall move the process cones back to the end of the order.

Also looks like we need some more tests then as this is passing

@shimwell
Copy link
Collaborator Author

I've now moved the process cones section back to the original location.

Keen to fix the surfaces so that the ordering of steps does not change the outcome, something for the future

@@ -754,6 +688,77 @@ def start(self):
logger.info(f"Translation time of solid cells {tempTime1} - {tempTime0}")
logger.info(f"Translation time of void cells {tempTime2} - {tempTime1}")

def _decompose_geometry(self):
Copy link
Member

@psauvan psauvan May 23, 2024

Choose a reason for hiding this comment

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

This method must be split in 3 methods:

  • 1st : from line 698 to 704 (decomposition of standard solids, and enclosures)
  • 2nd: from line 702 to 742 (processing of geometry with solids made of facets and enclosures decomposition)
  • 3rd: from line 708 to 726 (conversion to solids bool definition) and 744 to 758 (conversion to enclosure bool definition)

the 3rd method must have a flag to tell if standard or facets geometry is processed.

  • if standard geometry execute:
    * conversion to solids bool definition
    * forceNoOverlapping (lines 728 and 729)
    * conversion to enclosure bool

  • if facet geometry execute:
    * conversion to enclosure bool

In the start method the sequence of execution should be:
if facet geometry :
execute 2nd method (processing of geometry with solids made of facets and enclosures decomposition)
else
execute 1st method (decomposition of standard solids, and enclosures)

execute 3rd method (conversion of solids and enclosure). If flag is facet execute only conversion of enclosure

The geometry made of facets doesn't need decomposition and conversion process. The steps equivalent to these two processes are carried out in the function translate

Copy link
Collaborator Author

@shimwell shimwell May 27, 2024

Choose a reason for hiding this comment

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

Not quite understanding this as there are overlaps in those line numbers so I guess there will be repetition in the 3 methods.

Copy link
Member

@psauvan psauvan left a comment

Choose a reason for hiding this comment

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

see comment on line 691

@shimwell
Copy link
Collaborator Author

shimwell commented May 24, 2024

thanks for the detailed review, I'm going to work on the documentation versioning and come back to this later as it looks like it needs much more thought than I originally estimated. IT would be great if the code allowed more flexibility with the order that things are run so perhaps we even need a few PRs to work on the information flow before finishing this one

@shimwell shimwell marked this pull request as draft May 25, 2024 06:36
@shimwell shimwell closed this by deleting the head repository Nov 25, 2024
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