This branch contains multiple bugfixes and improvements. #16
Conversation
…mments in the code which explain the changes made but needs to be removed when merging into main Bugfixes: 1. _leave did not reset the character position after the animation finished. _enter on the other side needs the sprite to start from its original position. I fixt that by resetting the sprite when the tween finishes. 2. Pressing ui_accept causes animations to be skipped. As the same action is required to advance text, every animation except the first one was skipped. I removed that animation skip functionality, as it's required in my opinion and solves that issue. ------ Improvements: 1. There can now be 4 characters on screen instead of 2. The added ones are named left_center and right_center 2. Now only the talking character gets focus and is displayed in color. All the others are grey. 3. Characters can leave the screen now and talk from the off without becoming visible again. I added a new animation called -hidden- for that cause.
…n other one gets focus. MAde the game to quit and not end in a black screen when the last scene gets played
There was a problem hiding this comment.
I see a couple of issues with those changes. For one, darkening the characters like that isn't necessarily an improvement. It at least feels incomplete. For instance, it probably shouldn't be instant.
I refactored the code on my end. A few things you do make it a bit more complicated than necessary - the system probably needs more of a rewrite too if you're to handle more than two characters and layer new animations. I've got to do more testing and think about it.
I left some general comments but there's nothing for you to directly change right now
| # NOTE: | ||
| # This code does not make sense | ||
| # As soon as you press ui_accept the animation is skipped | ||
| # That always happens when the player presses Enter/Space to advance the text | ||
| # That way animations will never play except for the first animation or when animations play automatically | ||
| # |
There was a problem hiding this comment.
This is intentional. This is so when advancing text to the end you also skip the animations. Suppose you have longer animations: you don't want to just let them run without limits past the end of the text. Now, tweens can be a bit finicky so there's a need for some bug fixing there.
There was a problem hiding this comment.
The problem here is that an animation will NEVER play as the player always presses ui_accept whenever they want to advance conversation.
What is needed is some mechanism which only lets you skip the animation if ui_accept is held for XX amount of time.
Maybe there is a function for that and I don't need to add a timer for it, which would make it all much easier.
| # if event.is_action_pressed("ui_accept") and _tween.is_active(): | ||
| # _tween.seek(INF) |
There was a problem hiding this comment.
In a PR, you shouldn't leave dead code. But in this case, this code shouldn't be removed, if it doesn't work well, it should be fixed instead.
| else: | ||
| _displayed[side] = character | ||
|
|
||
|
|
||
| _determine_focus(character.id, side, sprite) |
There was a problem hiding this comment.
No need to split the code to a new function that's only being used once. It only creates an indirection when reading the code (you have to jump to another place, then come back)
There was a problem hiding this comment.
I disagree with that.
An indirection is not always bad for the reader.
In my opinion it is much better to make a function like that, than to put everything in one big function and add comments.
Someone who wants to understand my code can skit "_determine_focus" if them is not interested in that piece of code.
If everything was in one function, it's much harder to get a hold of the big picture.
|
I'm unable to work on this right now. I did some refactoring on my side last time, but I would implement the features differently, starting back from the requirements. When coding this for the course, I intentionally made the system limited and specific. So some of the code is highly specific. To expand it, there's a need to rethink in particular how characters work within the system. It'd also affect the course as this project's directly linked to it, so that's why it's quite a bit of work and I'm unable to tackle now. The first bugfix regarding the leave animation would still be welcome. The other isn't a bug, or maybe there's a bug, but skipping animations along with the corresponding text is intentional, as commented in the PR review. If it doesn't skip all animations, then that bit isn't intended. Now, I'm not a big player of visual novels, so if in most VN games they still play animations when skipping text and that's what players of those games expect, then the change would be welcome. Do you know more about that perhaps? |
|
And once again, thanks for taking the time to contribute. I hope you understand reviewing and adapting code + the course takes time I don't have right now, and that it's not rejecting your work in itself. |
I expanded that system just for my personal needs.
As mentioned above, your code does skip every animation as there is no check for how long the ui_accept button is held.
I too am not a big player of visual novels. What some games (not VN but Fire Emblem like games) do is to not skip all the text and character animation instantly, but play them at a much higher speed. |
Don't worry, I understand. I will make a PR then only containing the bugfixes. |
|
@Nino-Bock if you still looking for something like this check out my Rakugo Dialogue System: |
There are comments in the code which explain the changes made but needs to be removed when merging into main
Bugfixes:
_leave did not reset the character position after the animation finished. _enter on the other side needs the sprite to start from its original position. I fixed that by resetting the sprite when the tween finishes.
Pressing ui_accept causes animations to be skipped. As the same action is required to advance text, every animation except the first one was skipped. I removed that animation skip functionality, as it's required in my opinion and solves that issue.
Improvements:
There can now be 4 characters on screen instead of 2. The added ones are named left_center and right_center
Now only the talking character gets focus and is displayed in color. All the others are grey.
Characters can leave the screen now and talk from the off without becoming visible again. I added a new animation called -hidden- for that cause.