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.

Sign up for GitHub

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

Jump to bottom

Add support for visualizing bones in Advanced Import Settings #78188

Closed
wants to merge 1 commit into from
Closed

Add support for visualizing bones in Advanced Import Settings #78188

wants to merge 1 commit into from

Conversation

jeronimo-schreyer
Copy link
Contributor

When an imported model Skeleton3D type node is selected, the bones are drawn using lines or octahedrons to provide a clearer reference to their position. Refactored Skeleton3DGizmoPlugin::redraw now uses a static function to generate bone meshes

Update
editor/import/scene_import_settings.cpp
editor/import/scene_import_settings.h
editor/plugins/skeleton_3d_editor_plugin.h
editor/plugins/skeleton_3d_editor_plugin.cpp

@jeronimo-schreyer jeronimo-schreyer requested review from a team as code owners June 13, 2023 14:37
@YeldhamDev YeldhamDev added this to the 4.1 milestone Jun 13, 2023
@fire fire changed the title add support for visualizing bones in Advanced Import Settings Add support for visualizing bones in Advanced Import Settings Jun 13, 2023
@fire fire requested a review from a team June 13, 2023 14:44
@akien-mga akien-mga modified the milestones: 4.1, 4.2 Jun 13, 2023
@Calinou
Copy link
Member

Calinou commented Aug 9, 2023

@jeronimo-schreyer Did you forget to push local changes? This PR still has merge conflicts due to not being rebased yet.

@fire
Copy link
Member

fire commented Aug 19, 2023

Can you merge into one commit?

SaracenOne
SaracenOne requested changes Oct 16, 2023
View reviewed changes
Copy link
Member

@SaracenOne SaracenOne left a comment

Choose a reason for hiding this comment

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

This will probably be fine once the translation thing is addressed and the PR is rebased.

editor/import/scene_import_settings.cpp Outdated Show resolved Hide resolved
@AThousandShips AThousandShips modified the milestones: 4.2, 4.3 Oct 31, 2023
@jeronimo-schreyer jeronimo-schreyer force-pushed the add_3d_skeleton_mesh_in_ais branch 2 times, most recently from 65219f4 to 189c1b3 Compare December 10, 2023 22:53
@fire fire requested review from a team and SaracenOne December 11, 2023 01:25
SaracenOne
SaracenOne approved these changes Dec 15, 2023
View reviewed changes
Copy link
Member

@SaracenOne SaracenOne left a comment

Choose a reason for hiding this comment

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

LGTM👍

@YuriSizov
Copy link
Contributor

Could you please amend your commit message to remove this part?

Update
editor/import/scene_import_settings.cpp
editor/import/scene_import_settings.h
editor/plugins/skeleton_3d_editor_plugin.h
editor/plugins/skeleton_3d_editor_plugin.cpp
Conflicts:
	editor/import/scene_import_settings.cpp
	editor/import/scene_import_settings.h

It's not relevant to the changes and doesn't need to be stored in the git history.

YuriSizov
YuriSizov reviewed Dec 15, 2023
View reviewed changes
editor/import/scene_import_settings.cpp Outdated
Comment on lines 416 to 421
Skeleton3D *skeleton = Object::cast_to<Skeleton3D>(p_node);
if (skeleton) {
bones_mesh_preview->set_mesh(Skeleton3DGizmoPlugin::get_bones_mesh(skeleton, -1, true));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This relationship doesn't seem right to me. We shouldn't rely directly on plugin code here, that defeats the purpose of it being a plugin. We also can't use the same pattern going forward if we need to bring more viewport gizmos to the advanced import dialog preview.

I think it's time to integrate viewport/gizmo plugins into this preview, or alternatively devise a separate pluggable interface.

Copy link
Contributor

Choose a reason for hiding this comment

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

We talked about this at the animation asset meeting and we would like this change. Saracen has already approved the technical aspects.

I don't think this is such a big issue since it's a static function and both are in editor/ so there is no linking problem.
@AThousandShips would it be possible to review this usage of a plugin function from the import code and determine if this is ok?

Copy link
Member

Choose a reason for hiding this comment

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

I'll take a look :)

Copy link
Member

Choose a reason for hiding this comment

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

I don't think there's any issue with this, both are fully contained in the editor, and it saves a lot of code duplication, could theoretically move it to Skeleton3D or similar but it being a plugin here doesn't change anything in my opinion, as it's not a module or similar but fully integrated

@fire fire mentioned this pull request Feb 6, 2024
Open
@lyuma lyuma mentioned this pull request Feb 25, 2024
Merged
AThousandShips
AThousandShips approved these changes Feb 27, 2024
View reviewed changes
editor/import/scene_import_settings.cpp Outdated Show resolved Hide resolved
editor/plugins/skeleton_3d_editor_plugin.cpp Outdated Show resolved Hide resolved
editor/plugins/skeleton_3d_editor_plugin.cpp Outdated Show resolved Hide resolved
editor/plugins/skeleton_3d_editor_plugin.cpp Outdated Show resolved Hide resolved
editor/plugins/skeleton_3d_editor_plugin.cpp Show resolved Hide resolved
@lyuma
Copy link
Contributor

lyuma commented Mar 25, 2024

@jeronimo-schreyer do you think you will have time to resolve the pending review feedback?
If not, I can try take a look at it later this week

SaracenOne
SaracenOne requested changes Mar 30, 2024
View reviewed changes
Copy link
Member

@SaracenOne SaracenOne left a comment

Choose a reason for hiding this comment

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

I feel like the skeleton should match the animations when they are being played back. It currently doesn't seem to do that when I tested it.

@clayjohn
Copy link
Member

clayjohn commented Apr 4, 2024

@jeronimo-schreyer do you think you will have time to resolve the pending review feedback? If not, I can try take a look at it later this week

Maybe you should just take over. It would be really nice to have this done for 4.3 and we don't have much more time

@jeronimo-schreyer jeronimo-schreyer force-pushed the add_3d_skeleton_mesh_in_ais branch 2 times, most recently from 96a203f to c61708a Compare April 5, 2024 00:37
@jeronimo-schreyer
add support for visualizing bones in Advanced Import Settings
714527c 
When an imported model Skeleton3D type node is selected, the bones are drawn using lines or octahedrons to provide a clearer reference to their position.
Refactored Skeleton3DGizmoPlugin::redraw now uses a static function to generate bone meshes
@fire fire requested review from lyuma and SaracenOne April 5, 2024 05:50
@AThousandShips AThousandShips modified the milestones: 4.3, 4.4 Jul 25, 2024
@SaracenOne SaracenOne mentioned this pull request Aug 26, 2024
Merged
@akien-mga
Copy link
Member

Superseded by #96094.

@akien-mga akien-mga closed this Aug 26, 2024
@akien-mga akien-mga removed this from the 4.4 milestone Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@YuriSizov YuriSizov YuriSizov left review comments

@AThousandShips AThousandShips AThousandShips approved these changes

@Calinou Calinou Awaiting requested review from Calinou

@lyuma lyuma Awaiting requested review from lyuma

@SaracenOne SaracenOne Awaiting requested review from SaracenOne

Assignees
No one assigned
Labels
archived enhancement topic:animation topic:editor topic:3d
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Show skeleton bones in the Advanced Import Settings preview
10 participants
@jeronimo-schreyer @Calinou @fire @YuriSizov @lyuma @clayjohn @akien-mga @SaracenOne @AThousandShips @YeldhamDev