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

[player] Take clipping into account when calculating animation viewport #2219

Closed
Disorrder opened this issue Dec 30, 2022 · 4 comments
Closed
Assignees
Labels

Comments

@Disorrder
Copy link

Disorrder commented Dec 30, 2022

private calculateAnimationViewport (animation: Animation, viewport: Viewport) {

I suggest to calculate viewport size by clipping, but not min-max vertices. Sometimes animation goes out of bounds and player fits viewport size to not visible parts. It's unexpected behaviour for me, so I have to setup viewport manually which is taking much time and exhausting.
See example, I expect to see these two animations in same bounding box, but 2nd orange jump outside of bottom border

(Could try to provide PR later if you agree with idea. But only for JS platform)

image

image

@badlogic badlogic changed the title Calculate animation viewport by clipping if exist [player] Take clipping into account when calculating animation viewport Dec 30, 2022
@badlogic
Copy link
Collaborator

Oh, that's actually a bug! This method must absolutely incorporate clipping. I'll try to get this fixed after my vacation, unless you send a PR before that!

@Disorrder
Copy link
Author

@badlogic This method doesn't incorporate clipping because it's described inside skin category. But method only read animation params

@badlogic
Copy link
Collaborator

The method itself doesn't care where attachments, including clipping attachments, come from. What it needs to do is simulate the logic of SkeletonRenderer and clip region and mesh attachments when a clipping attachment is active, then take their min/max.

@davidetan
Copy link
Collaborator

This has been fixed with 03f8f67 and f309722.
See #2220 (comment) for more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging a pull request may close this issue.

4 participants