Added new typewriter effect #264
Conversation
|
Thanks. I just tried running it live and it didn't do what I expected... I think there are a couple of bugs and some possible improvements we could make here. I'll try to add some comments to the code to explain. |
|
Hope that all makes sense... Do you think you can make those changes? |
|
For sure. Document the issues you see here, and I'll try writing up some more extensive examples to look at on my end (I was using a SpeechBubble static renderer) |
|
Thanks. Detailed comments are in the review. Main bug I saw was that the first column of text was offset from the rest on my test. Hope that helps! |
|
Thanks for taking the time to review! Only, I can't seem to find it- have you published it yet? This could also perhaps be one of the Github redesign's quirks |
| """ | ||
|
|
||
| def __init__(self, screen, renderer, y, colour, **kwargs): | ||
| """ |
There was a problem hiding this comment.
It might also be good to offer a couple more options in here.
- Speed - people have their own feelings on how fast these effects should run, so we should probably take a speed option that simply affects how often we run the update. Simply remember here and update line 435 to use the speed instead of "2".
- Line by line - some poeple might use this effect to draw standard text one character at a time. It would be good if we could offer that as an option too.
| return | ||
|
|
||
| y = self._y | ||
| j = self._rendered_y |
There was a problem hiding this comment.
This isn't right. You use it to access line[j], which means it is the X offset within the text you want to display, not anything to do with the Y coordinates. I suspect that you want to maintain a counter of where you are in the text instead.
| if self._rendered_x == 0: | ||
| x = (self._screen.width - len(line)) // 2 | ||
| else: | ||
| x = self._rendered_x |
There was a problem hiding this comment.
Right now you don't have to do this for every iteration in the loop. You could do it once outside. This would change if we implement the line, by line typewriter option, though.
| colours[i][j][1]) | ||
| else: | ||
| self._screen.print_at(line[j], x, y, self._colour) | ||
| x += 1 |
There was a problem hiding this comment.
You shouldn't be incrementing x inside the loop that is displaying a single vertical line of the text. This only works because you current recalculate x earlier in the loop.
|
@mjip Any luck with those changes? |
|
I haven't had the time to take a look lately, but I'm hoping to revisit it next week. Thanks for the review though! |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Issues fixed by this PR
Closes #263
What does this implement/fix?
Any other comments?