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

Add copy components support #197

Merged
merged 9 commits into from
Mar 23, 2023
Merged

Add copy components support #197

merged 9 commits into from
Mar 23, 2023

Conversation

keianhzo
Copy link
Contributor

This PR adds copy properties support, a functionality that we lost in the add-on update.

@Exairnous
Copy link
Contributor

Hey, nice! I'd forgotten that this was present in the last version.
I found a couple issues and have some thoughts though.

  • Copying won't work for bones that are present in the same armature.
  • The operator is missing the {'REGISTER', 'UNDO'} options.
  • Is there a reason why they're being limited to a single mode? I think it should support Object, Edit, and Pose at least, and generally it's better UI practice to grey out/disable the button rather than hide it (hiding it on scenes seems good to me for the moment, but it might be nice to add a more traditional copy/paste for them in the future).
  • Most Blender collection properties/lists support both keyword and index access, so when appending the bones to selected objects you can just do selected_objects.append(target_armature_bones[target_bone.name])
  • Speaking of using a selected objects variable to store bones, I really think we should standardize on "host" as what we call the thing that has a component on it. I've tried to do this in all of my code and I think it makes things much clearer because objects are a specific thing in Blender and sometimes passed along with what the component is on, plus I think it's much easier to remember that you may have to deal with multiple data types when the variable has "host" in it as opposed to "obj" or "ob" which suggest that you're dealing only with objects and not with scenes, materials, etc.. So I'd personally rename selected_objects/src_obj to selected_hosts/src_host.

@keianhzo
Copy link
Contributor Author

Thanks for the feedback. I think I have address all of it.

Copy link
Contributor

@Exairnous Exairnous left a comment

Choose a reason for hiding this comment

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

Thanks. I left a couple small comments, but overall it looks good.

addons/io_hubs_addon/components/operators.py Outdated Show resolved Hide resolved
addons/io_hubs_addon/components/ui.py Show resolved Hide resolved
@keianhzo
Copy link
Contributor Author

Updated. Thanks!

@keianhzo keianhzo added this to the 1.1.0 milestone Mar 20, 2023
@keianhzo keianhzo merged commit ac9fa2e into master Mar 23, 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.

2 participants