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

Show beam ref sides #378

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Show beam ref sides #378

wants to merge 9 commits into from

Conversation

obucklin
Copy link
Contributor

This changes Show_beam_faces gh component to Show_beam_ref_side, which now takes an int index and shows the corresponding face including origin corner.

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_timber.datastructures.Beam.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added necessary documentation (if appropriate)

Copy link
Contributor

@chenkasirer chenkasirer left a comment

Choose a reason for hiding this comment

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

could you please add a screenshot of what the new component shows vs. the one you deleted?

@obucklin
Copy link
Contributor Author

old component:
image

new component:
image

@obucklin
Copy link
Contributor Author

could you please add a screenshot of what the new component shows vs. the one you deleted?

we could also combine them to still have a text label and show the origin corner. The big question is index starts at 0 or 1?

@chenkasirer
Copy link
Contributor

we could also combine them to still have a text label and show the origin corner. The big question is index starts at 0 or 1?

I find it hard to climb down from my zero-based tree ;) I vote for 0.
I like the idea of combining, maybe show all the labels but highlight the chosen side?

@papachap
Copy link
Contributor

Same here, I think combining the components would be better. The old one was handy for quickly verifying the ref side with the tags. Also, I totally agree that showing the origin corner makes sense for clarity.

As for indexing, I'd stick to the zero-based too :)

@obucklin
Copy link
Contributor Author

obucklin commented Feb 5, 2025

image I propose we get rid of the line and use the text to show the origin and orientation of the ref side...

@obucklin obucklin requested a review from chenkasirer February 5, 2025 12:28
@obucklin
Copy link
Contributor Author

obucklin commented Feb 5, 2025

I made the suggested changes:

  • Index starts at 0
  • Text labels on all faces
  • Text labels now located at ref_side origin to show RefSide orientation
  • ref side is shown as a surface based on index input

cheers!

Copy link
Contributor

@chenkasirer chenkasirer left a comment

Choose a reason for hiding this comment

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

This looks great, just fix an icon

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