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

Big clean up (through a PR, as requested) #29

Merged
merged 38 commits into from
Feb 26, 2024
Merged

Big clean up (through a PR, as requested) #29

merged 38 commits into from
Feb 26, 2024

Conversation

tomvanmele
Copy link
Collaborator

What type of change is this?

  • Bug fix in a backwards-compatible manner.
  • New feature in a backwards-compatible manner.
  • Breaking change: bug fix or new feature that involve incompatible API changes.
  • Other (e.g. doc update, configuration, etc)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • I added a line to the CHANGELOG.md file in the Unreleased section under the most fitting heading (e.g. Added, Changed, Removed).
  • I ran all tests on my computer and it's all green (i.e. invoke test).
  • I ran lint on my computer and there are no errors (i.e. invoke lint).
  • I added new functions/classes and made them available on a second-level import, e.g. compas.datastructures.Mesh.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added necessary documentation (if appropriate)

src/compas_model/model/element_node.py Outdated Show resolved Hide resolved
src/compas_model/model/group_node.py Outdated Show resolved Hide resolved
@petrasvestartas
Copy link
Collaborator

I reviewed the last clean-up.
If you want, I can reformat elements from __temp folder. Or shall I wait until it is finalized?

@tomvanmele
Copy link
Collaborator Author

i think we should wait because it is not yet clear t me what to do with the geometry of the elements

def __init__(self, model: compas_model.model.Model, **kwargs):
super().__init__(item=model, **kwargs)
self.model = model
self.show_hierarchy = False
Copy link
Collaborator

@petrasvestartas petrasvestartas Feb 21, 2024

Choose a reason for hiding this comment

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

What would be the most basic visualization of interactions applicable to all elements, regardless of interaction type?

Cases:
a) When visualizing a pair of elements, they are colored.
b) However, how would you display adjacencies between multiple or all elements (without using a form)? I often need to check visually what the collider detected and to which elements, specially when dealing with different tolerances.

This question pertains to the interaction attribute of the model class.


def __init__(self, name=None, value=None):
# type: (str | None, object | None) -> None
def __init__(self, name=None):
Copy link
Collaborator

Choose a reason for hiding this comment

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

What would be the mechanism behind to carry information other than name and indices of the interface?

Would you need to implement your own class that inherits interface with attributes you need?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not sure i understand what you mean. can you give a concrete example?

Copy link
Collaborator

@petrasvestartas petrasvestartas Feb 22, 2024

Choose a reason for hiding this comment

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

One example I had:

  1. I define interfaces through collision detection.
  2. then add interfaces with a pair of "features" at the interface level
  3. lastly I add geometry from the detected interface to the individual element features.

print("=" * 80)
def element_worldtransformation(self, element):
# type: (Element) -> compas.geometry.Transformation
"""Compute the element transformation to hte world coordinate system
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo: hte -> the.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What would be the simplest example to use element_worldtransformation?

attr.update(kwargs)
attr["name"] = name
super(GroupNode, self).__init__(**attr)
self._frame = frame
Copy link
Collaborator

@petrasvestartas petrasvestartas Feb 23, 2024

Choose a reason for hiding this comment

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

If setter and getter, sets and gets the same frame, why not to make the frame property simply public?
self.frame = frame

If this is not meant to be public, then maybe WorldXY is the default value as your wrote in the model?

    @property
    def frame(self):
        # type: () -> compas.geometry.Frame
        if not self._frame:
            self._frame = Frame.worldXY()
        return self._frame

print("Element Groups")
print("=" * 80)
print("n/a")
print("=" * 80)

# =============================================================================
# Attributes
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where is the frame assigned? I see only getter:

    @property
    def frame(self):
        # type: () -> compas.geometry.Frame
        if not self._frame:
            self._frame = Frame.worldXY()
        return self._frame

@tomvanmele tomvanmele merged commit 03cb0f2 into main Feb 26, 2024
11 checks passed
@tomvanmele tomvanmele deleted the cleanup-1 branch February 26, 2024 12:37
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