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

Allow setting armature axis manually #526

Merged
merged 4 commits into from
Jun 30, 2022

Conversation

TackYs
Copy link
Contributor

@TackYs TackYs commented Jun 15, 2022

@niftools/blender-niftools-addon-reviewer

Overview

When importing armatures, axis are currently set automatically by the guess_orientation function in nif_import/armature/init.py

# use heuristics to determine a suitable orientation 
forward, up = self.guess_orientation(n_armature) 
# pass them to the matrix utility 
math.set_bone_orientation(forward, up)

Some times it would be valuable to be able to set these manually, example is if you have a rigged armature in the scene with a specific orientation and want to import another armature with matching orientation.

Detailed Description

[List of functional updates]

Fixes Known Issues

[Ordered list of issues fixed by this PR]

Documentation

[Overview of updates to documentation]
I put in a block in the draw() call for setting axis if guess orientation is checked, that together with the operator descriptions is hopefully clear enough?

Testing

[Overview of testing required to ensure functionality is correctly implemented]
Import nif skeleton with guess armature checked, then import the same armature again with different orientations checked.
Compare bone rolls.

Manual

[Set of steps to manually verify updates are working correctly]

Automated

[List of tests run, updated or added to avoid future regressions]

Additional Information

[Anything else you deem relevant]

@TackYs
Copy link
Contributor Author

TackYs commented Jun 17, 2022

Maybe the checkbox option should be labeled "detect armature orientation" instead of "guess armature orientation"

@HENDRIX-ZT2
Copy link
Contributor

Maybe the checkbox option should be labeled "detect armature orientation" instead of "guess armature orientation"

Or Override Axis Armature Orientation? I feel like if you have this set manual orientation feature, and by default the manual setting would be off, we should name the toggle accordingly so you know what happens when you check it -> ie. you need to set the axes manually in the widget that appears.

@TackYs
Copy link
Contributor Author

TackYs commented Jun 20, 2022

I agree, changing it to an Override option with default False makes much more sense!
Will update PR.

My internet is out since yesterday so ill commit when ( if? D: ) it comes back

@neomonkeus
Copy link
Member

Good to merge?

@HENDRIX-ZT2
Copy link
Contributor

@TackYs @neomonkeus should try to implement orientation_helper instead of hardcoding the axes

@TackYs
Copy link
Contributor Author

TackYs commented Jun 29, 2022

Had a go at using orientation_helper as per Hendrix suggestion
As a result it was possible to remove most of the extra properties defined in NifImportOperator in the first commits, seem to work well.

Split the override settings into a separate panel - optional, can go back to one panel. Not really sure what feels best.

Copy link
Contributor

@HENDRIX-ZT2 HENDRIX-ZT2 left a comment

Choose a reason for hiding this comment

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

Great!

default=True)

#Specify axis_forward
armature_axis_forward: bpy.props.EnumProperty(
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a utility for this:
bpy_extras.io_utils.orientation_helper(axis_forward='Y', axis_up='Z')
https://docs.blender.org/api/current/bpy_extras.io_utils.html

Copy link
Contributor

Choose a reason for hiding this comment

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

Have you looked into this? Should make the code more elegant :)

layout.prop(operator, "guess_armature_orientation")
row_ax_fwd = layout.row()
row_ax_fwd.prop(operator, "armature_axis_forward")
row_ax_fwd.enabled = not operator.guess_armature_orientation #Grey out if guessing orientation is requested
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps don't draw the axes at all?
also comment format for pep (2 spaces before inline #, 1 after)

io_scene_niftools/ui/operators/nif_import.py Outdated Show resolved Hide resolved
io_scene_niftools/ui/operators/nif_import.py Outdated Show resolved Hide resolved
@HENDRIX-ZT2 HENDRIX-ZT2 merged commit dd66c97 into niftools:develop Jun 30, 2022
@Candoran2 Candoran2 mentioned this pull request Jun 20, 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.

None yet

3 participants