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 tutorials for blend animation and retargeting #6322

Merged

Conversation

TokageItLab
Copy link
Member

Added a section about the Blend animation which has been changed significantly in Godot 4.0, and a new page describing about the importer retarget.

In particular, the scene importer does not have a class reference, so it is necessary to explain it in the documentation.

Fixes godotengine/godot#59536
Fixes godotengine/godot#65771

@TokageItLab TokageItLab added enhancement content:new page Issues and PRs related to creation of new documentation pages for new or undocumented features labels Oct 17, 2022
@TokageItLab TokageItLab requested review from fire and a team October 17, 2022 07:46
tutorials/animation/animation_tree.rst Outdated Show resolved Hide resolved
tutorials/assets_pipeline/retargeting_3d_skeleton.rst Outdated Show resolved Hide resolved
tutorials/assets_pipeline/retargeting_3d_skeleton.rst Outdated Show resolved Hide resolved
tutorials/assets_pipeline/retargeting_3d_skeleton.rst Outdated Show resolved Hide resolved
@TokageItLab TokageItLab force-pushed the tuts-for-blend-and-retarget branch 2 times, most recently from 8a9db84 to c263fe0 Compare October 17, 2022 09:23
@mhilbrunner
Copy link
Member

Left a few comments. All in all, I feel like this PR needs some attention by a natural speaker, as I kind of struggled with whether my difficulty in following along was because of the language or due to my lack of familiarity with the topic.
I've left notes where I could to indicate where I think improvements could be made, but if someone like @clayjohn or someone else could give this a glance too, I'd be thankful. :)

Besides that, we need to come to a decision regarding the use of .webp for images and animated images, I'll try to get that done this week before we merge this.

Thanks for contributing!

@TokageItLab TokageItLab force-pushed the tuts-for-blend-and-retarget branch from c263fe0 to 3f8b871 Compare October 18, 2022 03:53
Copy link
Member

@mhilbrunner mhilbrunner left a comment

Choose a reason for hiding this comment

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

We just decided on the move to .webp, and seeing the file size savings here, this indeed seems to be a good improvement :)

If you can replace the .pngs with .webp (lossless if <250kb, otherwise use quality settings that seem like the best fit) and squash commits, that'd be appreciated!

@TokageItLab
Copy link
Member Author

@mhilbrunner

quality settings

Is there clear criteria like a target size?

@TokageItLab TokageItLab force-pushed the tuts-for-blend-and-retarget branch from ed44f87 to 6a66104 Compare October 19, 2022 18:09
@TokageItLab
Copy link
Member Author

TokageItLab commented Oct 19, 2022

@mhilbrunner I changed the images to webp and squashed commits. PNG images have been made webP with lossless.

There is no clear target size given for the animated images, but I compressed them with quality=75 as ffmpeg default for now. It appears a bit noisy, but I think it's acceptable. If necessary, I will re-encode with cleaner settings. FYI if use quality=100, these animated images size will be about tripled.

@mhilbrunner
Copy link
Member

Nice work! This is currently in the Asset Pipeline section, which makes sense as it mostly relates to import settings and importing skeletons I guess (from our/the engine's perspective).

Question is if a new user would be more likely to look under the Animation section? Opinions? I'm fine with leaving it where it is, just want to bring it up.

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Looks mostly good! I left some suggestions where the language can be cleaned up a bit.

One thing that I did not comment on in the doc is the use of capitals for important terms. I found that I wasn't sure if things were capitalized because they matched names in the animation system (i.e. they are properties or class names) or because they were something the document was referring to multiple times.

In general, I think it is more clear to readers to:

  1. Link to classes when speaking about a class (like SkeletonProfileHumanoid)
  2. use backticks to put things in a code block when you are referring to a Godot property or method

tutorials/assets_pipeline/retargeting_3d_skeletons.rst Outdated Show resolved Hide resolved
tutorials/assets_pipeline/retargeting_3d_skeletons.rst Outdated Show resolved Hide resolved
tutorials/assets_pipeline/retargeting_3d_skeletons.rst Outdated Show resolved Hide resolved
tutorials/assets_pipeline/retargeting_3d_skeletons.rst Outdated Show resolved Hide resolved
tutorials/assets_pipeline/retargeting_3d_skeletons.rst Outdated Show resolved Hide resolved
tutorials/assets_pipeline/retargeting_3d_skeletons.rst Outdated Show resolved Hide resolved
tutorials/assets_pipeline/retargeting_3d_skeletons.rst Outdated Show resolved Hide resolved
tutorials/assets_pipeline/retargeting_3d_skeletons.rst Outdated Show resolved Hide resolved
tutorials/assets_pipeline/retargeting_3d_skeletons.rst Outdated Show resolved Hide resolved
tutorials/assets_pipeline/retargeting_3d_skeletons.rst Outdated Show resolved Hide resolved
@TokageItLab TokageItLab force-pushed the tuts-for-blend-and-retarget branch from a6d749d to 703657e Compare October 20, 2022 03:52
@TokageItLab
Copy link
Member Author

TokageItLab commented Oct 20, 2022

@mhilbrunner @clayjohn I embedded links for notable classes and used code brackets for properties in the importer.

About some camel words:

  • Skeleton: This refers to the Skeleton3D class, but it is always "3D" added, which is annoying and I shortened
  • Position track: This is used to refer to the animation track, which is clearly a separate type, but it is defined by enum not independent class
  • Bone Rest, Bone Name: These exist as rest and name properties, but there are no properties called bone_rest or bone_name

@TokageItLab TokageItLab force-pushed the tuts-for-blend-and-retarget branch from 703657e to 046bcd4 Compare October 21, 2022 04:46
@TokageItLab TokageItLab requested a review from clayjohn October 21, 2022 07:04
@mhilbrunner
Copy link
Member

At this point, I'm happy to merge as-is and iterate where needed. Thanks for these huge additions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
content:new page Issues and PRs related to creation of new documentation pages for new or undocumented features enhancement
Projects
None yet
6 participants