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

Copy reference planes when copying legends to other docs #2323

Closed
wants to merge 72 commits into from

Conversation

bitsy
Copy link

@bitsy bitsy commented Jul 31, 2024

I've made some changes to ensure that reference planes get copied properly with legend views. The previous code copies all reference planes in legends to the model space (yikes!). This update basically recreates each reference plane in each new legend in the target doc, and it also creates new subcategories and line patterns IF they don't already exist in the target doc.

I'm not familiar with the pyrevit libraries yet, so there may be some less verbose ways to accomplish the same thing. Please feel free to nitpick and suggest changes. :)

@sanzoghenzo, thanks for responding to the issue! Here's the code.

I do have one issue: creating reference plane subcategories through the API causes these new subcategories to be subject to the scale of the view. Typically, a reference plane line weight/pattern is constant scale (scaled to the display rather than the paper space). However, when I create subcategories with the API, the new subcategory line weights/patterns are scaled to the paper space:

Expected Results (Left) -> Actual Results (Right)
image

github-actions bot and others added 30 commits April 30, 2024 21:28
Bumps [mkdocs-material](https://github.com/squidfunk/mkdocs-material) from 9.5.19 to 9.5.21.
- [Release notes](https://github.com/squidfunk/mkdocs-material/releases)
- [Changelog](https://github.com/squidfunk/mkdocs-material/blob/master/CHANGELOG)
- [Commits](squidfunk/mkdocs-material@9.5.19...9.5.21)

---
updated-dependencies:
- dependency-name: mkdocs-material
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Bumps [mkdocstrings](https://github.com/mkdocstrings/mkdocstrings) from 0.25.0 to 0.25.1.
- [Release notes](https://github.com/mkdocstrings/mkdocstrings/releases)
- [Changelog](https://github.com/mkdocstrings/mkdocstrings/blob/main/CHANGELOG.md)
- [Commits](mkdocstrings/mkdocstrings@0.25.0...0.25.1)

---
updated-dependencies:
- dependency-name: mkdocstrings
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Bumps [ruff](https://github.com/astral-sh/ruff) from 0.4.1 to 0.4.3.
- [Release notes](https://github.com/astral-sh/ruff/releases)
- [Changelog](https://github.com/astral-sh/ruff/blob/main/CHANGELOG.md)
- [Commits](astral-sh/ruff@v0.4.1...v0.4.3)

---
updated-dependencies:
- dependency-name: ruff
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
…evelop-4/ruff-0.4.3

Bump ruff from 0.4.1 to 0.4.3
…evelop-4/mkdocstrings-0.25.1

Bump mkdocstrings from 0.25.0 to 0.25.1
…evelop-4/mkdocs-material-9.5.21

Bump mkdocs-material from 9.5.19 to 9.5.21
Bumps [jinja2](https://github.com/pallets/jinja) from 3.1.3 to 3.1.4.
- [Release notes](https://github.com/pallets/jinja/releases)
- [Changelog](https://github.com/pallets/jinja/blob/main/CHANGES.rst)
- [Commits](pallets/jinja@3.1.3...3.1.4)

---
updated-dependencies:
- dependency-name: jinja2
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
…inja2-3.1.4

Bump jinja2 from 3.1.3 to 3.1.4
plane extents from crop instead of level, closes pyrevitlabs#2244, closes pyrevitlabs#2245,
better selection handling,
better doc changed event handling,
API object validity check,
view change necessity check
Faces Counter tools offered by BIM One Inc.
…pdate

Color Splasher - Nonica improvements
Bumps [ruff](https://github.com/astral-sh/ruff) from 0.4.3 to 0.4.7.
- [Release notes](https://github.com/astral-sh/ruff/releases)
- [Changelog](https://github.com/astral-sh/ruff/blob/main/CHANGELOG.md)
- [Commits](astral-sh/ruff@v0.4.3...v0.4.7)

---
updated-dependencies:
- dependency-name: ruff
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Bumps [setuptools](https://github.com/pypa/setuptools) from 69.5.1 to 70.0.0.
- [Release notes](https://github.com/pypa/setuptools/releases)
- [Changelog](https://github.com/pypa/setuptools/blob/main/NEWS.rst)
- [Commits](pypa/setuptools@v69.5.1...v70.0.0)

---
updated-dependencies:
- dependency-name: setuptools
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
Bumps [black](https://github.com/psf/black) from 23.10.1 to 24.4.2.
- [Release notes](https://github.com/psf/black/releases)
- [Changelog](https://github.com/psf/black/blob/main/CHANGES.md)
- [Commits](psf/black@23.10.1...24.4.2)

---
updated-dependencies:
- dependency-name: black
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
Bumps [requests](https://github.com/psf/requests) from 2.31.0 to 2.32.3.
- [Release notes](https://github.com/psf/requests/releases)
- [Changelog](https://github.com/psf/requests/blob/main/HISTORY.md)
- [Commits](psf/requests@v2.31.0...v2.32.3)

---
updated-dependencies:
- dependency-name: requests
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Bumps [mkdocstrings-python](https://github.com/mkdocstrings/python) from 1.10.0 to 1.10.3.
- [Release notes](https://github.com/mkdocstrings/python/releases)
- [Changelog](https://github.com/mkdocstrings/python/blob/main/CHANGELOG.md)
- [Commits](mkdocstrings/python@1.10.0...1.10.3)

---
updated-dependencies:
- dependency-name: mkdocstrings-python
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
…evelop-4/ruff-0.4.7

Bump ruff from 0.4.3 to 0.4.7
jmcouffin and others added 21 commits July 1, 2024 20:10
…evelop-4/setuptools-70.1.1

Bump setuptools from 70.0.0 to 70.1.1
…evelop-4/mypy-1.10.1

Bump mypy from 1.10.0 to 1.10.1
…evelop-4/pygount-1.8.0

Bump pygount from 1.6.1 to 1.8.0
…evelop-4/mkdocs-material-9.5.27

Bump mkdocs-material from 9.5.21 to 9.5.27
…evelop-4/black-24.4.2

Bump black from 23.10.1 to 24.4.2
Removed trailing )
Removed Indentation error
Bumps the pip group with 1 update: [certifi](https://github.com/certifi/python-certifi).


Updates `certifi` from 2024.2.2 to 2024.7.4
- [Commits](certifi/python-certifi@2024.02.02...2024.07.04)

---
updated-dependencies:
- dependency-name: certifi
  dependency-type: indirect
  dependency-group: pip
...

Signed-off-by: dependabot[bot] <support@github.com>
…ip-a8b23622d7

Bump certifi from 2024.2.2 to 2024.7.4 in the pip group
Fixed issue with pallette based icons by converting to RGBA, and added check for existing dark icon so as to not overwrite ones already specifically made.
Update convert_icon_to_dark_mode.py
Translate of all the components in PyRevitTab, with the expection of the Extensions and Settings windows.
Testing to futher commits to develop-4
Added enhancements to ensure that reference planes in legend views are copied to the destination document. Also corrected the command title/tooltip for US English.
@bitsy bitsy changed the base branch from master to develop-4 July 31, 2024 00:58
Copy link
Contributor

@sanzoghenzo sanzoghenzo left a comment

Choose a reason for hiding this comment

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

I didn't test it, but for now I suggest some code style changes for better readability

if src_subcat.Id != src_plane_category.Id:
if src_subcat.Name not in dest_subcats:
p_line_style = DB.GraphicsStyleType.Projection
dest_subcats[src_subcat.Name] = \
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of creating the objects directly inside the dictionaries, use variables to hold them and add them to the dictionaries only at the end.
If you combine this with the comment above, you can create a function that returns the two objects, and the if branch only adds them to the dictionaries:

if src_subcat.Name nor in dest_subcats:
    subcat, pat = create_dest_subcat(....)
    dest_subcats[src_subcat.Name] = subcat
    dest_pats[src_pat.Name] = pat

Copy link
Author

Choose a reason for hiding this comment

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

I reworked this bit of code a bit differently than you suggested. The weird thing about this construct is that if the function returns a solid line pattern, it would get added to the dictionary, which is not desirable. Committing the changes soon...

Moved the bulk of the reference plane matching to functions for readability. Also made other minor corrections.
@bitsy
Copy link
Author

bitsy commented Aug 9, 2024

@sanzoghenzo - I made the suggested changes. I think overall it's a net positive for readability, but my function match_subcategory could still use some clarity when creating line patterns...I'll review this again.

What's a good convention for passing arguments into the function vs. just modifying variables directly from the parent scope? While readable, the functions assumes that variables like dest_subcats and dest_pats already exist, which seems like it makes the code less editable in the future. Having started my programming journey in C++ years ago, I find the "squishiness" of scope in python baffling at times. 😅

@jmcouffin jmcouffin changed the base branch from develop-4 to develop August 16, 2024 12:54
@sanzoghenzo
Copy link
Contributor

Hi @bitsy, sorry for not getting back to you sooner, busy with the revit2025/pyrevit5 thing 😅

As you can see @jmcouffin changed the branch to which merge the change to develop, since we got to a point that version 5 (beta) is somewhat usable; unfortunately this introduced a ton of junk on your PR.
If you don’t want to spend too much time resolving the merge conflict, I suggest you to:

  • backup the files you edited somewhere outside the pyrevit folder
  • Fetch the latest changes from the pyrevitlabs repo (if you didn't do already, add the repo to your remotes as "upstream" or "pyrevitlabs")
  • reset your branch to the latest commit on upstream develop
  • re-add your files
  • commit and force push your edits.

What's a good convention for passing arguments into the function vs. just modifying variables directly from the parent scope?

Generally it is a code smell to modify parent scope variables, as it result in highly coupled code instead of reusable functions.
You could add the variables as function parameters, but this will introduce side effects, that is, you're modifying an input instead of returning a value and avoid touching the input parameters. This is forbidden in functional programming, but allowed (and misused) in a flexible language like python. It's doing object oriented programming without objects (your entire module is your class).

Instead of doing it all in a function, just extract the portions that create and set the attributes of an object (like the dest_subcat) and return that object; the "parent" function will do the existence checks on the dictionary, call the function and insert the resulting object if it isn't already there.

@bitsy
Copy link
Author

bitsy commented Sep 4, 2024

@sanzoghenzo, thank you for your thorough response, and no worries! I myself am neck deep studying for the ARE on top of work... And I super appreciate y'all working on pyRevit 5. I'll do as you suggest: copy my work and reset. I'll also do a little refactoring of those functions to make things cleaner.

@jmcouffin
Copy link
Contributor

I shall close this one then. Thanks @bitsy

@jmcouffin jmcouffin closed this Sep 4, 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.

9 participants