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

Update 2_create_player_character.rst #57

Open
wants to merge 1 commit into
base: devel
Choose a base branch
from

Conversation

elvisish
Copy link

made it slightly easier to understand

@@ -204,8 +204,7 @@ specific animations for the character for each direction angle:
The ``Speak`` animations are optional and only required if your game needs
them.

For each direction angle, put the name of the matching animation (with the name
specified in "Adding a walkcycle" above) in the "Animation" field, and choose
For each direction angle, each slot needs to contain a ``ESCAnimationName`` resource. Put the name of the matching walking animation (with the name specified in "Adding a walkcycle" above) in the "Animation" field, and choose
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
For each direction angle, each slot needs to contain a ``ESCAnimationName`` resource. Put the name of the matching walking animation (with the name specified in "Adding a walkcycle" above) in the "Animation" field, and choose
Each slot for the above animations needs to contain a ``ESCAnimationName`` resource,
corresponding to each direction angle. Put the name of the matching animations (using
the names specified in "Adding a walkcycle" above) in their respective "Animation" fields,
and choose whether each animation should be played mirrored by selecting the
"Mirrored" checkbox.

Copy link
Author

Choose a reason for hiding this comment

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

The escanimationname resource should go before anything else, otherwise the user following the tutorial won't understand what to use until they scroll below the images that use the resource.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand what your argument is: The suggested change I made follows your change, but uses slightly different verbiage. ESCAnimationName still appears in exactly the same place you had it.

Copy link
Author

Choose a reason for hiding this comment

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

Sure thing, I'm new to pull requests so I wasn't entirely sure what was approved or not, sorry!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, no worries. Yeah, GitHub's PR interface takes a little getting used to. So far, nothing's been "approved", but I have requested a change to be made (above) before I decide on whether to accept the PR.

Note that, for this project, approval requires "acceptance" from at least two of the devs.

Copy link
Author

Choose a reason for hiding this comment

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

Ahh thanks for the explanation!

Copy link
Collaborator

@balloonpopper balloonpopper May 15, 2022

Choose a reason for hiding this comment

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

I'd actually like to overhaul this part of the documentation as I think we could explain the world angle to ESCAnimation resource to actual animation mechanism a lot better. Happy to run with changing this section in the interim.

Comment on lines 208 to 209
whether that animation should be played mirrored by selecting the "Mirrored"
checkbox.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
whether that animation should be played mirrored by selecting the "Mirrored"
checkbox.

@StraToN
Copy link
Contributor

StraToN commented Jul 5, 2022

What is the status on this? There are some doc8 checks failing (most probably trailing empty spaces).

@BHSDuncan
Copy link
Collaborator

@elvisish care to update us on this?

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.

4 participants