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

Feature: Flip dialog portrait avatar based on character direction #298

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

edmundito
Copy link
Contributor

This change flips the avatar image of the character based on the character's direction.

Work in Progress

This probably needs a bit more work before it's ready for a final review. I will add a checklist here:

  • Avoid the private character var _looking
  • Let the developer determine whether to flip the avatar
  • Let the developer set the avatar portrait direction

@@ -30,12 +35,16 @@ func _update_avatar(chr: PopochiuCharacter, _msg := '') -> void:
E.scale if E.settings.scale_gui else Vector2.ONE
)

var flip_h := _LOOKING_LEFT_DIRS.has(chr._looking_dir)
Copy link
Contributor Author

@edmundito edmundito Sep 29, 2024

Choose a reason for hiding this comment

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

It looks like _looking_dir is being misused publically in other singletons should either be a getter/setter or a public variable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, I guess that's very legacy code that nobody touched in a while. Nice spot!

Copy link
Collaborator

Choose a reason for hiding this comment

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

You're right @edmundito . We can make that property public (by removing the underscore at the beggining) or use getter/setter methods. I prefer the first option.

Changing that will also affect the i_room.gd and the popochiu_room_data.gd scripts. And this line of code, of course.

@stickgrinder
Copy link
Collaborator

I like this one a lot! Have you got an idea on the API functions to expose for avatar direction management, already?
I would discuss them here so we can decide on their signatures before implementing them :)

@stickgrinder stickgrinder added the improvement New feature or request label Sep 30, 2024
@@ -5,6 +5,11 @@ extends PopochiuDialogText
@onready var right_avatar_container: PanelContainer = %RightAvatarContainer
@onready var right_avatar: TextureRect = %RightAvatar

const _LOOKING_LEFT_DIRS := [
Copy link
Collaborator

Choose a reason for hiding this comment

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

Following Godot's style guide, constants should be defined without underscore on the first character.

Suggested change
const _LOOKING_LEFT_DIRS := [
const LOOKING_LEFT_DIRS := [

@mapedorr
Copy link
Collaborator

mapedorr commented Oct 1, 2024

Interesting feature.

I’m afraid relying solely on whether it’s facing left might not be enough. Maybe (and this can be part of that API @stickgrinder is wondering about) characters should have a function to know if their Sprite is flipped or not, and based on that, the avatar will flip too (if it’s set up to flip with the character).

@stickgrinder
Copy link
Collaborator

Hi @edmundito!

Did you, by any chance, managed to do more work on this PR?
If you don't plan to do more work on that, me or @mapedorr can take this over.

Let us know, thanks.

@edmundito
Copy link
Contributor Author

I have not and may not have the time. Feel free to take over!

@stickgrinder
Copy link
Collaborator

Thanks @edmundito!

We'll add you as reviewer, should you have any comments or suggestion (since you actually needed this feature)!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement New feature or request
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

3 participants